SI-10075 propagate annotations to lazy val's underlying field

[adriaanm]

This likely regressed in #5294.

Fixes scala/scala-dev#213

Review by @retronym

[retronym]

Not sure we've got this sorted yet. Here's a test case for combinations of trait defined/class defined/trait-mixed in members; val/var/lazy val; field/getter/setter meta annotations that shows the difference between 2.11.8, 2.12.0, and 2.12.1 (as proposed in this PR).

There are still some problems:

• A field targeted annotation on a class member lazy val ends up on both the field and the getter.
• A field targeted annotation on a trait member lazy val is lost (not propagated to the field in the class that mixes in the trait)
• The static accessor method of a getter in a trait contain the same list of annotations as the method it accesses. The same probably occurs for regular concrete trait methods.

[adriaanm]

Thanks for the test case -- where did it go, though?

[adriaanm]

I think I have fixes for the first two. Not sure about the static accessors yet

[retronym]

https://gist.github.com/retronym/c46af6372d325a8bdd1d8cf1315eccf5

Static accessors are spawned in the backend using

scala/src/compiler/scala/tools/nsc/ast/TreeGen.scala

Line 329 in d9974be

final def mkStatic(orig: DefDef, newName: Name, maybeClone: Symbol => Symbol): DefDef = {

[adriaanm]

Thanks. So you're saying we should just drop all annotations from the static accessor?

[adriaanm]

I'll incorporate your test tomorrow as well as the logic for the static accessor annotation triage

[retronym]

Yes, I think we should drop them.

[durban]

Shouldn't the lazy vals be forced before trying to serialize? Otherwise the backing field will be null, and serialization will succeed even if it is not transient, right?

[adriaanm]

Good point. Thanks!

[adriaanm]

One test failure:

!!  201 - run/junitForwarders                       [non-zero exit code]
##### Log file '/home/jenkins/workspace/scala-2.12.x-validate-test@2/test/files/run/junitForwarders-run.log' from failed test #####

java.lang.AssertionError: assertion failed: found: $init$ - ;foo - @org.junit.Test();foo$ - 
expected: $init$ - ;foo - @org.junit.Test();foo$ - @org.junit.Test()
	at Test$.check(C_1.scala:10)

[adriaanm]

Updated test, since it had a nice TODO message that confirmed it needed updating :-) thanks, @lrytz

[dwijnand]

It's green now

[retronym]

LGTM, tricky stuff!

[adriaanm]

If I understand @lrytz's comment at scala/scala-dev#213 (comment) correctly, I should drop the withoutAnnotations for static accessors. I think we should either drop all annotations or we keep them all. If we have to keep @inline, someone may be able to make a similar case for other annotations.

Alternatively, could we avoid looking at annotations of static accessors in the inliner? Could the inliner see through the static accessor and decide inlining based on the target of the accessor? inlineRequest is already special cased on isTraitSuperAccessor to use findSingleCall, but I didn't look deeper than that.

[lrytz]

It turns out the inliner does not rely on the @inline annotation on static super accessors.

For every member method, we store some metadata in the ClassBType (The MethodInlineInfo, it contains is information that can only be obtained from a method symbol, but, without unpickling, not from the classfile signature. Example: is the method annotated @inline. This information is added to the class as a custom attribute and used in case the inliner pulls in a new classfile.)

When building a ClassBType, the member method symbols are obtained from the classSym.info.decls. However, the static accessors are not entered into the class scope, so they don't show up. That's why there's a special case that creates additional MethodInlineInfos for static accessors (

scala/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala

Lines 598 to 608 in ee1c02b

	if (needsStaticImplMethod(methodSym)) {
	val staticName = traitSuperAccessorName(methodSym).toString
	val selfParam = methodSym.newSyntheticValueParam(methodSym.owner.typeConstructor, nme.SELF)
	val staticMethodType = methodSym.info match {
	case mt @ MethodType(params, res) => copyMethodType(mt, selfParam :: params, res)
	}
	val staticMethodSignature = staticName + methodBTypeFromMethodType(staticMethodType, isConstructor = false)
	val staticMethodInfo = MethodInlineInfo(
	effectivelyFinal = true,
	annotatedInline = info.annotatedInline,
	annotatedNoInline = info.annotatedNoInline)

). The annotatedInline bit is taken from the corresponding member method.

[adriaanm]

Aha! Thanks for explaining. So this is ready to go as far as you're concerned?

[lrytz]

yes, LGTM

[adriaanm]

Cool. When I split up my old commit in two, I missed one test case. The last commit is green, so I'm going to merge. One broken test isn't going to break future git bisecting.