Skip to content

Assume StubSymbols are not StaticAnnotations#8379

Closed
retronym wants to merge 1 commit intoscala:2.12.xfrom
retronym:ticket/11679
Closed

Assume StubSymbols are not StaticAnnotations#8379
retronym wants to merge 1 commit intoscala:2.12.xfrom
retronym:ticket/11679

Conversation

@retronym
Copy link
Member

In the reproduction in scala/bug#11679, an SBT build that uses
--release 8 and as such does not have sun._ on the classpath,
APIPhase (a custom compiler phase in SBT/Zinc that serializes the API
of each file to perform change detection in incremental compilation)
is attempting to serialize the API of the member
@CallerSensitive ClassLoader getParent that
de.sciss.synth.proc.impl.MemoryClassLoader inherits from
j.l.ClassLoader.

Until that point, scalac was doing okay without having a classfile
for CallerSensitve -- it just used a StubSymbol in its place.
Scala's pickle phase only serializes .decls, not .members.

When APIPhase filters the list of annotations:

https://github.com/sbt/zinc/blob/4b414b6677/internal/compiler-bridge/src/main/scala/xsbt/ExtractAPI.scala#L789-L800

for those that extend scala.StaticAnnotation, the stub symbol fails
and aborts compilation. Static annotations are part of the API because
the are could affect how client code is compiled.

This commit changes AnnotationInfo.isStatic to return false
for annotations to absent classfiles. It also makes sure that we
still have a hard-error if annotations based on absent classfiles
are processed by the pickle phase.

I have manually tested this commit with the SBT project in the bug.

Fixes scala/bug#11679

In the reproduction in scala/bug#11679, an SBT build that uses
`--release 8` and as such does not have `sun._` on the classpath,
`APIPhase` (a custom compiler phase in SBT/Zinc that serializes the API
of each file to perform change detection in incremental compilation)
is attempting to serialize the API of the member
`@CallerSensitive ClassLoader getParent` that
`de.sciss.synth.proc.impl.MemoryClassLoader` inherits from
`j.l.ClassLoader`.

Until that point, scalac was doing okay without having a classfile
for `CallerSensitve` -- it just used a `StubSymbol` in its place.
Scala's pickle phase only serializes `.decls`, not `.members`.

When `APIPhase` filters the list of annotations:

  https://github.com/sbt/zinc/blob/4b414b6677/internal/compiler-bridge/src/main/scala/xsbt/ExtractAPI.scala#L789-L800

for those that extend `scala.StaticAnnotation`, the stub symbol fails
and aborts compilation. Static annotations are part of the API because
the are could affect how client code is compiled.

This commit changes `AnnotationInfo.isStatic` to return false
for annotations to absent classfiles. It also makes sure that we
still have a hard-error if annotations based on absent classfiles
are processed by the pickle phase.

I have manually tested this commit with the SBT project in the bug.

Fixes scala/bug#11679
@lrytz
Copy link
Member

lrytz commented Aug 28, 2019

It seems to me this is a bit of a flaw in the design of the -release feature / ct.sym. The symbols in ct.sym refernce types that don't exist within it.

Could we instead change our classpath to fall back to the classfiles in the jar for symbols that don't exist? Maybe with a whitelist? We could scan ct.sym to come up with such a whitelist.

@retronym
Copy link
Member Author

retronym commented Aug 28, 2019

In general, I don't believe that Zinc should fail when a class being compiled has an inherited member that is annotated with an absent class.

If we were to add sun._ to the compiler's classpath under --release 8 we'd differ from what javac does and direct references to those classes from the code being compiled would be allowed, which doesn't seem desirable.

@lrytz
Copy link
Member

lrytz commented Aug 28, 2019

doesn't seem desirable

I agree there.

don't believe that Zinc should fail

Would it be cleaner to fix it in Zinc?

@retronym retronym closed this Aug 29, 2019
@SethTisue SethTisue removed this from the 2.12.10 milestone Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants