Handle TypeProxy of Named Tuples, minimal fix without refactoring #22325
Conversation
| (if normalize then self.normalized else self).dealias match | ||
| case defn.NamedTuple(nmes, vals) => | ||
| val names = nmes.tupleElementTypesUpTo(bound, normalize).getOrElse(Nil).map(_.dealias).map: | ||
| case ConstantType(Constant(str: String)) => str.toTermName | ||
| case t => throw TypeError(em"Malformed NamedTuple: names must be string types, but $t was found.") | ||
| val values = vals.tupleElementTypesUpTo(bound, normalize).getOrElse(Nil) | ||
| names.zip(values) | ||
| case tp: TypeProxy => |
There was a problem hiding this comment.
Not sure about this. In principle it looks OK, but TypeProxy#superType goes through normalize AFAIK, so it is as if the normalize argument has no effect anymore. Does that break things somewhere? What do the tests say?
There was a problem hiding this comment.
All compilation tests are passing, haven't finished the full suite yet. From what I could tell the normalize argument is just used for pretty-printing?
The only unrelated tests that failed with this change were i12456 and i9328 which were fixed with the bounds check. I'm really not sure this is the right use of bounds, but I noticed it wasn't used anywhere in this method but was used in the regular tuple method to limit recursion. Given the docstring, this is a bit of a misuse, but for the moment I couldn’t find a nice way to check for cycles / don’t know if that’s done anywhere else I can reuse?
There was a problem hiding this comment.
OK, should we predicate TypeProxy and following cases on normalize = false then?
There was a problem hiding this comment.
Will the printer need to print trees with invalid cycles? It's used in error messages so I assume it's possible. We can either keep using the original namedTupleElementTypes in the printer, which does not call TypeProxy.supertype (so no need to update it to respect the normalization flag). If the printer does have to deal with cyclic structures but also if it uses the derived namedTupleElementTypes, then we can update TypeProxy to respect the normalization flag but then we'd need some way to check there aren't cycles.
| @@ -128,6 +128,7 @@ class TypeUtils: | |||
| case None => throw new AssertionError("not a tuple") | |||
|
|
|||
| def namedTupleElementTypesUpTo(bound: Int, normalize: Boolean = true)(using Context): List[(TermName, Type)] = | |||
| if bound < 0 then Nil else | |||
There was a problem hiding this comment.
I think that's a bit too coarse. You recurse 2^31 times in the normal case. It's a bit tricky to limit recursion in the face of illegal recursive aliases, because we do the namedTupleElementTypesUpTo before Typer in desugaring. There we have not yet checked and ruled out such illegal cycles.
One idea would be to leave namedTupleElementTypesUpTo as it is for desugaring, but have another version that follows superTypes of TypeProxies in Typer and Completions. Or have another Boolean flag that says whether we should follow proxies and &/I.
There was a problem hiding this comment.
One idea would be to leave namedTupleElementTypesUpTo as it is for desugaring, but have another version that follows superTypes of TypeProxies in Typer and Completions.
It's not clear to me why desugaring should be treated specially, in
scala3/compiler/src/dotty/tools/dotc/ast/Desugar.scala
Lines 1747 to 1748 in 0677702
pt.namedTupleElementTypes we call pt.classSymbol and classSymbol also recurses on TypeProxy: scala3/compiler/src/dotty/tools/dotc/core/Types.scala
Lines 580 to 581 in 0677702
There was a problem hiding this comment.
But selNames.isEmpty will be false
…rinter using pre-typing version
Per the discussion on #22150 (comment) we should do a refactor of how we check for Named Tuples and move the logic into unapply, which is WIP. Not sure if we should just do the refactor right away, or merge the slightly quicker fix that's in this PR first?