implement genericsOpenSym for symchoices #23873

metagn · 2024-07-21T22:34:33Z

The node flag nfOpenSym implemented in #23091 for sym nodes is now also implemented for open symchoices. This means the intended behavior is still achieved when multiple overloads are in scope to be captured, so the issue is fixed. The code for the flag is documented and moved into a helper proc and the experimental switch is now enabled for the compiler test suite.

arnetheduck · 2024-07-22T06:45:19Z

tests/generics/tmacroinjectedsym.nim

+    value
+
+  type
+    Result[T, E] = object


lol we really need to stop copy-pasting this version of Result that has all the previous nim bug workarounds in it ;)

Making the test cases reflect real world code has its advantages too.

metagn · 2024-07-22T19:30:57Z

compiler/ast.nim

@@ -331,6 +331,8 @@ type
    nfHasComment # node has a comment
    nfSkipFieldChecking # node skips field visable checking
    nfOpenSym # node is a captured sym but can be overriden by local symbols
+              # ideally a unary node containing nkSym/nkOpenSymChoice or an
+              # extension over nkOpenSymChoice


To elaborate on this (extension over nkOpenSymChoice):

If some version of #23104 is implemented (IMO the node flag for this instead is the lesser of 2 evils, but changing the representation for it is easier anyway), we could replace every nkSym instance of nfOpenSym with a unary nkOpenSymChoice that always prefers its first node, then add the symbol-replacing logic to nkOpenSymChoice in general, so we don't need nfOpenSym anymore. The condition for the new symbol replacing the symchoice would then become s2 != symChoice.preferredSym and s2 notin OverloadableSyms. The only problem is making sure the compiler can handle nkOpenSymChoice instead of nkSym in those spots.

The reason I couldn't implement these in a merged manner before is because I couldn't think of the symbol replacing logic we have now that only checks symbols in the generic proc instantiation.

Give a new node kind a try please, I consider every single flag a hack that lives on for far too long.

I'll try it in another PR, I think the merging with nkOpenSymChoice should still eventually be done so we might end up deprecating the new node kind, at least maybe before the experimental switch is default.

I'm also not sure about the best representation for #23104, this seems wrong to me:

OpenSymChoice Preferred Sym "foo" Sym "foo" ...

Adding nkPreferredOpenSymChoice and nkPreferredClosedSymChoice seems kind of excessive too.

An option that isn't as intuitive but should be easier for the compiler/macros to deal with is to leave the first child of the symchoice empty/as an ident node if there isn't a preferred choice, and to make the preferred choice the first child if there is one.

# no preference: OpenSymChoice Empty # or Ident "foo" Sym "foo" Sym "foo" # first Sym "foo" is preferred: OpenSymChoice Sym "foo" Sym "foo" ...

github-actions · 2024-07-25T20:14:45Z

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from 469a604

Hint: mm: orc; opt: speed; options: -d:release
173295 lines; 8.032s; 652.078MiB peakmem

arnetheduck · 2024-08-06T06:51:30Z

tests/generics/tmacroinjectedsym.nim

+    let x = f().valueOr:
+      return $error
+    "ok"
+  doAssert g(int) == "error"


this should be f

arnetheduck · 2024-08-06T06:52:17Z

Unfortunately, this PR does not fix the issue - the wrong test case was used so it merely shows that it is an issue :/

actually fixes nim-lang#23865 following up nim-lang#23873

actually fixes #23865 following up #23873 In the handling of `nkIdent` in `semExpr`, the compiler looks for the closest symbol with the name and [checks the symbol kind](https://github.com/nim-lang/Nim/blob/6126a0bf46f4e29a368b8baefea69a2bcae54e93/compiler/semexprs.nim#L3171) to also consider the overloads if the symbol kind is overloadable. But it treats the normally overloadable template/macro/module sym kinds the same as non-overloadable symbols, just calling `semSym` on it. We need to mirror this behavior in `semOpenSym`; we treat the captured symchoice as a fresh identifier, so if the symbol we find is a template/macro/module, we use that symbol immediately as opposed to waiting for overloads.

refs #23873 (comment), fixes #23386, fixes #23385, supersedes #23572 Turns the `nfOpenSym` node flag implemented in #23091 and extended in #23102 and #23873, into a node kind `nkOpenSym` that forms a unary node containing either `nkSym` or `nkOpenSymChoice`. Since this affects macros working on generic proc AST, the node kind is now only generated when the experimental switch `genericsOpenSym` is enabled, and a new node flag `nfDisabledOpenSym` is set to the `nkSym` or `nkOpenSymChoice` when the switch is not enabled so that we can give a warning. Now that the experimental switch has more reasonable semantics, we define `nimHasGenericsOpenSym2`.

@Araq

This PR adds a workaround for the case where a global symbol is matched instead of the local `error`/`value` template in `valueOr` and friends, when `valueOr` is being used in a generic context. Two options are added: * when using Nim version supporting the experimental `genericsOpenSym` [feature](nim-lang/Nim#23873), we use it * when not, a macro tries to replace all accesses in the given body with the correct symbol - this matching is done by name and some minimal heuristics which potentially could be wrong - the upstream solution is obviously preferable Both solutions can be disabled via compile-time options to retain the old behavior of matching the global symbol. See also nim-lang/Nim#22605 thanks to @Araq for the macro hack and @metagn for the language fix!

fixes #23865 The node flag `nfOpenSym` implemented in #23091 for sym nodes is now also implemented for open symchoices. This means the intended behavior is still achieved when multiple overloads are in scope to be captured, so the issue is fixed. The code for the flag is documented and moved into a helper proc and the experimental switch is now enabled for the compiler test suite. (cherry picked from commit 469a604)

actually fixes #23865 following up #23873 In the handling of `nkIdent` in `semExpr`, the compiler looks for the closest symbol with the name and [checks the symbol kind](https://github.com/nim-lang/Nim/blob/6126a0bf46f4e29a368b8baefea69a2bcae54e93/compiler/semexprs.nim#L3171) to also consider the overloads if the symbol kind is overloadable. But it treats the normally overloadable template/macro/module sym kinds the same as non-overloadable symbols, just calling `semSym` on it. We need to mirror this behavior in `semOpenSym`; we treat the captured symchoice as a fresh identifier, so if the symbol we find is a template/macro/module, we use that symbol immediately as opposed to waiting for overloads. (cherry picked from commit a64aa51)

refs #23873 (comment), fixes #23386, fixes #23385, supersedes #23572 Turns the `nfOpenSym` node flag implemented in #23091 and extended in containing either `nkSym` or `nkOpenSymChoice`. Since this affects macros working on generic proc AST, the node kind is now only generated when the experimental switch `genericsOpenSym` is enabled, and a new node flag `nfDisabledOpenSym` is set to the `nkSym` or `nkOpenSymChoice` when the switch is not enabled so that we can give a warning. Now that the experimental switch has more reasonable semantics, we define `nimHasGenericsOpenSym2`. (cherry picked from commit 0c890ff)

fixes #23865 The node flag `nfOpenSym` implemented in #23091 for sym nodes is now also implemented for open symchoices. This means the intended behavior is still achieved when multiple overloads are in scope to be captured, so the issue is fixed. The code for the flag is documented and moved into a helper proc and the experimental switch is now enabled for the compiler test suite. (cherry picked from commit 469a604)

actually fixes #23865 following up #23873 In the handling of `nkIdent` in `semExpr`, the compiler looks for the closest symbol with the name and [checks the symbol kind](https://github.com/nim-lang/Nim/blob/6126a0bf46f4e29a368b8baefea69a2bcae54e93/compiler/semexprs.nim#L3171) to also consider the overloads if the symbol kind is overloadable. But it treats the normally overloadable template/macro/module sym kinds the same as non-overloadable symbols, just calling `semSym` on it. We need to mirror this behavior in `semOpenSym`; we treat the captured symchoice as a fresh identifier, so if the symbol we find is a template/macro/module, we use that symbol immediately as opposed to waiting for overloads. (cherry picked from commit a64aa51)

refs #23873 (comment), fixes #23386, fixes #23385, supersedes #23572 Turns the `nfOpenSym` node flag implemented in #23091 and extended in containing either `nkSym` or `nkOpenSymChoice`. Since this affects macros working on generic proc AST, the node kind is now only generated when the experimental switch `genericsOpenSym` is enabled, and a new node flag `nfDisabledOpenSym` is set to the `nkSym` or `nkOpenSymChoice` when the switch is not enabled so that we can give a warning. Now that the experimental switch has more reasonable semantics, we define `nimHasGenericsOpenSym2`. (cherry picked from commit 0c890ff)

fixes #23865 The node flag `nfOpenSym` implemented in #23091 for sym nodes is now also implemented for open symchoices. This means the intended behavior is still achieved when multiple overloads are in scope to be captured, so the issue is fixed. The code for the flag is documented and moved into a helper proc and the experimental switch is now enabled for the compiler test suite. (cherry picked from commit 469a604)

actually fixes #23865 following up #23873 In the handling of `nkIdent` in `semExpr`, the compiler looks for the closest symbol with the name and [checks the symbol kind](https://github.com/nim-lang/Nim/blob/6126a0bf46f4e29a368b8baefea69a2bcae54e93/compiler/semexprs.nim#L3171) to also consider the overloads if the symbol kind is overloadable. But it treats the normally overloadable template/macro/module sym kinds the same as non-overloadable symbols, just calling `semSym` on it. We need to mirror this behavior in `semOpenSym`; we treat the captured symchoice as a fresh identifier, so if the symbol we find is a template/macro/module, we use that symbol immediately as opposed to waiting for overloads. (cherry picked from commit a64aa51)

refs #23873 (comment), fixes #23386, fixes #23385, supersedes #23572 Turns the `nfOpenSym` node flag implemented in #23091 and extended in containing either `nkSym` or `nkOpenSymChoice`. Since this affects macros working on generic proc AST, the node kind is now only generated when the experimental switch `genericsOpenSym` is enabled, and a new node flag `nfDisabledOpenSym` is set to the `nkSym` or `nkOpenSymChoice` when the switch is not enabled so that we can give a warning. Now that the experimental switch has more reasonable semantics, we define `nimHasGenericsOpenSym2`. (cherry picked from commit 0c890ff)

metagn added 5 commits July 21, 2024 16:27

implement genericsOpenSym for symchoices

40905f2

fix warning test

08d2d0f

temporarily enable globally

e2aa20c

fix temp config

2b2522f

fix again

1c1d1cf

arnetheduck reviewed Jul 22, 2024

View reviewed changes

fix CI + cleanup

65c0ee4

metagn commented Jul 22, 2024

View reviewed changes

CI passed, removing temp global switch

a174996

metagn marked this pull request as ready for review July 22, 2024 20:29

Araq merged commit 469a604 into nim-lang:devel Jul 25, 2024
18 checks passed

metagn mentioned this pull request Jul 25, 2024

opensym as node kind + fixed experimental switch #23892

Merged

arnetheduck reviewed Aug 6, 2024

View reviewed changes

tests/generics/tmacroinjectedsym.nim

let x = f().valueOr:

return $error

"ok"

doAssert g(int) == "error"

Copy link

Contributor

arnetheduck Aug 6, 2024

Choose a reason for hiding this comment

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

this should be f

metagn mentioned this pull request Aug 6, 2024

genericsOpenSym not working when additional generally non-matching symbol with same name is present #23865

Closed

metagn added a commit to metagn/Nim that referenced this pull request Aug 11, 2024

don't treat template/macro/module as overloaded for opensym

ddff0e8

actually fixes nim-lang#23865 following up nim-lang#23873

metagn mentioned this pull request Aug 11, 2024

don't treat template/macro/module as overloaded for opensym #23939

Merged

arnetheduck mentioned this pull request Aug 13, 2024

Workaround for #34 arnetheduck/nim-results#35

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement genericsOpenSym for symchoices #23873

implement genericsOpenSym for symchoices #23873

metagn commented Jul 21, 2024 •

edited

Loading

arnetheduck Jul 22, 2024

Araq Jul 22, 2024

metagn Jul 22, 2024

Araq Jul 23, 2024

metagn Jul 25, 2024 •

edited

Loading

github-actions bot commented Jul 25, 2024

arnetheduck Aug 6, 2024

arnetheduck commented Aug 6, 2024 •

edited

Loading

implement genericsOpenSym for symchoices #23873

implement genericsOpenSym for symchoices #23873

Conversation

metagn commented Jul 21, 2024 • edited Loading

arnetheduck Jul 22, 2024

Choose a reason for hiding this comment

Araq Jul 22, 2024

Choose a reason for hiding this comment

metagn Jul 22, 2024

Choose a reason for hiding this comment

Araq Jul 23, 2024

Choose a reason for hiding this comment

metagn Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jul 25, 2024

arnetheduck Aug 6, 2024

Choose a reason for hiding this comment

arnetheduck commented Aug 6, 2024 • edited Loading

metagn commented Jul 21, 2024 •

edited

Loading

metagn Jul 25, 2024 •

edited

Loading

arnetheduck commented Aug 6, 2024 •

edited

Loading