test refactor that uses LayeredIdTable in sigmatch #24198
Closed
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
metagn
force-pushed
the
sigmatch-layered-type-map
branch
from
80fa67e to
467e6f5
Compare
|
Seems like it will work with some touch ups, opening separate PR for LayeredIdTable first to not complicate review. Edit: #24216 |
Araq
pushed a commit
that referenced
this pull request
Oct 6, 2024
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.
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
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.
I think this is required for a good solution of #4858 but I'm not sure how it'll turn out, testing this by itself first.
There might be a performance hit because of the double pointer indirection. Edit: There is a little bit, I tried changing to object but
setToPreviousLayersegfaulted in nimsuggest which uses--gc:markAndSweep, I can't find a workaround.