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 generic param substitution in templates #22535

Merged
merged 2 commits into from
Aug 25, 2023

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Aug 23, 2023

fixes #13527, fixes #17240, fixes #6340, fixes #20033, fixes #19576, fixes #19076

When a generic template is called, for the generic parameter in the template body to be substituted with the matching generic parameter, before evaluation the matched generic types are added to the call node as extra arguments. These nodes are nkSym nodes of the original generic parameter symbol, with the type of the symbol and thus the node being the matched type. This is irregular in normal code, and when the template tries to use it, it gets treated as some value with the type instead of the type itself.

To prevent this, remove the type field from these nodes which will make regular code semcheck them and provide them with the appropriate type (similar to #20315). Different places in the compiler treat these symbols differently so there is no uniform computation we can do ahead of time.

@metagn
Copy link
Collaborator Author

metagn commented Aug 23, 2023

PR to fix nimPNG which relied on the bug: jangko/nimPNG#83 (merged)

PR to fix nim-lang NESM (seemingly this is the one CI uses): nim-lang/NESM#1

And original NESM: xomachine/NESM#22

preemptive NESM patch description

NESM is really bothersome to patch, the problem is here:

https://github.com/nim-lang/NESM/blob/0861da16e59974eb5c04ee8777adbcf3f8464d75/nesm.nim#L98-L99

The S argument provided to the macro used to have an S type, now it has a type S type, which the macro has to recognize and skip. This also needs to work with and without the bugfix. Not to mention what I linked is the nim-lang org fork and not the original nesm repo, when a PR would ideally go to both as the entire library seems to rely on that template. Just an annoying situation.

@metagn
Copy link
Collaborator Author

metagn commented Aug 24, 2023

It seems like original NESM has been inactive for a long time now, would be nice if someone could merge the patch into the nim-lang repo (nim-lang/NESM#1) for the time being.

@metagn
Copy link
Collaborator Author

metagn commented Aug 24, 2023

Sorry, the PR to NESM was not enough, made a new one for the nim-lang repo, locally tested this time: nim-lang/NESM#2 and updated the one for the original repo

@metagn metagn force-pushed the template-generic-param branch from 2240aa4 to 303063d Compare August 25, 2023 17:20
@metagn
Copy link
Collaborator Author

metagn commented Aug 25, 2023

CI finally passes

@Araq Araq merged commit 1cc4d3f into nim-lang:devel Aug 25, 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 1cc4d3f

Hint: mm: orc; opt: speed; options: -d:release
169329 lines; 12.760s; 610.566MiB peakmem

narimiran pushed a commit that referenced this pull request Dec 1, 2023
* fix generic param substitution in templates

fixes #13527, fixes #17240, fixes #6340, fixes #20033, fixes #19576, fixes #19076

* fix bare except in test, test updated packages in CI

(cherry picked from commit 1cc4d3f)
narimiran pushed a commit that referenced this pull request Dec 1, 2023
* fix generic param substitution in templates

fixes #13527, fixes #17240, fixes #6340, fixes #20033, fixes #19576, fixes #19076

* fix bare except in test, test updated packages in CI

(cherry picked from commit 1cc4d3f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment