-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
refactor to make sigmatch use LayeredIdTable for bindings #24216
Merged
Araq
merged 11 commits into
nim-lang:devel
from
metagn:sigmatch-layeredtypemap-standalone
Oct 6, 2024
Merged
refactor to make sigmatch use LayeredIdTable for bindings #24216
Araq
merged 11 commits into
nim-lang:devel
from
metagn:sigmatch-layeredtypemap-standalone
Oct 6, 2024
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
Thanks for your hard work on this PR! Hint: mm: orc; opt: speed; options: -d:release |
metagn
added a commit
to metagn/Nim
that referenced
this pull request
Oct 7, 2024
fixes nim-lang#4858, fixes nim-lang#10027, fixes nim-lang#12552, fixes nim-lang#15721, fixes nim-lang#21331, refs nim-lang#1214, follows up nim-lang#24216, split from nim-lang#24198
narimiran
pushed a commit
that referenced
this pull request
Jan 14, 2025
split from #24198 This is a required refactor for the only good solution I've been able to think of for #4858 etc. Explanation: --- `sigmatch` currently [disables bindings](https://github.com/nim-lang/Nim/blob/d6a71a10671b66ee4f5be09f99234b3d834e7fce/compiler/sigmatch.nim#L1956) (except for binding to other generic parameters) when matching against constraints of generic parameters. This is so when the constraint is a general metatype like `seq`, the type matching will not treat all following uses of `seq` as the type matched against that generic parameter. However to solve #4858 etc we need to bind `or` types with a conversion match to the type they are supposed to be converted to (i.e. matching `int literal(123)` against `int8 | int16` should bind `int8`[^1], not `int`). The generic parameter constraint binding needs some way to keep track of this so that matching `int literal(123)` against `T: int8 | int16` also binds `T` to `int8`[^1]. The only good way to do this IMO is to generate a new "binding context" when matching against constraints, then binding the generic param to what the constraint was bound to in that context (in #24198 this is restricted to just `or` types & concrete types with convertible matches, it doesn't work in general). --- `semtypinst` already does something similar for bindings of generic invocations using `LayeredIdTable`, so `LayeredIdTable` is now split into its own module and used in `sigmatch` for type bindings as well, rather than a single-layer `TypeMapping`. Other modules which act on `sigmatch`'s binding map are also updated to use this type instead. The type is also made into an `object` type rather than a `ref object` to reduce the pointer indirection when embedding it inside `TCandidate`/`TReplTypeVars`, but only on arc/orc since there are some weird aliasing bugs on refc/markAndSweep that cause a segfault when setting a layer to its previous layer. If we want we can also just remove the conditional compilation altogether and always use `ref object` at the cost of some performance. [^1]: `int8` binding here and not `int16` might seem weird, since they match equally well. But we need to resolve the ambiguity here, in #24012 I tested disallowing ambiguities like this and it broke many packages that tries to match int literals to things like `int16 | uint16` or `int8 | int16`. Instead of making these packages stop working I think it's better we resolve the ambiguity with a rule like "the earliest `or` branch with the best match, matches". This is the rule used in #24198. (cherry picked from commit cad8726)
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.
split from #24198
This is a required refactor for the only good solution I've been able to think of for #4858 etc. Explanation:
sigmatch
currently disables bindings (except for binding to other generic parameters) when matching against constraints of generic parameters. This is so when the constraint is a general metatype likeseq
, the type matching will not treat all following uses ofseq
as the type matched against that generic parameter.However to solve #4858 etc we need to bind
or
types with a conversion match to the type they are supposed to be converted to (i.e. matchingint literal(123)
againstint8 | int16
should bindint8
1, notint
). The generic parameter constraint binding needs some way to keep track of this so that matchingint literal(123)
againstT: int8 | int16
also bindsT
toint8
1.The only good way to do this IMO is to generate a new "binding context" when matching against constraints, then binding the generic param to what the constraint was bound to in that context (in #24198 this is restricted to just
or
types & concrete types with convertible matches, it doesn't work in general).semtypinst
already does something similar for bindings of generic invocations usingLayeredIdTable
, soLayeredIdTable
is now split into its own module and used insigmatch
for type bindings as well, rather than a single-layerTypeMapping
. Other modules which act onsigmatch
's binding map are also updated to use this type instead.The type is also made into an
object
type rather than aref object
to reduce the pointer indirection when embedding it insideTCandidate
/TReplTypeVars
, but only on arc/orc since there are some weird aliasing bugs on refc/markAndSweep that cause a segfault when setting a layer to its previous layer. If we want we can also just remove the conditional compilation altogether and always useref object
at the cost of some performance.Footnotes
int8
binding here and notint16
might seem weird, since they match equally well. But we need to resolve the ambiguity here, in rejector
types matching multiple types for int literals #24012 I tested disallowing ambiguities like this and it broke many packages that tries to match int literals to things likeint16 | uint16
orint8 | int16
. Instead of making these packages stop working I think it's better we resolve the ambiguity with a rule like "the earliestor
branch with the best match, matches". This is the rule used in test refactor that uses LayeredIdTable in sigmatch #24198. ↩ ↩2