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

fix dot calls with resolved symbols in templates #22076

Merged
merged 6 commits into from
Jun 12, 2023

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Jun 11, 2023

fixes #20073, fixes #7085

dotTransformation transforms any resolved nkSyms as the field name back into nkIdent, but does not do this for sym choices. Generics account for this by converting to nkClosedSymChoice, but templates don't do anything similar. Now this is done in templates, but since code depended on it turning to nkIdent which was not resolved, we use nkOpenSymChoice instead.

doAssert deb1(-12'wrap) == "-12'wrap"
doAssert deb1(-12'nonexistent) == "-12'nonexistent"
doAssert deb2(-12'nonexistent) == """(DotExpr (RStrLit "-12") (Ident "\'nonexistent"))"""
when false: # xxx bug:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was not a bug. It's because this code is in a giant main template, which binds the outer 'wrap and wrap2, because that's how the language works.

@@ -165,21 +162,16 @@ template main =
doAssert fn2() == "[[-12]]"
doAssert fn3() == "[[-12]]"

when false: # xxx this fails; bug 9 from https://github.com/nim-lang/Nim/pull/17020#issuecomment-803193947
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Meanwhile this was actually the bug fixed by this PR, custom numeric literals generate dot fields which did not keep their resolved symbols in templates. It really wouldn't have taken much investigation to figure out this is related to dot fields and has nothing to do with custom numeric literals. The linked comment thinks it has something to do with 'abc not working where abc is a template param??? Very weird understanding of the language

@Araq Araq merged commit 71801c2 into nim-lang:devel Jun 12, 2023
@github-actions
Copy link
Contributor

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

Hint: mm: orc; opt: speed; options: -d:release
167824 lines; 10.123s; 610.965MiB peakmem

@narimiran
Copy link
Member

@arnetheduck I think I've seen this kind of error appearing in Status' codebase. Backport to 1.6?

@arnetheduck
Copy link
Contributor

sure, looks interesting as far as bugfixes go

narimiran pushed a commit that referenced this pull request Jun 13, 2023
* fix dot calls with resolved symbols in templates

* make old code work

* fix custom number literals test

* remove leftover debug marker

* enable "bug 9" test too

* fix renderer, add test for #7085

(cherry picked from commit 71801c2)
metagn added a commit to metagn/Nim that referenced this pull request Jun 16, 2023
Araq pushed a commit that referenced this pull request Jun 16, 2023
add test to close #7223, close #11733

closes #7223, closes #11733, were fixed by #22076
bung87 pushed a commit to bung87/Nim that referenced this pull request Jul 29, 2023
* fix dot calls with resolved symbols in templates

* make old code work

* fix custom number literals test

* remove leftover debug marker

* enable "bug 9" test too

* fix renderer, add test for nim-lang#7085
bung87 pushed a commit to bung87/Nim that referenced this pull request Jul 29, 2023
@metagn metagn mentioned this pull request Aug 22, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants