Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

open + preferred syms #22744

Closed
wants to merge 7 commits into from
Closed

open + preferred syms #22744

wants to merge 7 commits into from

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Sep 23, 2023

fixes #11184, fixes #22605, fixes #20000

Non-routine symbol nodes are now marked as nfOpenSym in symbol mixin contexts, and get replaced when another symbol would have been preferred in the local sem context.

Might break code that depended on old behavior, the semOperand problems as a consequence of result.typ = nil might not be fully fixed, the dotTransformation change might break code but I doubt it.

Originally proposed as nkOpenSym but macros will break on a new ident node kind and it's not really a big deal if it's not respected.

Edit: Also had to implement preferred syms to make routines work like they used to.

Symchoices with the first element marked as nfPreferredSym (could be changed to put this on the symchoice node instead) treat the first element as 999 scopes deep and the rest as -1.

Standalone symbol nodes can also be marked as nfPreferredSym and should act the same as a unary nkOpenSymChoice node, but 1. I had a lot of unexplainable errors with the compiler after just generating these (though it might work now after having improved the implementation) and 2. I'm not sure they serve the same purpose. Might try it again eventually.

todo notes:

  • templates and generics should account for nfOpenSym nodes, currently all nkSym is ignored
  • mixin for non-routine symbols should treat them as nfOpenSym
  • decide if templates should generate nfOpenSym

might be changed significantly

@metagn metagn changed the title open syms open + preferred syms Sep 23, 2023
@Varriount
Copy link
Contributor

Varriount commented Sep 30, 2023

Why are symbols for routines special?

@metagn
Copy link
Collaborator Author

metagn commented Sep 30, 2023

They ideally shouldn't be super special, the main difference is that routines and now enums have alternative options, whereas other symbols can only become 1 thing. This is reflected by the compiler never accounting for symchoices with non overloadable symbols up to this point.

The PR as described above is admittedly confused regarding this and for now I have changed it so the "open sym" thing only applies to new symbols generated inside generic procs, which makes it specialized for #22605 as it probably should be.

The break in the delaunay package, where a symbol outside of generic proc scope beat it out due to slightly more precise parameters, shows that this design also treats scope difference as a last ditch resort in overloading rather than, say, procs in declaration scope always being prioritized as long as they match. While this is another arbitrary difference from other symbols, it's essentially preexisting and code probably depends on mixed in symbols not receiving less priority.

That break specifically is caused by the implementation of #11184. This is actually independent from the rest of the PR (which itself is now multiple independent parts), but the consensus seemed to be that it should be implemented.

The PR situation and previous comments shouldn't be cause for alarm, we should have something cleaner and more reasonable later.

metagn added a commit to metagn/Nim that referenced this pull request Dec 18, 2023
Araq pushed a commit that referenced this pull request Dec 18, 2023
fixes #22605, separated from #22744

This marks symbol captures in macro calls in generic contexts as
`nfOpenSym`, which means if there is a new symbol in the local
instantiatied body during instantiation time, this symbol replaces the
captured symbol. We have to be careful not to consider symbols outside
of the instantiation body during instantiation, because this will leak
symbols from the instantiation context scope rather than the original
declaration scope. This is done by checking if the local context owner
(maybe should be the symbol of the proc currently getting instantiated
instead? not sure how to get this) is the same as or a parent owner of
the owner of the replacement candidate symbol.

This solution is distinct from the symchoice mechanisms which we
originally assumed had to be related, if this assumption was wrong it
would explain why this solution took so long to arrive at.
@metagn metagn closed this Dec 19, 2023
narimiran pushed a commit that referenced this pull request Mar 7, 2024
fixes #22605, separated from #22744

This marks symbol captures in macro calls in generic contexts as
`nfOpenSym`, which means if there is a new symbol in the local
instantiatied body during instantiation time, this symbol replaces the
captured symbol. We have to be careful not to consider symbols outside
of the instantiation body during instantiation, because this will leak
symbols from the instantiation context scope rather than the original
declaration scope. This is done by checking if the local context owner
(maybe should be the symbol of the proc currently getting instantiated
instead? not sure how to get this) is the same as or a parent owner of
the owner of the replacement candidate symbol.

This solution is distinct from the symchoice mechanisms which we
originally assumed had to be related, if this assumption was wrong it
would explain why this solution took so long to arrive at.

(cherry picked from commit 9416595)
narimiran pushed a commit that referenced this pull request Apr 26, 2024
fixes #22605, separated from #22744

This marks symbol captures in macro calls in generic contexts as
`nfOpenSym`, which means if there is a new symbol in the local
instantiatied body during instantiation time, this symbol replaces the
captured symbol. We have to be careful not to consider symbols outside
of the instantiation body during instantiation, because this will leak
symbols from the instantiation context scope rather than the original
declaration scope. This is done by checking if the local context owner
(maybe should be the symbol of the proc currently getting instantiated
instead? not sure how to get this) is the same as or a parent owner of
the owner of the replacement candidate symbol.

This solution is distinct from the symchoice mechanisms which we
originally assumed had to be related, if this assumption was wrong it
would explain why this solution took so long to arrive at.

(cherry picked from commit 9416595)
narimiran pushed a commit that referenced this pull request May 2, 2024
fixes #22605, separated from #22744

This marks symbol captures in macro calls in generic contexts as
`nfOpenSym`, which means if there is a new symbol in the local
instantiatied body during instantiation time, this symbol replaces the
captured symbol. We have to be careful not to consider symbols outside
of the instantiation body during instantiation, because this will leak
symbols from the instantiation context scope rather than the original
declaration scope. This is done by checking if the local context owner
(maybe should be the symbol of the proc currently getting instantiated
instead? not sure how to get this) is the same as or a parent owner of
the owner of the replacement candidate symbol.

This solution is distinct from the symchoice mechanisms which we
originally assumed had to be related, if this assumption was wrong it
would explain why this solution took so long to arrive at.

(cherry picked from commit 9416595)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants