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

opensym as node kind + fixed experimental switch #23892

Merged
merged 11 commits into from
Aug 12, 2024
Merged

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Jul 25, 2024

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.

@metagn
Copy link
Collaborator Author

metagn commented Jul 26, 2024

Seems to work, but until packages can handle it (macros just need simple skips AFAIK) or until regressions not detected in CI are fixed the node probably shouldn't be generated without the experimental switch enabled. So for the warning we probably need something like #23572, swapping out the existing node flag with one that only exists for a warning and is temporary. I'll do it in this PR if there are no objections, the other PR is out of date already.

@Araq
Copy link
Member

Araq commented Jul 27, 2024

swapping out the existing node flag with one that only exists for a warning and is temporary. I'll do it in this PR if there are no objections, the other PR is out of date already.

Fine with me.

@metagn metagn changed the title opensym as node kind opensym as node kind + fixed experimental switch Jul 28, 2024
@metagn metagn marked this pull request as ready for review July 28, 2024 02:29
@Araq
Copy link
Member

Araq commented Aug 11, 2024

Please rebase.

@metagn
Copy link
Collaborator Author

metagn commented Aug 11, 2024

Done

@@ -166,4 +166,5 @@ proc initDefines*(symbols: StringTableRef) =
defineSymbol("nimHasWarnStdPrefix")

defineSymbol("nimHasVtables")
defineSymbol("nimHasGenericsOpenSym")
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be defined only when the experimental is on?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The nimHas defines usually signal that the experimental switch is available, checking if it's enabled is trickier since it can change depending on the context with push experimental etc. #23933 was opened recently related to this

Copy link
Contributor

Choose a reason for hiding this comment

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

turns out nimHasGenericsOpenSym is already declared, but for the broken version shipped in 2.0.8:

will need a new name for this new, working version 🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

nimHasGenericsOpenSym2?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to nimHasGenericsOpenSym2

@arnetheduck
Copy link
Contributor

not sure it's intended, but compiling this branch (3fe15eb) results in lots of warnings:

....
/home/arnetheduck/src/Nim/compiler/ic/cbackend.nim(106, 31) template/generic instantiation of `==` from here
/home/arnetheduck/src/Nim/lib/system/comparisons.nim(324, 16) Warning: a new symbol 'true' has been injected during instantiation of ==, however 'true' [enumField declared in /home/arnetheduck/src/Nim/lib/system/basic_types.nim(46, 16)] captured at the proc declaration will be used instead; either enable --experimental:genericsOpenSym to use the injected symbol or `bind` this captured symbol explicitly [GenericsIgnoredInjection]
.
/home/arnetheduck/src/Nim/compiler/ic/integrity.nim(46, 24) template/generic instantiation of `containsOrIncl` from here
/home/arnetheduck/src/Nim/lib/pure/collections/setimpl.nim(59, 17) Warning: a new symbol 'defaultInitialSize' has been injected during instantiation of containsOrIncl, however 'defaultInitialSize' [const declared in /home/arnetheduck/src/Nim/lib/pure/collections/sets.nim(89, 3)] captured at the proc declaration will be used instead; either enable --experimental:genericsOpenSym to use the injected symbol or `bind` this captured symbol explicitly [GenericsIgnoredInjection]
/home/arnetheduck/src/Nim/compiler/ic/integrity.nim(46, 24) template/generic instantiation of `containsOrIncl` from here
/home/arnetheduck/src/Nim/lib/pure/collections/setimpl.nim(67, 14) template/generic instantiation of `enlarge` from here
/home/arnetheduck/src/Nim/lib/pure/collections/setimpl.nim(43, 16) template/generic instantiation of `rawInsert` from here
/home/arnetheduck/src/Nim/lib/pure/collections/setimpl.nim(28, 17) Warning: a new symbol 'defaultInitialSize' has been injected during instantiation of rawInsert, however 'defaultInitialSize' [const declared in /home/arnetheduck/src/Nim/lib/pure/collections/sets.nim(89, 3)] captured at the proc declaration will be used instead; either enable --experimental:genericsOpenSym to use the injected symbol or `bind` this captured symbol explicitly [GenericsIgnoredInjection]
.
/home/arnetheduck/src/Nim/compiler/ic/navigator.nim(96, 26) template/generic instantiation of `containsOrIncl` from here
/home/arnetheduck/src/Nim/lib/pure/collections/setimpl.nim(59, 17) Warning: a new symbol 'defaultInitialSize' has been injected during instantiation of containsOrIncl, however 'defaultInitialSize' [const declared in /home/arnetheduck/src/Nim/lib/pure/collections/sets.nim(89, 3)] captured at the proc declaration will be used instead; either enable --experimental:genericsOpenSym to use the injected symbol or `bind` this captured symbol explicitly [GenericsIgnoredInjection]

