opensym for templates + move behavior of opensymchoice to itself #24007
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Should we have separate For now adding |
doesn't feel immensely useful, what would be the use case for it? ie this basically brings the two behavior in line with each other, no? |
|
Yeah this makes it more universal but I figured maybe one of them can cause more breaks and people might want to pick which ones to enable. I also didn't want to remove |
4089b56
to
2945dd2
Compare
|
lgtm, good to backport as well with any potential dependencies cc @narimiran |
|
Thanks for your hard work on this PR! Hint: mm: orc; opt: speed; options: -d:release |
narimiran
added a commit
that referenced
this pull request
Aug 29, 2024
) fixes #15314, fixes #24002 The OpenSym behavior first added to generics in #23091 now also applies to templates, since templates can also capture symbols that are meant to be replaced by local symbols if the context imports symbols with the same name, as in the issue #24002. The experimental switch `templateOpenSym` is added to enable this behavior for templates only, and the experimental switch `openSym` is added to enable it for both templates and generics, and the documentation now mainly mentions this switch. Additionally the logic for `nkOpenSymChoice` nodes that were previously wrapped in `nkOpenSym` now apply to all `nkOpenSymChoice` nodes, and so these nodes aren't wrapped in `nkOpenSym` anymore. This means `nkOpenSym` can only have children of kind `nkSym` again, so it is more in line with the structure of symchoice nodes. As for why they aren't merged with `nkOpenSymChoice` nodes yet, we need some way to signal that the node shouldn't become ambiguous if other options exist at instantiation time, we already captured a symbol at the beginning and another symbol can only replace it if it's closer in scope and unambiguous. (cherry picked from commit 770f8d5)
narimiran
added a commit
that referenced
this pull request
Aug 29, 2024
) fixes #15314, fixes #24002 The OpenSym behavior first added to generics in #23091 now also applies to templates, since templates can also capture symbols that are meant to be replaced by local symbols if the context imports symbols with the same name, as in the issue #24002. The experimental switch `templateOpenSym` is added to enable this behavior for templates only, and the experimental switch `openSym` is added to enable it for both templates and generics, and the documentation now mainly mentions this switch. Additionally the logic for `nkOpenSymChoice` nodes that were previously wrapped in `nkOpenSym` now apply to all `nkOpenSymChoice` nodes, and so these nodes aren't wrapped in `nkOpenSym` anymore. This means `nkOpenSym` can only have children of kind `nkSym` again, so it is more in line with the structure of symchoice nodes. As for why they aren't merged with `nkOpenSymChoice` nodes yet, we need some way to signal that the node shouldn't become ambiguous if other options exist at instantiation time, we already captured a symbol at the beginning and another symbol can only replace it if it's closer in scope and unambiguous. (cherry picked from commit 770f8d5)
narimiran
added a commit
that referenced
this pull request
Aug 29, 2024
) fixes #15314, fixes #24002 The OpenSym behavior first added to generics in #23091 now also applies to templates, since templates can also capture symbols that are meant to be replaced by local symbols if the context imports symbols with the same name, as in the issue #24002. The experimental switch `templateOpenSym` is added to enable this behavior for templates only, and the experimental switch `openSym` is added to enable it for both templates and generics, and the documentation now mainly mentions this switch. Additionally the logic for `nkOpenSymChoice` nodes that were previously wrapped in `nkOpenSym` now apply to all `nkOpenSymChoice` nodes, and so these nodes aren't wrapped in `nkOpenSym` anymore. This means `nkOpenSym` can only have children of kind `nkSym` again, so it is more in line with the structure of symchoice nodes. As for why they aren't merged with `nkOpenSymChoice` nodes yet, we need some way to signal that the node shouldn't become ambiguous if other options exist at instantiation time, we already captured a symbol at the beginning and another symbol can only replace it if it's closer in scope and unambiguous. (cherry picked from commit 770f8d5)
narimiran
added a commit
that referenced
this pull request
Aug 29, 2024
) fixes #15314, fixes #24002 The OpenSym behavior first added to generics in #23091 now also applies to templates, since templates can also capture symbols that are meant to be replaced by local symbols if the context imports symbols with the same name, as in the issue #24002. The experimental switch `templateOpenSym` is added to enable this behavior for templates only, and the experimental switch `openSym` is added to enable it for both templates and generics, and the documentation now mainly mentions this switch. Additionally the logic for `nkOpenSymChoice` nodes that were previously wrapped in `nkOpenSym` now apply to all `nkOpenSymChoice` nodes, and so these nodes aren't wrapped in `nkOpenSym` anymore. This means `nkOpenSym` can only have children of kind `nkSym` again, so it is more in line with the structure of symchoice nodes. As for why they aren't merged with `nkOpenSymChoice` nodes yet, we need some way to signal that the node shouldn't become ambiguous if other options exist at instantiation time, we already captured a symbol at the beginning and another symbol can only replace it if it's closer in scope and unambiguous. (cherry picked from commit 770f8d5)
narimiran
added a commit
that referenced
this pull request
Aug 29, 2024
) fixes #15314, fixes #24002 The OpenSym behavior first added to generics in #23091 now also applies to templates, since templates can also capture symbols that are meant to be replaced by local symbols if the context imports symbols with the same name, as in the issue #24002. The experimental switch `templateOpenSym` is added to enable this behavior for templates only, and the experimental switch `openSym` is added to enable it for both templates and generics, and the documentation now mainly mentions this switch. Additionally the logic for `nkOpenSymChoice` nodes that were previously wrapped in `nkOpenSym` now apply to all `nkOpenSymChoice` nodes, and so these nodes aren't wrapped in `nkOpenSym` anymore. This means `nkOpenSym` can only have children of kind `nkSym` again, so it is more in line with the structure of symchoice nodes. As for why they aren't merged with `nkOpenSymChoice` nodes yet, we need some way to signal that the node shouldn't become ambiguous if other options exist at instantiation time, we already captured a symbol at the beginning and another symbol can only replace it if it's closer in scope and unambiguous. (cherry picked from commit 770f8d5)
narimiran
added a commit
that referenced
this pull request
Aug 29, 2024
) fixes #15314, fixes #24002 The OpenSym behavior first added to generics in #23091 now also applies to templates, since templates can also capture symbols that are meant to be replaced by local symbols if the context imports symbols with the same name, as in the issue #24002. The experimental switch `templateOpenSym` is added to enable this behavior for templates only, and the experimental switch `openSym` is added to enable it for both templates and generics, and the documentation now mainly mentions this switch. Additionally the logic for `nkOpenSymChoice` nodes that were previously wrapped in `nkOpenSym` now apply to all `nkOpenSymChoice` nodes, and so these nodes aren't wrapped in `nkOpenSym` anymore. This means `nkOpenSym` can only have children of kind `nkSym` again, so it is more in line with the structure of symchoice nodes. As for why they aren't merged with `nkOpenSymChoice` nodes yet, we need some way to signal that the node shouldn't become ambiguous if other options exist at instantiation time, we already captured a symbol at the beginning and another symbol can only replace it if it's closer in scope and unambiguous. (cherry picked from commit 770f8d5)
narimiran
pushed a commit
that referenced
this pull request
Aug 29, 2024
) fixes #15314, fixes #24002 The OpenSym behavior first added to generics in #23091 now also applies to templates, since templates can also capture symbols that are meant to be replaced by local symbols if the context imports symbols with the same name, as in the issue #24002. The experimental switch `templateOpenSym` is added to enable this behavior for templates only, and the experimental switch `openSym` is added to enable it for both templates and generics, and the documentation now mainly mentions this switch. Additionally the logic for `nkOpenSymChoice` nodes that were previously wrapped in `nkOpenSym` now apply to all `nkOpenSymChoice` nodes, and so these nodes aren't wrapped in `nkOpenSym` anymore. This means `nkOpenSym` can only have children of kind `nkSym` again, so it is more in line with the structure of symchoice nodes. As for why they aren't merged with `nkOpenSymChoice` nodes yet, we need some way to signal that the node shouldn't become ambiguous if other options exist at instantiation time, we already captured a symbol at the beginning and another symbol can only replace it if it's closer in scope and unambiguous. (cherry picked from commit 770f8d5)
narimiran
pushed a commit
that referenced
this pull request
Sep 16, 2024
) fixes #15314, fixes #24002 The OpenSym behavior first added to generics in #23091 now also applies to templates, since templates can also capture symbols that are meant to be replaced by local symbols if the context imports symbols with the same name, as in the issue #24002. The experimental switch `templateOpenSym` is added to enable this behavior for templates only, and the experimental switch `openSym` is added to enable it for both templates and generics, and the documentation now mainly mentions this switch. Additionally the logic for `nkOpenSymChoice` nodes that were previously wrapped in `nkOpenSym` now apply to all `nkOpenSymChoice` nodes, and so these nodes aren't wrapped in `nkOpenSym` anymore. This means `nkOpenSym` can only have children of kind `nkSym` again, so it is more in line with the structure of symchoice nodes. As for why they aren't merged with `nkOpenSymChoice` nodes yet, we need some way to signal that the node shouldn't become ambiguous if other options exist at instantiation time, we already captured a symbol at the beginning and another symbol can only replace it if it's closer in scope and unambiguous. (cherry picked from commit 770f8d5)
narimiran
pushed a commit
that referenced
this pull request
Sep 16, 2024
) fixes #15314, fixes #24002 The OpenSym behavior first added to generics in #23091 now also applies to templates, since templates can also capture symbols that are meant to be replaced by local symbols if the context imports symbols with the same name, as in the issue #24002. The experimental switch `templateOpenSym` is added to enable this behavior for templates only, and the experimental switch `openSym` is added to enable it for both templates and generics, and the documentation now mainly mentions this switch. Additionally the logic for `nkOpenSymChoice` nodes that were previously wrapped in `nkOpenSym` now apply to all `nkOpenSymChoice` nodes, and so these nodes aren't wrapped in `nkOpenSym` anymore. This means `nkOpenSym` can only have children of kind `nkSym` again, so it is more in line with the structure of symchoice nodes. As for why they aren't merged with `nkOpenSymChoice` nodes yet, we need some way to signal that the node shouldn't become ambiguous if other options exist at instantiation time, we already captured a symbol at the beginning and another symbol can only replace it if it's closer in scope and unambiguous. (cherry picked from commit 770f8d5)
Araq
pushed a commit
that referenced
this pull request
Sep 17, 2024
fixes #24112 Sym nodes in templates that could be open are [given `nil` type](https://github.com/nim-lang/Nim/blob/22d2cf217597468ace8ba540d6990b1f6d8a816a/compiler/semtempl.nim#L274) when `--experimentalOpenSym` is disabled so that they can be semchecked to give a warning since #24007. The first nodes of object constructors (in this case) and in type conversions don't replace their first node (the symbol) with a typechecked one, they only call `semTypeNode` on it and leave it as is. Effect tracking checks if the type of a sym node has a destructor to check if the node type should be replaced with the sym type. But this causes a segfault when the type of the node is nil. To fix this, we always set the node type to the sym type if the node type is nil. Alternatively `semObjConstr` and `semConv` could be changed to set the type of their first node to the found type but I'm not sure if this would break anything. They could call `semExprWithType` on the first node but `semTypeNode` would still have to be called (maybe call it before?). This isn't a problem if the sym node has a type but is just nested in `nkOpenSym` or `nkOpenSymChoice` which have nil type instead (i.e. with openSym enabled), so maybe this still is the "most general" solution, I don't know.
narimiran
pushed a commit
that referenced
this pull request
Sep 18, 2024
fixes #24112 Sym nodes in templates that could be open are [given `nil` type](https://github.com/nim-lang/Nim/blob/22d2cf217597468ace8ba540d6990b1f6d8a816a/compiler/semtempl.nim#L274) when `--experimentalOpenSym` is disabled so that they can be semchecked to give a warning since #24007. The first nodes of object constructors (in this case) and in type conversions don't replace their first node (the symbol) with a typechecked one, they only call `semTypeNode` on it and leave it as is. Effect tracking checks if the type of a sym node has a destructor to check if the node type should be replaced with the sym type. But this causes a segfault when the type of the node is nil. To fix this, we always set the node type to the sym type if the node type is nil. Alternatively `semObjConstr` and `semConv` could be changed to set the type of their first node to the found type but I'm not sure if this would break anything. They could call `semExprWithType` on the first node but `semTypeNode` would still have to be called (maybe call it before?). This isn't a problem if the sym node has a type but is just nested in `nkOpenSym` or `nkOpenSymChoice` which have nil type instead (i.e. with openSym enabled), so maybe this still is the "most general" solution, I don't know. (cherry picked from commit 680a13a)
metagn
added a commit
to metagn/Nim
that referenced
this pull request
Sep 21, 2024
…24114) fixes nim-lang#24112 Sym nodes in templates that could be open are [given `nil` type](https://github.com/nim-lang/Nim/blob/22d2cf217597468ace8ba540d6990b1f6d8a816a/compiler/semtempl.nim#L274) when `--experimentalOpenSym` is disabled so that they can be semchecked to give a warning since nim-lang#24007. The first nodes of object constructors (in this case) and in type conversions don't replace their first node (the symbol) with a typechecked one, they only call `semTypeNode` on it and leave it as is. Effect tracking checks if the type of a sym node has a destructor to check if the node type should be replaced with the sym type. But this causes a segfault when the type of the node is nil. To fix this, we always set the node type to the sym type if the node type is nil. Alternatively `semObjConstr` and `semConv` could be changed to set the type of their first node to the found type but I'm not sure if this would break anything. They could call `semExprWithType` on the first node but `semTypeNode` would still have to be called (maybe call it before?). This isn't a problem if the sym node has a type but is just nested in `nkOpenSym` or `nkOpenSymChoice` which have nil type instead (i.e. with openSym enabled), so maybe this still is the "most general" solution, I don't know.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #15314, fixes #24002
The OpenSym behavior first added to generics in #23091 now also applies to templates, since templates can also capture symbols that are meant to be replaced by local symbols if the context imports symbols with the same name, as in the issue #24002. The experimental switch
templateOpenSymis added to enable this behavior for templates only, and the experimental switchopenSymis added to enable it for both templates and generics, and the documentation now mainly mentions this switch.Additionally the logic for
nkOpenSymChoicenodes that were previously wrapped innkOpenSymnow apply to allnkOpenSymChoicenodes, and so these nodes aren't wrapped innkOpenSymanymore. This meansnkOpenSymcan only have children of kindnkSymagain, so it is more in line with the structure of symchoice nodes. As for why they aren't merged withnkOpenSymChoicenodes yet, we need some way to signal that the node shouldn't become ambiguous if other options exist at instantiation time, we already captured a symbol at the beginning and another symbol can only replace it if it's closer in scope and unambiguous.