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.

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

signature inference for overriding val should propagate annotations #10471

Closed
adriaanm opened this Issue Aug 16, 2017 · 13 comments

Comments

Projects
None yet
2 participants
@adriaanm
Member

adriaanm commented Aug 16, 2017

@scala.annotation.meta.field class blort extends scala.annotation.StaticAnnotation
class C1 {
  @blort val foo = "hi"
}
object X {
  def accessIt(c: C2) = c.foo
}
class C2 extends C1 {
  @blort override val foo = "bye"
  @blort val bar = "whee"
}

under -Xprint:typer, we see that the field for C2.foo is not annotated with @blort

@adriaanm adriaanm added this to the 2.12.4 milestone Aug 16, 2017

@adriaanm

This comment has been minimized.

Member

adriaanm commented Aug 16, 2017

@lrytz found the spot where the annotation is dropped:

When the type completer for the getter C2.foo runs, it invokes
typeSig(valDef, Nil), which then calls vdef.symbol setInfo tp,
where tp is taken from the overriden symbol. This discards
the type completer of the vdef.symbol, which is the field symbol
foo . That completer would have filtered the annotations and
assigned them to the field symbol
when completed.

@adriaanm

This comment has been minimized.

Member

adriaanm commented Aug 16, 2017

the regression was introduced by scala/scala#5141 to fix #2742

@adriaanm adriaanm changed the title from signature inference of overriding val should propagate annotations to signature inference for overriding val should propagate annotations Aug 16, 2017

@adriaanm

This comment has been minimized.

@adriaanm

This comment has been minimized.

Member

adriaanm commented Aug 16, 2017

Simply dropping the calls to patchSymInfo breaks pos/t6780.scala. I've been trying to remember what the second comment was about:

need to re-align with assignTypeToTree, as the type we're returning from valDefSig
(tptFromRhsUnderPt) may actually go to the accessor, not the valdef (and if assignTypeToTree returns a subtype of pt, we would be out of synch between field and its accessors),
and thus the type completer won't fix the symbol's info for us -- we set it to tmpInfo above,
which may need to be improved to tptFromRhsUnderPt

I'll experiment some more tomorrow.

/cc @retronym

@lrytz

This comment has been minimized.

Member

lrytz commented Aug 17, 2017

When removing the patchSymInfo, here's what leads to the cycle of t6780

object O {
  implicit def i: Int = 0
}
import O._
trait Foo {
  implicit val v1: Any
}
trait Bar1 extends Foo {
  implicit val v1      = {implicitly[Int]; ()}
}
Bar1.v1Getter.info
..
Bar1.this.type.computeMemberType(Bar1.v1Getter) --> sym.tpeHK
..
FindMembers.isNewMember(member = Bar1.v1Getter, other = Foo.v1Getter) --> memberTypeLow(member) matches memberTypeHi(other)
FindMembers.addMemberIfNew(sym = Foo.v1Getter)
..
Bar1.this.type.findMembers(requiredFlags = IMPLICIT)
..
Bar1.this.type.implicitMembers
..
Context.implicits  --> owner.thisType.implicitMembers
..
Context.implicitss
..
ImplicitSearch.bestImplicit
..
ImplicitSearch(implicitly[Int], pt = Int)
..
applyImplicitArgs(implicitly[Int])
..
adapt(implicitly[Int])
..
typed1({ implicitly[Int]; () })
..
assignTypeToTree(ValDef(...), pt = Any) <<< pt is taken from the overridden symbol
valDefSig(ValDef(...))                  <<< here is the call to `patchSymInfo` that I removed, right before the call to `assignTypeToTree`
..
typeSig(ValDef(implicit val v1 = ...))
..
ValTypeCompleter.completeImpl(sym = Bar1.v1Getter)
..
typed1(ValDef(implicit val v1 = ...)) --> tree.symbol.initialize
type-checking Bar1
@lrytz

This comment has been minimized.

Member

lrytz commented Aug 17, 2017

I checked why we don't have the t6780 problem with methods (change the two val v1 above into def), and found what I overlooked: there is a setInfo call in methodSig. This prevents the cycle.

