Skip to content
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

Jump to bottom

Regression in ClassfileParser #15166

Closed
WojciechMazur opened this issue May 11, 2022 · 2 comments · Fixed by #15185
Closed

Regression in ClassfileParser #15166

WojciechMazur opened this issue May 11, 2022 · 2 comments · Fixed by #15185
Assignees
@griggt
Labels
compat:java itype:bug regression This worked in a previous version but doesn't anymore
Milestone
3.2.0

Comments

@WojciechMazur
Copy link
Contributor

WojciechMazur commented May 11, 2022 •
edited by nicolasstucki

Issue spotted when trying to compile https://github.com/mjakubowski84/parquet4s using the latest nightly
More logs: https://scala3.westeurope.cloudapp.azure.com/job/buildCommunityProject/775/artifact/build-logs.txt

Compiler version

3.2.0-RC1-bin-20220429-400427d-NIGHTLY
Works with 3.1.2
Fails with 3.1.3-RC2

First bad commit 681e17e in #14426

Minimized code

Needs further minimization:
Sources failing class: https://github.com/apache/yetus/blob/main/audience-annotations-component/audience-annotations/src/main/java/org/apache/yetus/audience/InterfaceAudience.java

//> using scala "3.2.0-RC1-bin-20220429-400427d-NIGHTLY"
// //> using scala "3.1.2" // Last stable working version

//> using lib "org.apache.parquet:parquet-hadoop:1.12.2"

//Any usage of class from library
val x: org.apache.yetus.audience.InterfaceAudience.Private = ???

Output

Error: error while loading InterfaceStability,
class file org/apache/yetus/audience/InterfaceStability.class is broken, reading aborted with class java.lang.UnsupportedOperationException
complete
Warning: Caught: java.lang.AssertionError: assertion failed: failure to resolve inner class:
externalName = org.apache.yetus.audience.InterfaceStability$Evolving,
outerName = org.apache.yetus.audience.InterfaceStability,
innerName = Evolving
owner.fullName = org.apache.yetus.audience.InterfaceStability
while parsing /home/wmazur/.cache/coursier/v1/https/repo1.maven.org/maven2/org/apache/yetus/audience-annotations/0.13.0/audience-annotations-0.13.0.jar(org/apache/yetus/audience/InterfaceAudience.class) while parsing annotations in /home/wmazur/.cache/coursier/v1/https/repo1.maven.org/maven2/org/apache/yetus/audience-annotations/0.13.0/audience-annotations-0.13.0.jar(org/apache/yetus/audience/InterfaceAudience.class)

Expectation

ClassfileParser should be able to read to jar
@WojciechMazur WojciechMazur added itype:bug stat:needs minimization Needs a self contained minimization stat:needs triage Every issue needs to have an "area" and "itype" label labels May 11, 2022
@griggt griggt self-assigned this May 11, 2022
@griggt
Copy link
Collaborator

griggt commented May 11, 2022

Regressed in #14426

@griggt griggt added the regression This worked in a previous version but doesn't anymore label May 11, 2022
@griggt
Copy link
Collaborator

griggt commented May 11, 2022

Minimized to:

InterfaceStability.java

@InterfaceAudience.Public
public class InterfaceStability {
  public @interface Evolving { }
}

InterfaceAudience.java

@InterfaceStability.Evolving
public class InterfaceAudience {
  public @interface Public { }
}

Test.scala

val x: InterfaceAudience.Public = ???
$ javac -version
javac 1.8.0_292

$ scalac -version
Scala compiler version 3.2.0-RC1-bin-SNAPSHOT-git-7c446ce -- Copyright 2002-2022, LAMP/EPFL

$ javac -d . InterfaceAudience.java InterfaceStability.java
$ scalac Test.scala
error while loading InterfaceStability,
class file InterfaceStability.class is broken, reading aborted with class java.lang.UnsupportedOperationException
complete
Caught: java.lang.AssertionError: assertion failed: failure to resolve inner class:
externalName = InterfaceStability$Evolving,
outerName = InterfaceStability,
innerName = Evolving
owner.fullName = InterfaceStability
while parsing ./InterfaceAudience.class while parsing annotations in ./InterfaceAudience.class
1 warning found
1 error found

@griggt griggt added compat:java and removed stat:needs minimization Needs a self contained minimization stat:needs triage Every issue needs to have an "area" and "itype" label labels May 11, 2022
@griggt griggt assigned griggt and unassigned griggt May 12, 2022
@griggt griggt mentioned this issue May 13, 2022
Merged
@Kordyjan Kordyjan added this to the 3.1.3 milestone May 16, 2022
griggt added a commit to griggt/dotty that referenced this issue May 19, 2022
@griggt
Use names instead of type symbols when scanning for pickling annotations
e4b1cfe 
When scanning for Scala pickling annotations, all annotations in all
classfiles (including those produced by the Java compiler) are considered.

