[DO NOT MERGE] Illustrate issue with TreeTypeMap and miniphases

[smarter]

Ping @DarkDimius @odersky, I'm not sure how to proceed from here.

The first commit illustrates the issue:

When running TreeTypeMap in a miniphase, the tree we're transforming has
already been transformed by all the miniphases in the same group, but
the symbols we're looking at might be older than the trees, this means
that we can miss transforming some symbols as illustrated in details by
the added testcase which currently fails.

The second commit contains a hack to show that my diagnostic of the issue above is correct, but I don't know how to fix this properly:

With this commit, tests/pos/treetypemap-miniphase.scala now compiles,
but this is only a hack: there are other instances of TreeTypeMap in the
compiler and they would all need to be run with "atGroupEnd", this is
tricky since we are not always calling TreeTypeMap in a context where we
have access to the current TreeTransformer (for example when we create a
TreeTypeMap inside TypeMap to deal with annotations), ideas and
alternative fixes welcome.

[DarkDimius]

In short: we have already encountered this issue and we have introduced changeOwnerAfter that is supposed to be robust against this(which in turn introduced Denotation.transformAfter). See 979ee0f

If I remember correctly @odersky proposed to keep changeOwner though it is unsafe because it is faster.

[DarkDimius]

Similarly: entered is generaly unsafe while enteredAfter is more robust.

[DarkDimius]

try changeOwnerAfter. It's supposed to fix the issue. No need for atGroupEnd.

[DarkDimius]

The correct solution to this issue is to only change denotations\owners in InfoTransormer. Than neither of those functionalities would have been needed.

[odersky]

If I remember correctly @odersky proposed to keep changeOwner though it is unsafe because it is faster.

No, it's the opposite. changeOwnerAfter is faster. I kept changeOwner in elimByName to make sure it was debugged, because we need it in other cases where we duplicate a tree. elimByName exercises the functionality much more than anything else. But now that we have done that, I think it's time to switch to changeOwnerAfter in elimByName.

But there are other tree type maps, for instance in tailrec and extensionMethods. So we need to be careful with those.

[smarter]

The correct solution to this issue is to only change denotations\owners in InfoTransormer. Than neither of those functionalities would have been needed.

That's what I thought too, but that seems like a major refactoring since many phases change denotations while transforming trees.

[smarter]

But there are other tree type maps, for instance in tailrec and extensionMethods. So we need to be careful with those.

In particular, every TypeMap might instantiate a TreeTypeMap to map over an annotation and that can end up creating new symbols: https://github.com/lampepfl/dotty/blob/47d208448e614125446c7f294f8231c3fb7108d6/compiler/src/dotty/tools/dotc/core/Types.scala#L3493

[felixmulder]

Closed in favor of opening an issue