Fix bounds of encoded type variables in quote patterns #17956
Closed
Conversation
This file contains hidden or 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
Merged
smarter
requested changes
Jun 21, 2023
| var newBindingsRefs: List[Type] = mapping.values.toList.map(_.typeRef) | ||
| for newBindings <- mapping.values do | ||
| newBindings.info = newBindings.info.subst(oldBindings, newBindingsRefs) | ||
| ctx.gadtState.addToConstraint(newBindings) // This must be preformed after the info has been updated |
There was a problem hiding this comment.
Suggested change
| ctx.gadtState.addToConstraint(newBindings) // This must be preformed after the info has been updated | |
| ctx.gadtState.addToConstraint(newBindings) // This must be performed after the info has been updated |
Comment on lines
356
to
357
| var oldBindings: List[Symbol] = mapping.keys.toList | ||
| var newBindingsRefs: List[Type] = mapping.values.toList.map(_.typeRef) |
There was a problem hiding this comment.
Look like those don't need to be vars?
Suggested change
| var oldBindings: List[Symbol] = mapping.keys.toList | |
| var newBindingsRefs: List[Type] = mapping.values.toList.map(_.typeRef) | |
| val oldBindings: List[Symbol] = mapping.keys.toList | |
| val newBindingsRefs: List[Type] = mapping.values.toList.map(_.typeRef) |
| * This mapping retains the original type variable definition order. | ||
| */ | ||
| private def unapplyBindingsMapping(quoted: Tree)(using Context): collection.Map[Symbol, Bind] = { | ||
| val mapping = mutable.LinkedHashMap.empty[Symbol, Symbol] |
There was a problem hiding this comment.
It looks like we never actually query this map, we only call mapping.keys and mapping.values, could we replace it by directly defining oldBindings and newBindingsRefs as ListBuffers?
There was a problem hiding this comment.
There is a mapping.contains(sym). I will simplify it to a LinkedHashSet and a ListBuffer.
| @@ -288,7 +282,7 @@ trait QuotesAndSplices { | |||
| report.error(IllegalVariableInPatternAlternative(tdef.symbol.name), tdef.srcPos) | |||
| if variance == -1 then | |||
| tdef.symbol.addAnnotation(Annotation(New(ref(defn.QuotedRuntimePatterns_fromAboveAnnot.typeRef)).withSpan(tdef.span))) | |||
| val bindingType = getBinding(tdef.symbol).symbol.typeRef | |||
| val bindingType = bindSymMapping(tdef.symbol).symbol.typeRef | |||
There was a problem hiding this comment.
If all we use bindSymMapping for is to get the symbol of the value, why do we need to have Bind nodes as values at all instead of symbols or typerefs?
There was a problem hiding this comment.
This function returns them. But I would create them later. I will refactor this to make it simpler.
nicolasstucki
force-pushed
the
fix-quote-type-variable-bounds
branch
2 times, most recently
from
e7d4857 to
a096e75
Compare
When we encode quote patterns into `unapply` methods, we need to create a copy of each type variable. One copy is kept within the quote(in the next stage) and the other is used in the `unapply` method to define the usual pattern type variables. When creating the latter we copied the symbols but did not update the infos. This implies that if type variables would be bounded by each other, the bounds of the copies would be the original types instead of the copies. We need to update those references. To update the info we now create all the symbols in one pass and the update all their infos in a second pass. This also implies that we cannot use the `newPatternBoundSymbol` to create the symbol as this constructor will register the info into GADT bounds. Instead we use the plain `newSymbol`. Then in the second pass, when we have updated the infos, we register the symbol into GADT bounds. Note that the code in the added test does compiles correctly, but it had the inconsistent bounds. This test is added in case we need to manually inspect the bounds latter. This test does fail to compile in scala#17935 if this fix is not applied.
Now, the mapping is a `SeqMap[Symbol, Symbol]` to convey its ordering and the `Bind` is created when we are creating the tree of the quote pattern.
nicolasstucki
force-pushed
the
fix-quote-type-variable-bounds
branch
from
a096e75 to
1f4d46c
Compare
smarter
approved these changes
Jun 28, 2023
Comment on lines
+350
to
+351
| val oldBindingsBuffer = mutable.LinkedHashSet.empty[Symbol] | ||
| val newBindingsBuffer = mutable.ListBuffer.empty[Symbol] |
There was a problem hiding this comment.
Now that we're returning a map from old bindings to new bindings without creating Bind nodes, I think the original idea of directly having a LinkedHashMap here makes more sense, sorry for the back and forth!
nicolasstucki
added a commit
that referenced
this pull request
Jun 28, 2023
We add the definition of the `QuotePattern` AST. In this first step, keep the typing of quote patterns untouched and decode the resulting `unapply` into a `QuotePattern`. This AST is used to perform all transformations until the `staging` phase, here it is encoded into an `unapply` and a `Quote` AST. We also encode the AST into an `unapply` when we pickle the tree to keep the same pickled representation as before. Based on #17956
nicolasstucki
added a commit
that referenced
this pull request
Jun 30, 2023
Now, the mapping is a `SeqMap[Symbol, Symbol]` to convey its ordering and the `Bind` is created when we are creating the tree of the quote pattern. Follow up of the refactoring in #17956
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.
When we encode quote patterns into
unapplymethods, we need to create a copy of each type variable. One copy is kept within the quote(in the next stage) and the other is used in theunapplymethod to define the usual pattern type variables. When creating the latter we copied the symbols but did not update the infos. This implies that if type variables would be bounded by each other, the bounds of the copies would be the original types instead of the copies. We need to update those references.To update the info we now create all the symbols in one pass and the update all their infos in a second pass. This also implies that we cannot use the
newPatternBoundSymbolto create the symbol as this constructor will register the info into GADT bounds. Instead we use the plainnewSymbol. Then in the second pass, when we have updated the infos, we register the symbol into GADT bounds.Note that the code in the added test does compiles correctly, but it had the inconsistent bounds. This test is added in case we need to manually inspect the bounds latter. This test does fail to compile in #17935 if this fix is not applied.