Skip to content

Invalidate symbols for artifact classfiles, refactor classfile parser#5952

Merged
lrytz merged 3 commits intoscala:2.13.xfrom
lrytz:skipScalaRaw
Jun 26, 2017
Merged

Invalidate symbols for artifact classfiles, refactor classfile parser#5952
lrytz merged 3 commits intoscala:2.13.xfrom
lrytz:skipScalaRaw

Conversation

@lrytz
Copy link
Member

@lrytz lrytz commented Jun 20, 2017

No longer run the classfile parser on Scala generated classfiles that
don't have a Scala signature (module classes, inner classes, etc).

Various cleanups in the classfile parser, minimize the work performed
on Scala classfiles. Before, the attributes section was parsed twice:
once to find the ScalaSig attribute, the second time to find the
ScalaSignature in the RuntimeVisibleAnnotations. Now everything happens
in the first iteration.

Also fixes a bug in the backend: classes ending in $ did not get a
ScalaSignature by mistake. They were filtered out by the name-based
test that is supposed to identify module classes.

Also cleans up an unnecessary abstraction in the representation of
ScalaSignature annotations.

@lrytz
Remove unnecessary abstraction
e216e0e 
Added in ced7214, no longer needed since ICodeReader is gone.
@scala-jenkins scala-jenkins added this to the 2.13.0-M2 milestone Jun 20, 2017
lrytz added 2 commits June 20, 2017 22:37
@lrytz
Invalidate symbols for artifact classfiles, refactor classfile parser
3aea776 
No longer run the classfile parser on Scala generated classfiles that
don't have a Scala signature (module classes, inner classes, etc).

Various cleanups in the classfile parser, minimize the work performed
on Scala classfiles. Before, the attributes section was parsed twice:
once to find the ScalaSig attribute, the second time to find the
ScalaSignature in the RuntimeVisibleAnnotations. Now everything happens
in the first iteration.

Also fixes a bug in the backend: classes ending in `$` did not get a
ScalaSignature by mistake. They were filtered out by the name-based
test that is supposed to identify module classes.
@lrytz
ScalaSigBytes is no longer a ClassfileAnnotArg
1e9052c 
This abstraction was not necessary, it was just dealing with encoding
the signature. This is now handled more directly.
@scala scala deleted a comment from SethTisue Jun 21, 2017
retronym
retronym approved these changes Jun 23, 2017
View reviewed changes
Copy link
Member

@retronym retronym left a comment

Choose a reason for hiding this comment

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

Such an improvement! LGTM.

We should consider backporting the "no scala sig for classes ending in '$'" fix, if it is a regression.

@lrytz
Copy link
Member Author

lrytz commented Jun 26, 2017

Good point, I'll submit a backport for this fix.

Jardiff (lib+reflect+comp) looks good:

➜  sandbox git:(c8b0e1da51) jardiff a b
WARN: unable to invoke scalap on: a/scala/concurrent/duration/package.class: Unexpected failure
WARN: unable to invoke scalap on: b/scala/concurrent/duration/package.class: Unexpected failure
diff --git a/scala/runtime/Nothing$.class.scalap b/scala/runtime/Nothing$.class.scalap
new file mode 100644
index 0000000..367d133
--- /dev/null
+++ b/scala/runtime/Nothing$.class.scalap
@@ -0,0 +1,4 @@
+package scala.runtime
+sealed abstract class Nothing$ extends scala.Throwable {
+  def this() = { /* compiled code */ }
+}
diff --git a/scala/runtime/Null$.class.scalap b/scala/runtime/Null$.class.scalap
new file mode 100644
index 0000000..2a0e3a4
--- /dev/null
+++ b/scala/runtime/Null$.class.scalap
@@ -0,0 +1,3 @@
+package scala.runtime
+sealed abstract class Null$ extends scala.AnyRef {
+}

When compiling the compiler, a number of scala-artifact-classfiles are being completed. They fall in two categories. When completed, they are now unlinked and marked non-existing:

scala.annotation.meta.package$: parsed during typer when computing
lazy val metaAnnotations: Set[Symbol] = getPackage(TermName("scala.annotation.meta")).info.members filter (_ isSubClass StaticAnnotationClass) toSet

scala.runtime.AbstractFunction2$mcZDD$sp and many more specialized classfiles. They are completed as part of the fix of scala/bug#5545, c5f68c1. We could revert that fix: we don't parse the classfile anymore, so the crash with incompatible signatures doesn't happen. But the symbols would still get completed, namely during the backend (as described in the bug report).

I'll leave the fix in place. In general, when a new class symbol is created synthetically with a name that matches a classfile that already exists on the classpath, it's probably a good idea to unlink the existing symbol (by completing it) and its companionModule/companionModuleClass.

@lrytz lrytz merged commit 12d8ad6 into scala:2.13.x Jun 26, 2017
@smarter smarter mentioned this pull request Jul 4, 2017
Closed
smarter added a commit to dotty-staging/dotty that referenced this pull request Jul 5, 2017
@smarter
Fix scala#2809: Invalidate and unlink symbols for artifact classfiles
ae6e7c6 
This is similar to scala/scala#5952, we do not
need to parse artifact classfiles (that is, classfiles produced by
scalac or dotty that do not contain pickling information), so we discard
them as soon as possible.
This fixes various IDE crashes when trying to complete packages such as
"scala."
smarter added a commit to dotty-staging/dotty that referenced this pull request Jul 5, 2017
@smarter
Fix scala#2809: Invalidate and unlink symbols for artifact classfiles
583e2b5 
This is similar to scala/scala#5952, we do not
need to parse artifact classfiles (that is, classfiles produced by
scalac or dotty that do not contain pickling information), so we discard
them as soon as possible.
This fixes various IDE crashes when trying to complete packages such as
"scala."
smarter added a commit to dotty-staging/dotty that referenced this pull request Jul 5, 2017
@smarter
Fix scala#2809: Invalidate and unlink symbols for artifact classfiles
fe4341b 
This is similar to scala/scala#5952, we do not
need to parse artifact classfiles (that is, classfiles produced by
scalac or dotty that do not contain pickling information), so we discard
them as soon as possible.
This fixes various IDE crashes when trying to complete packages such as
"scala."
smarter added a commit to dotty-staging/dotty that referenced this pull request Jul 5, 2017
@smarter
Fix scala#2809: Invalidate and unlink symbols for artifact classfiles
0fc8632 
This is similar to scala/scala#5952, we do not
need to parse artifact classfiles (that is, classfiles produced by
scalac or dotty that do not contain pickling information), so we discard
them as soon as possible.
This fixes various IDE crashes when trying to complete packages such as
"scala."
smarter added a commit to dotty-staging/dotty that referenced this pull request Jul 6, 2017
@smarter
Fix scala#2809: Invalidate and unlink symbols for artifact classfiles
8aa02c0 
This is similar to scala/scala#5952, we do not
need to parse artifact classfiles (that is, classfiles produced by
scalac or dotty that do not contain pickling information), so we discard
them as soon as possible.
This fixes various IDE crashes when trying to complete packages such as
"scala."
smarter added a commit to dotty-staging/dotty that referenced this pull request Jul 11, 2017
@smarter
Fix scala#2809: Invalidate and unlink symbols for artifact classfiles
07fa44e 
This is similar to scala/scala#5952, we do not
need to parse artifact classfiles (that is, classfiles produced by
scalac or dotty that do not contain pickling information), so we discard
them as soon as possible.
This fixes various IDE crashes when trying to complete packages such as
"scala."
smarter added a commit to dotty-staging/dotty that referenced this pull request Jul 11, 2017
@smarter
Fix scala#2809: Invalidate and unlink symbols for artifact classfiles
77b247d 
This is similar to scala/scala#5952, we do not
need to parse artifact classfiles (that is, classfiles produced by
scalac or dotty that do not contain pickling information), so we discard
them as soon as possible.
This fixes various IDE crashes when trying to complete packages such as
"scala."
@lrytz lrytz deleted the skipScalaRaw branch July 20, 2017 14:32
@retronym
Copy link
Member

retronym commented Oct 30, 2018 •
edited
Loading

I've seen some code in the wild that is broken by this change.

x.isInstanceOf[Enumeration$Val] / classOf[Enumeration$Val] will no longer work. Class.forName("scala.Enumeration$Val") is an alternative.

retronym added a commit to retronym/scala that referenced this pull request Oct 30, 2018
@retronym
Revert code inadvertently backported from scala#5952
f4546a6
retronym added a commit to retronym/scala that referenced this pull request Oct 30, 2018
@retronym
Revert code inadvertently backported from scala#5952
bb201a8
@lrytz
Copy link
Member Author

lrytz commented Nov 1, 2018

Ah, as in x.getClass isAssignableFrom Class.forName("scala.Enumeration$Val"). I guess that's ok.

@ashawley ashawley mentioned this pull request May 3, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@retronym retronym retronym approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.13.0-M2

Development

Successfully merging this pull request may close these issues.

3 participants

@lrytz @retronym @scala-jenkins