Fix #13503 by adding a "catch all" clause in pickleDef pattern match

[soronpo]

No description provided.

[dwijnand]

[info] Test dotty.tools.dotc.CompilationTests.pickling started
class dotty.tools.dotc.reporting.Diagnostic$Error at ?: pickling difference for module class i13503$package$ in tests/pos/i13503.scala, for details:

  diff before-pickling.txt after-pickling.txt while compiling tests/pos/i13503.scala
Fatal compiler crash when compiling: tests/pos/i13503.scala:
pickling difference for module class i13503$package$ in tests/pos/i13503.scala, for details:

  diff before-pickling.txt after-pickling.txt
...
Test dotty.tools.dotc.CompilationTests.pickling failed
...
tests/pos/i13503.scala failed

[smarter]

I think the problem is in the typer, not the pickler. Literal is not a TypeTree so it shouldn't show up in tpt, I would expect a TypeTree(ConstantType(Constant(123))) instead /cc @nicolasstucki

[soronpo]

So is there anything wrong with my initial commit that just ignores Literal? Is there really a problem in the typer?

[smarter]

well we do need to pickle the type to be able to restore it in unpickling so I don't see how the tpt field could be ignored.

[dwijnand]

Yeah, your first commit fails with

[info] Test dotty.tools.dotc.CompilationTests.pickling started
java.lang.AssertionError: assertion failed: illegal modifier tag 111 at Addr(333), end = Addr(331) while compiling tests/pos/i13503.scala

[nicolasstucki]

TypeTree(ConstantType(Constant(123))) makes more sense. We should find where this tree is created and try to change it.

[nicolasstucki]

I wondering if Ycheck should have caught this as it is a TermTree used as a type tree.

[soronpo]

From the discussion I get that this PR is the wrong approach, so I'm closing it.
I'll be glad to submit a new PR with a little guidance of where to attack the problem.

[dwijnand]

I'll be glad to submit a new PR with a little guidance of where to attack the problem.

There's two ways I would tackle this.

1. Find out what mdef.symbol id is (e.g 123), and uncomment the assert in Symbol to die when id == 123. That will get you to where that tree is created, and maybe from there you can figure out why it isn't wrapping it in a TypeTree.

2. Alternatively you can look to adding to the -Ycheck logic, because type trees should never be a TermTree, so you can extend the checking. That might give you some interesting other cases from the tests, but otherwise it's just a good check to have under Ycheck, for the future.

[smarter]

Instead of debugging using symbol id, we can use a tree uniqueId (via -Yshow-tree-ids and -Ydebug-tree-with-id) to figure out where the right-hand-side tree Literal(Constant(123)) is created:

Debug tree (id=504) creation 
Literal(Constant(123))

java.lang.Exception: Stack trace
        at java.lang.Thread.dumpStack(Thread.java:1336)
        at dotty.tools.dotc.ast.Positioned.allocateId(Positioned.scala:39)
        at dotty.tools.dotc.ast.Positioned.<init>(Positioned.scala:41)
        at dotty.tools.dotc.ast.Trees$Tree.<init>(Trees.scala:56)
        at dotty.tools.dotc.ast.Trees$Literal.<init>(Trees.scala:545)
        at dotty.tools.dotc.ast.untpd$.Literal(untpd.scala:373)
        at dotty.tools.dotc.ast.tpd$.Literal(tpd.scala:57)
        at dotty.tools.dotc.ast.TypedTreeInfo.constToLiteral(TreeInfo.scala:579)
        at dotty.tools.dotc.ast.TypedTreeInfo.constToLiteral$(TreeInfo.scala:394)
        at dotty.tools.dotc.ast.tpd$.constToLiteral(tpd.scala:22)
        at dotty.tools.dotc.typer.Inliner$InlineTyper.typedSelect(Inliner.scala:1507)
        at dotty.tools.dotc.typer.Typer.typedNamed$1(Typer.scala:2725)
        at dotty.tools.dotc.typer.Typer.typedUnadapted(Typer.scala:2817)
        at dotty.tools.dotc.typer.ReTyper.typedUnadapted(ReTyper.scala:121)
        at dotty.tools.dotc.typer.Inliner$InlineTyper.typedUnadapted(Inliner.scala:1635)
        at dotty.tools.dotc.typer.Typer.typed(Typer.scala:2883)
        at dotty.tools.dotc.typer.Typer.typed(Typer.scala:2887)
        at dotty.tools.dotc.typer.Typer.typedType(Typer.scala:3005)
        at dotty.tools.dotc.typer.Typer.typedTypeDef(Typer.scala:2278)
        at dotty.tools.dotc.typer.Typer.typedTypeOrClassDef$2(Typer.scala:2745)
        at dotty.tools.dotc.typer.Typer.typedNamed$1(Typer.scala:2747)
        at dotty.tools.dotc.typer.Typer.typedUnadapted(Typer.scala:2817)
        at dotty.tools.dotc.typer.ReTyper.typedUnadapted(ReTyper.scala:121)
        at dotty.tools.dotc.typer.Inliner$InlineTyper.typedUnadapted(Inliner.scala:1635)
[...]

From that we can see that the problem comes from the call to constToLiteral in https://github.com/lampepfl/dotty/blob/3a6a9499f0bd1821351f052631a0a1d3842f05c1/compiler/src/dotty/tools/dotc/typer/Inliner.scala#L1507 where Select(Inlined(EmptyTree,List(),Ident(given_First)),Out) (which returns true to .isType) gets reduced to Literal(Constant(123)) (which returns false to .isType), as far as I can tell, constToLiteral isn't designed to handle type trees as input and we should add an assert to avoid misusing it like this.

[soronpo]

So if I understand correctly, we have the following issues:

• The one in anonymous class crash pickler via transparent inline cascade of a literal type member #13503
• The fact that -Ycheck didn't catch this.
• The lack of assert in constToLiteral that could have prevented this.

Should all three be solved in the same PR or separate ones?

[smarter]

The Ycheck one is less useful I'd say, even if we add an assert there, it won't help find the root cause of any future issue, we do have assertions on tree creations that could be changed to check that tpt.isType is true (https://github.com/lampepfl/dotty/blob/3a6a9499f0bd1821351f052631a0a1d3842f05c1/compiler/src/dotty/tools/dotc/ast/Trees.scala#L822), but it's just as easy to add that by hand whenever the compiler crashes to debug it.

[soronpo]

The new PR is #13578