So now I need to check why the original problem (annotations disappearing) doesn't happen for methods.

In larger terms, this is related to #10459 (why do we even assign annotations in the type completer? Couldn't we do it using lazy annotations directly in the namer?)

lrytz added a commit to lrytz/scala that referenced this issue Aug 17, 2017

Don't skip the ValTypeCompleter from AccessorTypeCompleter
When requesting the field's info in the AccessorTypeCompleter, don't
skip the ValTypeCompleter, which sets the annotations on the field
symbol.

Fixes scala/bug#10471
@lrytz

This comment has been minimized.

Member

lrytz commented Aug 17, 2017

So the difference is that the AccessorTypeCompleter of the getter invokes typeSig(valDef, Nil) and not valDef.symbol.info. The latter would go through the ValTypeCompleter, which would set the annotations before calling typeSig.

Testing this in scala/scala#6040.

So it's also order-dependent: when the field symbol is completed before the getter symbol, the annotation is there. This can be observed by removing the accessIt method in the example.

@lrytz

This comment has been minimized.

Member

lrytz commented Aug 17, 2017

Build log: https://scala-ci.typesafe.com/job/scala-2.12.x-validate-test/5436/consoleFull

pos/t7046-2 failed, maybe we need a special-case for macros:

Test_2.scala:6: error: erroneous or inaccessible type
  val x = Macro.p1_Base_knownDirectSubclasses
      ^

neg/names-defaults-neg.scala lost some errors, need to check.

neg/t1038.scala got a new error, need to check.

In neg/t2388.scala the cycle is reported in a different spot.

@lrytz

This comment has been minimized.

Member

lrytz commented Aug 18, 2017

scala/scala#6040 is not a working solution, the additional calls to .info cause new cyclic reference errors. The test failures above are due to that. Maybe we just have to add the annotation filtering for the field in AccessorTypeCompleter? @adriaanm what do you recommend?

@adriaanm

This comment has been minimized.

Member

adriaanm commented Aug 18, 2017

That would probably be the safest fix for now. adriaanm/scala@33d444d

@lrytz

This comment has been minimized.

Member

lrytz commented Aug 19, 2017

If I understand correctly, this would pick annotations from the overridden symbol? That's not what should be done, we need the (filtered) annotations that are in the ValDef tree.

lrytz added a commit to lrytz/scala that referenced this issue Aug 19, 2017

Don't skip the ValTypeCompleter in the AccessorTypeCompleter
When the AccessorTypeCompleter gets the type signature of the
ValDef, ensure that the ValTypeCompleter runs (which sets the
annotations on the field symbol) instead of just calling typeSig.

Fixes scala/bug#10471

lrytz added a commit to lrytz/scala that referenced this issue Aug 19, 2017

Don't skip the ValTypeCompleter in the AccessorTypeCompleter
When the AccessorTypeCompleter gets the type signature of the
ValDef, ensure that the ValTypeCompleter runs (which sets the
annotations on the field symbol) instead of just calling typeSig.

Fixes scala/bug#10471

lrytz added a commit to lrytz/scala that referenced this issue Aug 19, 2017

Don't skip the ValTypeCompleter in the AccessorTypeCompleter
When the AccessorTypeCompleter gets the type signature of the
ValDef, ensure that the ValTypeCompleter runs (which sets the
annotations on the field symbol) instead of just calling typeSig.

Fixes scala/bug#10471
@lrytz

This comment has been minimized.

Member

lrytz commented Aug 19, 2017

@adriaanm

This comment has been minimized.

Member

adriaanm commented Aug 19, 2017

You're right. Thanks, your fix LGTM.

lrytz added a commit to lrytz/scala that referenced this issue Aug 20, 2017

Don't skip the ValTypeCompleter in the AccessorTypeCompleter
When the AccessorTypeCompleter gets the type signature of the
ValDef, ensure that the ValTypeCompleter runs (which sets the
annotations on the field symbol) instead of just calling typeSig.

Fixes scala/bug#10471
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment