Skip to content

fixes #25726; adds sink/var when match or type param#25779

Open
demotomohiro wants to merge 3 commits into
nim-lang:develfrom
demotomohiro:fix-25726
Open

fixes #25726; adds sink/var when match or type param#25779
demotomohiro wants to merge 3 commits into
nim-lang:develfrom
demotomohiro:fix-25726

Conversation

@demotomohiro
Copy link
Copy Markdown
Contributor

No description provided.

@demotomohiro demotomohiro marked this pull request as ready for review May 1, 2026 18:14
@demotomohiro
Copy link
Copy Markdown
Contributor Author

var T doesn't work with or type.
It can be fixed in the same way as this PR, but that fix can be breaking change(compiling tests/arc/trtree.nim fails).

Following code compiles without error because typeof(x) is evaluated to int:

proc test(x: var int): typeof(x) =
  var y: typeof(x)
  return y

var a = 1
echo test(a)

In following code, typeof(x) is int after test is instantiated.
But if I fixed the bug, typeof(x) becomes var int and generates compile error.

proc test(x: string or var int): typeof(x) =
  var y: typeof(x)
  return y

var a = 1
echo test(a)

Fixing the bug and keep typeof(x) is int, not var int seems hard.
When instantiating a generic proc, typeof(x) is already expanded and there is no way to know whether tyVar in return type is a result of typeof(x) or user wrote var int.

@Araq
Copy link
Copy Markdown
Member

Araq commented May 9, 2026

typeof can grow another variant KeepTypeModifiers but the defaults must remain the same.

Comment thread tests/generics/tor_sink_param.nim Outdated
@@ -0,0 +1,12 @@
# issue #25726

proc testOrSink(v: string | sink seq[int]) =
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes no sense to allow for inconsistent type modifiers for an or-type. The case we should support is this:

proc testOrSink(v: sink (string | seq[int]))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants