Skip to content

Fix #11464: avoid expanding local paramRef in HKTypeLambda #11465

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 2, 2021
Merged

Fix #11464: avoid expanding local paramRef in HKTypeLambda #11465

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b987eeb
Fix #11464: avoid expanding local paramRef in HKTypeLambda
liufengyun Feb 18, 2021
68f6ace
Enable pickling test
liufengyun Feb 20, 2021
5ff7b10
Address review
liufengyun Mar 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions compiler/src/dotty/tools/dotc/core/TypeOps.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,7 @@ object TypeOps:
def avoid(tp: Type, symsToAvoid: => List[Symbol])(using Context): Type = {
val widenMap = new ApproximatingTypeMap {
@threadUnsafe lazy val forbidden = symsToAvoid.toSet
val locals = util.HashSet[Type]()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an optimization, this could be an uninitialized var which is only initialized the first time we encounter an HKTypeLambda.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(or just make it a lazy val).

def toAvoid(sym: Symbol) = !sym.isStatic && forbidden.contains(sym)
def partsToAvoid = new NamedPartsAccumulator(tp => toAvoid(tp.symbol))

Expand Down Expand Up @@ -429,17 +430,22 @@ object TypeOps:
case _ =>
emptyRange // should happen only in error cases
}
case tp: ThisType if toAvoid(tp.cls) =>
range(defn.NothingType, apply(classBound(tp.cls.classInfo)))
case tp: ThisType =>
tp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why this change is needed and why it's safe to not do avoidance here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this, this is an optimization -- the rationale is that ThisType doesn't exist outside of class, thus they are always safe in avoidance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think this makes sense but it'd be good to check with an assert that this never happens in our tests (once the tests pass we can disable the assert for performance), and to add a comment explaining that this is safe.

Copy link
Contributor Author

@liufengyun liufengyun Mar 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smarter The CI confirmed the assertion. See https://github.com/lampepfl/dotty/actions/runs/614834038

case tp: SkolemType if partsToAvoid(Nil, tp.info).nonEmpty =>
range(defn.NothingType, apply(tp.info))
case tp: TypeVar if mapCtx.typerState.constraint.contains(tp) =>
val lo = TypeComparer.instanceType(
tp.origin, fromBelow = variance > 0 || variance == 0 && tp.hasLowerBound)(using mapCtx)
val lo1 = apply(lo)
if (lo1 ne lo) lo1 else tp
case tp: LazyRef if isExpandingBounds =>
emptyRange
case tp: LazyRef =>
if locals.contains(tp.ref) then tp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it safe to return tp here, doesn't it need to be avoided if it contains a local symbol?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here locals mean locally bound ParamRef in a HKTypeLambda -- we remember that in the set locals. The set is not the set of local symbols to be avoided.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, would be good to give it another name to avoid confusing it with localSyms I think :). Maybe localParamRefs ?

else if isExpandingBounds then emptyRange
else mapOver(tp)
case tl: HKTypeLambda =>
locals ++= tl.paramRefs
mapOver(tl)
case _ =>
mapOver(tp)
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/core/Types.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2924,7 +2924,7 @@ object Types {
tp match {
case tp: TypeRef => apply(x, tp.prefix)
case tp: RecThis => RecType.this eq tp.binder
case tp: LazyRef => true // To be safe, assume a reference exists
case tp: LazyRef => this(x, tp.ref)
case _ => foldOver(x, tp)
}
}
Expand Down
1 change: 0 additions & 1 deletion compiler/test/dotc/pos-test-pickling.blacklist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,4 +60,3 @@ i9793.scala

# lazy_implicit symbol has different position after pickling
i8182.scala

16 changes: 16 additions & 0 deletions tests/pos/i11464.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
trait Txn[T <: Txn[T]]

trait Adjunct

trait Type0
trait Type[A1, Repr[~ <: Txn[~]] <: Expr[~, A1]] extends Type0

object Expr {
def test(peer: Type0): Adjunct = {
new AdjunctImpl(peer.asInstanceOf[Type[Any, ({ type R[~ <: Txn[~]] <: Expr[~, Any] }) # R]])
}
}

trait Expr[T <: Txn[T], +A]

class AdjunctImpl[A, E[~ <: Txn[~]] <: Expr[~, A]](tpe: Type[A, E]) extends Adjunct