@arnetheduck
Copy link
Contributor

arnetheduck commented Aug 12, 2024

testing this, the semantics of push experimental are quite weird:

Neither of these two work at top-level - the when condition is basically to allow -d:resultsGenericsOpenSym to enable the experimental feature for results only.

when condition:
  {.experimental: "genericsOpenSym".}
  {.push experimental: "genericsOpenSym".}

However, in a situation analogous to the following, the push covers both a and b, even though the push is only inside a:

template a =
  when condition:
    {.push experimental: "genericsOpenSym".}

template b = 
  template injector = discard # this works now

It's kind of weird that inside a template scope, pushing extends to other unrelated templates but not outside..

@metagn
Copy link
Collaborator Author

metagn commented Aug 12, 2024

not sure it's intended, but compiling this branch (3fe15eb) results in lots of warnings:

Definitely not, my bad, I moved the warnings to the wrong spot in the code

@arnetheduck
Copy link
Contributor

arnetheduck commented Aug 12, 2024

arnetheduck/nim-results#35 updated to work with this branch - also includes an ugly emulation hack for Nim versions that don't have this feature..

@Araq Araq merged commit 0c890ff into nim-lang:devel Aug 12, 2024
18 checks passed
Copy link
Contributor

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

Hint: mm: orc; opt: speed; options: -d:release
173437 lines; 8.000s; 654.785MiB peakmem

narimiran pushed a commit that referenced this pull request Aug 14, 2024
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)
narimiran pushed a commit that referenced this pull request Aug 14, 2024
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)
narimiran pushed a commit that referenced this pull request Aug 14, 2024
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)
@narimiran narimiran mentioned this pull request Aug 15, 2024
Araq pushed a commit that referenced this pull request Sep 18, 2024
…penSym` (#24111)

alternative to #24101

#23892 changed the opensym experimental switch so that it has to be
enabled in the context of the generic/template declarations capturing
the symbols, not the context of the instantiation of the
generics/templates. This was to be in line with where the compiler gives
the warnings and changes behavior in a potentially breaking way.

However `results` [depends on the old
behavior](https://github.com/arnetheduck/nim-results/blob/71d404b314479a6205bfd050f4fe5fe49cdafc69/results.nim#L1428),
so that the callers of the macros provided by results always take
advantage of the opensym behavior. To accomodate this, we change the
behavior of the old experimental option that `results` uses,
`genericsOpenSym`, so that ignores the information of whether or not
symbols are intentionally opened and always gives the opensym behavior
as long as it's enabled at instantiation time. This should keep
`results` working as is. However this differs from the normal opensym
switch in that it doesn't generate `nnkOpenSym`.

Before it was just a generics-only version of `openSym` along with
`templateOpenSym` which was only for templates. So `templateOpenSym` is
removed along with this change, but no one appears to have used it.
metagn added a commit to metagn/Nim that referenced this pull request Sep 21, 2024
…penSym` (nim-lang#24111)

alternative to nim-lang#24101

nim-lang#23892 changed the opensym experimental switch so that it has to be
enabled in the context of the generic/template declarations capturing
the symbols, not the context of the instantiation of the
generics/templates. This was to be in line with where the compiler gives
the warnings and changes behavior in a potentially breaking way.

However `results` [depends on the old
behavior](https://github.com/arnetheduck/nim-results/blob/71d404b314479a6205bfd050f4fe5fe49cdafc69/results.nim#L1428),
so that the callers of the macros provided by results always take
advantage of the opensym behavior. To accomodate this, we change the
behavior of the old experimental option that `results` uses,
`genericsOpenSym`, so that ignores the information of whether or not
symbols are intentionally opened and always gives the opensym behavior
as long as it's enabled at instantiation time. This should keep
`results` working as is. However this differs from the normal opensym
switch in that it doesn't generate `nnkOpenSym`.

Before it was just a generics-only version of `openSym` along with
`templateOpenSym` which was only for templates. So `templateOpenSym` is
removed along with this change, but no one appears to have used it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

genericsOpenSym not respecting injections across modules Add compile-time symbol for genericsOpenSym
3 participants