WIP: Fix #11251 #11335
WIP: Fix #11251 #11335
Conversation
| @@ -501,7 +501,7 @@ class Inliner(call: tpd.Tree, rhsToInline: tpd.Tree)(using Context) { | |||
| ref(rhsClsSym.sourceModule) | |||
| else | |||
| inlineCallPrefix | |||
| val binding = ValDef(selfSym.asTerm, rhs).withSpan(selfSym.span).setDefTree | |||
| val binding = ValDef(selfSym.asTerm, rhs.changeOwner(ctx.owner,selfSym)).withSpan(selfSym.span).setDefTree | |||
There was a problem hiding this comment.
This looks dangerous from a compilation-speed perspective. The invariant to keep is that the owners of definition inside the ValDef of symbol x must be either x, or one of x's owners up to and including the method containing x. changeOwner is expensive since it does a full tree-traversal and it inspects every type in that tree. so we should do this only if it is absolutely necessary.
One idea would be to do a changeOwner only if !ctx.owner.isContainedIn(selfSym.enclosingMethodOrClass).
There was a problem hiding this comment.
Currently -Y"check:macros" check more strict invariant: that inside definition symbol should have the owner as nearest enclosing declaration:
https://github.com/lampepfl/dotty/blob/a63b1d114179d79be32a1ee6552fc160c1bb2410/compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala#L2804
Maybe we should change this check, to keep a set of 'allowed owners' in check traverse.
On the other side, moving block of code from one method to another (as we do during inlining), requires changing owner, and with 'less strict invariant' in any case full traverse will be needed (because we don't know - what in this tree). With more strict (which enforced by current yCheck) - changeOwner can omit subtrees of low-level definitions since they already have correct owners.
Don't know, what policy is better.
There was a problem hiding this comment.
Yes, quite possibly. I let @nicolasstucki comment on that.
There was a problem hiding this comment.
We have QuoteUtils.changeOwnerOfTree for this purpose. This variant checks if the tree has an owner that needs to be changed.
There was a problem hiding this comment.
Can we reproduce the failure without macros (only inline)? If not, this might be the wrong place to change the owner of that tree. It might also be an issue with the owners in the TreeMap or one of the methods the treemap calls.
There was a problem hiding this comment.
But yCheckOwner is defined in QuoteImpl, so it will not be called without macros.
Note, that currently macros not change anything and used only to call yCheckOwner to tree.
(I can move yCheckOwner be a separate check between phases and look).
|
@rssh I edited the comment so that the issues are automatically linked. It used to work when issues were mentioned in the title but doesn't anymore. |
compiler/test/dotty/tools/dotc/BootstrappedOnlyCompilationTests.scala
Outdated
Show resolved
Hide resolved
| @@ -501,7 +501,7 @@ class Inliner(call: tpd.Tree, rhsToInline: tpd.Tree)(using Context) { | |||
| ref(rhsClsSym.sourceModule) | |||
| else | |||
| inlineCallPrefix | |||
| val binding = ValDef(selfSym.asTerm, rhs).withSpan(selfSym.span).setDefTree | |||
| val binding = ValDef(selfSym.asTerm, rhs.changeOwner(ctx.owner,selfSym)).withSpan(selfSym.span).setDefTree | |||
There was a problem hiding this comment.
We have QuoteUtils.changeOwnerOfTree for this purpose. This variant checks if the tree has an owner that needs to be changed.
| @@ -501,7 +501,7 @@ class Inliner(call: tpd.Tree, rhsToInline: tpd.Tree)(using Context) { | |||
| ref(rhsClsSym.sourceModule) | |||
| else | |||
| inlineCallPrefix | |||
| val binding = ValDef(selfSym.asTerm, rhs).withSpan(selfSym.span).setDefTree | |||
| val binding = ValDef(selfSym.asTerm, rhs.changeOwner(ctx.owner,selfSym)).withSpan(selfSym.span).setDefTree | |||
There was a problem hiding this comment.
Can we reproduce the failure without macros (only inline)? If not, this might be the wrong place to change the owner of that tree. It might also be an issue with the owners in the TreeMap or one of the methods the treemap calls.
(also split the test from scala#11335)
|
// test is currently broken because #1400 not here, will be fixed automatically after one will be merged in this branch (I guess the right way will be from master). |
|
(hmm, looks like something fixed this). |
|
@rssh could you squash and rebase your commits? |
done |
Fixes #11251
-- add changing ownity when generating ValDef for this proxy. (which can be a result of generated inline), see commit