Before this commit, a Type and Symbol were created for each discovered
annotation and comparared to the known Scala signature annotation types.
In certain situations (as in tests/pos/i15166), this is problematic, as
the denotation of an annotation symbol defined as a Java inner class may
be forced before the inner class table is populated and setClassInfo is
called on the class root.

Comparing names (as strings) rather than type symbols avoids this situation.

Fixes scala#15166
griggt added a commit to griggt/dotty that referenced this issue May 19, 2022
@griggt
Require seeing ScalaSig before scanning for Scala pickling annotations
bc767cc 
An alternative fix for scala#15166, which aligns with the behavior of the
Scala 2 classfile parser.

Before this commit, all classfiles, including those produced by the Java
compiler and compilers of other JVM languages, were scanned for Scala
pickling annotations. In certain situations (as in tests/pos/i15166),
this is problematic, as the denotation of an annotation symbol defined as
a Java inner class may be forced before the inner class table is populated
and setClassInfo is called on the class root.

We avoid this situation by only performing the pickling annotation scan for
those classfiles having the `ScalaSig` attribute, i.e. those produced by
the Scala 2 compiler.

We also drop support for pickling TASTy using the classfile annotations
  `scala.annotation.internal.TASTYSignature` and
  `scala.annotation.internal.TASTYLongSignature`.
These were never used by the compiler, there are no plans for future use,
and preserving support would complicate this fix.
griggt added a commit to griggt/dotty that referenced this issue May 23, 2022
@griggt
Require seeing ScalaSig before scanning for Scala pickling annotations
57a5da8 
An alternative fix for scala#15166, which aligns with the behavior of the
Scala 2 classfile parser.

Before this commit, all classfiles, including those produced by the Java
compiler and compilers of other JVM languages, were scanned for Scala
pickling annotations. In certain situations (as in tests/pos/i15166),
this is problematic, as the denotation of an annotation symbol defined as
a Java inner class may be forced before the inner class table is populated
and setClassInfo is called on the class root.

We avoid this situation by only performing the pickling annotation scan for
those classfiles having the `ScalaSig` attribute, i.e. those produced by
the Scala 2 compiler.

We also drop support for pickling TASTy using the classfile annotations
  `scala.annotation.internal.TASTYSignature` and
  `scala.annotation.internal.TASTYLongSignature`.
These were never used by the compiler, there are no plans for future use,
and preserving support would complicate this fix.
bishabosha pushed a commit to dotty-staging/dotty that referenced this issue Oct 18, 2022
@griggt @bishabosha
Use names instead of type symbols when scanning for pickling annotations
cd011fb 
When scanning for Scala pickling annotations, all annotations in all
classfiles (including those produced by the Java compiler) are considered.

Before this commit, a Type and Symbol were created for each discovered
annotation and comparared to the known Scala signature annotation types.
In certain situations (as in tests/pos/i15166), this is problematic, as
the denotation of an annotation symbol defined as a Java inner class may
be forced before the inner class table is populated and setClassInfo is
called on the class root.

Comparing names (as strings) rather than type symbols avoids this situation.

Fixes scala#15166
bishabosha pushed a commit to dotty-staging/dotty that referenced this issue Oct 18, 2022
@griggt @bishabosha
Require seeing ScalaSig before scanning for Scala pickling annotations
e519989 
An alternative fix for scala#15166, which aligns with the behavior of the
Scala 2 classfile parser.

Before this commit, all classfiles, including those produced by the Java
compiler and compilers of other JVM languages, were scanned for Scala
pickling annotations. In certain situations (as in tests/pos/i15166),
this is problematic, as the denotation of an annotation symbol defined as
a Java inner class may be forced before the inner class table is populated
and setClassInfo is called on the class root.

We avoid this situation by only performing the pickling annotation scan for
those classfiles having the `ScalaSig` attribute, i.e. those produced by
the Scala 2 compiler.

We also drop support for pickling TASTy using the classfile annotations
  `scala.annotation.internal.TASTYSignature` and
  `scala.annotation.internal.TASTYLongSignature`.
These were never used by the compiler, there are no plans for future use,
and preserving support would complicate this fix.
@Kordyjan Kordyjan modified the milestones: 3.1.3, 3.2.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@griggt griggt

Labels
compat:java itype:bug regression This worked in a previous version but doesn't anymore
Projects
None yet
Milestone
3.2.0
Development

Successfully merging a pull request may close this issue.

Avoid overeager completion of Java annotations in classfile parser griggt/dotty
3 participants
@Kordyjan @griggt @WojciechMazur