refactor to make sigmatch use LayeredIdTable for bindings #24216
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