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

fix backend crash: Cannot create ClassBType from non-class symbol; also fix SI-7139 #5482

Merged
merged 10 commits into from Oct 28, 2016

Conversation

Projects
None yet
6 participants
@lrytz
Member

lrytz commented Oct 26, 2016

Frontend fixes for scala/scala-dev#248

See individual commit messages.

lrytz added some commits Oct 26, 2016

Ensure companionClass returns a class, not a type alias
This fixes scala/scala-dev#248, where a type alias reached the backend
through this method.

This is very similar to the fix for SI-5031, which changed it only in
ModuleSymbol, but not in Symbol.

The override in ModuleSymbol is actually unnecessary (it's identical),
so it's removed in this commit. It was added for unclear reasons in
296b706.
Don't follow type aliases in getClassByName and friends
This makes getClassByName fail / getClassIfDefined return NoSymbol
when querying an alias.

The current behavior can confuse the classfile parser: when parsing a
class, a cross-check verifies that `pool.getClassSymbol(nameIdx)`
returns the symbol of the class currently being parsed. If there's a
type alias that shadows the linked class, following the alias would
return an unrelated class.

(The cross-check didn't fail because there are some other guards
around it)

The logic to follow aliases was was added in ff98878, without a clear
explanation. Note that `requiredClass[C]` works if `C` is an alias, it
is expanded by the compiler.
@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Oct 26, 2016

Member

Awesome, this looks to fix https://issues.scala-lang.org/browse/SI-7139!

% cat sandbox/test.scala
package test {
  object A
}
package object test {
  type A = Int
}

% scalac sandbox/test.scala && printf ':silent\ndef bug = null.asInstanceOf[test.A.type]\ndef bug = null.asInstanceOf[test.A.type]\n' | scala
Welcome to Scala 2.11.8 (OpenJDK 64-Bit Server VM, Java 1.8.0_112-release).
Type in expressions for evaluation. Or try :help.

scala> :silent

scala> def bug = null.asInstanceOf[test.A.type]

scala> def bug = null.asInstanceOf[test.A.type]
<console>:11: error: type mismatch;
 found   : test.A.type
 required: AnyRef
Note that A extends Any, not AnyRef.
Such types can participate in value classes, but instances
cannot appear in singleton types or in reference comparisons.
       def bug = null.asInstanceOf[test.A.type]
                                        ^

scala> :quit

% qscalac sandbox/test.scala && printf ':silent\ndef bug = null.asInstanceOf[test.A.type]\ndef bug = null.asInstanceOf[test.A.type]\n' | qscala
Welcome to Scala 2.12.0-20161026-205457-37b3df3 (OpenJDK 64-Bit Server VM, Java 1.8.0_112-release).
Type in expressions for evaluation. Or try :help.

scala> :silent

scala> def bug = null.asInstanceOf[test.A.type]

scala> def bug = null.asInstanceOf[test.A.type]

scala> :quit
Member

retronym commented Oct 26, 2016

Awesome, this looks to fix https://issues.scala-lang.org/browse/SI-7139!

% cat sandbox/test.scala
package test {
  object A
}
package object test {
  type A = Int
}

% scalac sandbox/test.scala && printf ':silent\ndef bug = null.asInstanceOf[test.A.type]\ndef bug = null.asInstanceOf[test.A.type]\n' | scala
Welcome to Scala 2.11.8 (OpenJDK 64-Bit Server VM, Java 1.8.0_112-release).
Type in expressions for evaluation. Or try :help.

scala> :silent

scala> def bug = null.asInstanceOf[test.A.type]

scala> def bug = null.asInstanceOf[test.A.type]
<console>:11: error: type mismatch;
 found   : test.A.type
 required: AnyRef
Note that A extends Any, not AnyRef.
Such types can participate in value classes, but instances
cannot appear in singleton types or in reference comparisons.
       def bug = null.asInstanceOf[test.A.type]
                                        ^

scala> :quit

% qscalac sandbox/test.scala && printf ':silent\ndef bug = null.asInstanceOf[test.A.type]\ndef bug = null.asInstanceOf[test.A.type]\n' | qscala
Welcome to Scala 2.12.0-20161026-205457-37b3df3 (OpenJDK 64-Bit Server VM, Java 1.8.0_112-release).
Type in expressions for evaluation. Or try :help.

scala> :silent

scala> def bug = null.asInstanceOf[test.A.type]

scala> def bug = null.asInstanceOf[test.A.type]

scala> :quit
Show outdated Hide outdated src/reflect/scala/reflect/internal/pickling/UnPickler.scala
case ex: IOException =>
throw ex
case ex: MissingRequirementError =>
throw ex
case ex: Throwable =>

This comment has been minimized.

@soc

soc Oct 27, 2016

Member

Maybe ex: NonFatal instead?

@soc

soc Oct 27, 2016

Member

Maybe ex: NonFatal instead?

This comment has been minimized.

@lrytz

lrytz Oct 27, 2016

Member

that's reasonable, will do

@lrytz

lrytz Oct 27, 2016

Member

that's reasonable, will do

sym = owner.newClass(ss.toTypeName) setInfoAndEnter completer
debuglog("loaded "+sym+" from file "+file)
sym
}

This comment has been minimized.

@soc

soc Oct 27, 2016

Member

This is a nice cleanup! :-)

@soc

soc Oct 27, 2016

Member

This is a nice cleanup! :-)

lrytz added some commits Oct 26, 2016

Clean up lookup class by name in the classfile parser
There was a piece of logic essentially duplicating getClassByName
in Mirrors (split up a fully qualified class name by ".", look up
pieces). That piece of code was added in 0ce0ad5 to fix one example in
SI-2464.

However, since 020053c (2012, 2.10) that code was broken: the line

    ss = name.subName(0, start)

should be

    ss = name.subName(start, name.length).toTypeName

As a result, the code would always create a stub symbol. Returning a
stub seems to be the right thing to do anyway, and the fact that we were
doing so during two major releases is a good proof.
Clean up cross-check in classfile parser, remove unnecessary assignment
One of the first entries in the classfile is the class name. The
classfile parser performs a cross-check by looking up the class sybmol
corresponding to that name and ensures it's the same as `clazz`, the
class symbol that the parser currently populates.

Since 322c980 ("Another massive IDE checkin"), if at the time of the
check `clazz` but the lookup returns some class, the `clazz` field is
assigned.

The commit following this one makes sure `clazz` is never NoSymbol, so
the assignment can safely be removed.
Classfile parser and unpickler require class and module symbol arguments
In SymbolLoaders, when seeing a classfile `Foo.class`, we always
(unconditionally) create 3 symbols: a class, a module and a module
class. Some symbols get invalidated later (`.exists`).

Until now, the classfile parser (and unpickler) received the "root"
symbol as argument, which is the symbol whose type is being completed.
This is either the class symbol or the module symbol.

The classfile parser would then try to lookup the other symbol through
`root.companionClass` or `root.companionModule`. Howver, this lookup can
fail. One example is scala-dev#248: when a type alias (in a package
object) shadows a class symbol, `companionClass` will fail.

The implementations of the  classfile parser / unpickler assume that
both the `clazz` and the `staticModule` symbols are available. This
change makes sure that they are always passed in explicitly.

Before this patch, in the example of scala-dev#248, the `classRoot` of
the unpickler was NoSymbol. This caused a bug when unpickling the
module class symbol, causing a second module class symbol to be created
mistakingly. The next commit cleans up this logic, more details there.
This second symbol would then cause the crash in the backend because it
doesn't have an `associatedFile`, therefore `isCoDefinedWith` would
spuriously return `true`.
Robustly identify unpickling the current module class
When unpickling a class, if the name and owner matches the current
`classRoot` of the unpickling Scan, that `classRoot` symbol is used
instead of creating a new symbol.

If, in addition, the class being unpickled has the MODULE flag, the
unpickler should use the `moduleRoot.moduleClass` symbol (instead of
creating a new one).

To identify the module class, the current implementation compares the
name and owner to the `classRoot`. This fails in case the `classRoot`
is `NoSymbol`, which can happen in corner cases (when a type alias
shadows a class symbol, scala-dev#248).

In this patch we identify the module class by comparing the name and
owner to the `moduleRoot` symbol directly (using a `toTypeName`).
@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Oct 27, 2016

Member

added a test case for SI-7139. all commits are green, so review by @retronym. maybe also @adriaanm, as this touches some low-level internals.

Member

lrytz commented Oct 27, 2016

added a test case for SI-7139. all commits are green, so review by @retronym. maybe also @adriaanm, as this touches some low-level internals.

@lrytz lrytz removed the WIP label Oct 27, 2016

@adriaanm

Really good stuff! I have a few small suggestions for locking this down a bit more while we're at it.

* This is intended as a hook allowing to support loading symbols from
* files other than .class files.
*/
protected def newClassLoader(bin: AbstractFile): SymbolLoader =

This comment has been minimized.

@adriaanm

adriaanm Oct 28, 2016

Member

I verified this hook (re-introduced by #2963) is no longer used by scala-js (since scala-js/scala-js@8d85779)

@adriaanm

adriaanm Oct 28, 2016

Member

I verified this hook (re-introduced by #2963) is no longer used by scala-js (since scala-js/scala-js@8d85779)

if (isTopLevel) {
if (c != clazz) {
if ((clazz eq NoSymbol) && (c ne NoSymbol)) clazz = c

This comment has been minimized.

@adriaanm

adriaanm Oct 28, 2016

Member

Is there any scenario in e.g. presentation/interactive mode where we needed this? /cc @dragos

@adriaanm

adriaanm Oct 28, 2016

Member

Is there any scenario in e.g. presentation/interactive mode where we needed this? /cc @dragos

This comment has been minimized.

@dragos

dragos Oct 28, 2016

Contributor

No, I don't think so. This looks more like defensive coding. I don't know if there's any scenario in which pool.getClassSymbol returns NoSymbol, but that seems to be the only purpose of this call. Maybe the new logic involving StubSymbol subsumes it?

@dragos

dragos Oct 28, 2016

Contributor

No, I don't think so. This looks more like defensive coding. I don't know if there's any scenario in which pool.getClassSymbol returns NoSymbol, but that seems to be the only purpose of this call. Maybe the new logic involving StubSymbol subsumes it?

This comment has been minimized.

@lrytz

lrytz Oct 28, 2016

Member

With this PR, clazz cannot ever be NoSymbol.

@lrytz

lrytz Oct 28, 2016

Member

With this PR, clazz cannot ever be NoSymbol.

This comment has been minimized.

@adriaanm

adriaanm Oct 28, 2016

Member

Ok, thanks.

@adriaanm

adriaanm Oct 28, 2016

Member

Ok, thanks.

Show outdated Hide outdated src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala
clazz setInfo completer
enterIfNew(owner, clazz, completer)
}
def newModule(owner: Symbol, name: String): Symbol = owner.newModule(newTermName(name))

This comment has been minimized.

@adriaanm

adriaanm Oct 28, 2016

Member

: ModuleSymbol as the result? (Allows tightening enterClassAndModule and ClassfileLoader).

@adriaanm

adriaanm Oct 28, 2016

Member

: ModuleSymbol as the result? (Allows tightening enterClassAndModule and ClassfileLoader).

This comment has been minimized.

@adriaanm

adriaanm Oct 28, 2016

Member

Implemented in 5c7b8ed

@adriaanm

adriaanm Oct 28, 2016

Member

Implemented in 5c7b8ed

This comment has been minimized.

@lrytz

lrytz Oct 28, 2016

Member

ok, done.

@lrytz

lrytz Oct 28, 2016

Member

ok, done.

Show outdated Hide outdated src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala
def enterClassAndModule(root: Symbol, name: String, completer: SymbolLoader) {
val clazz = enterClass(root, name, completer)
val module = enterModule(root, name, completer)
def enterClassAndModule(root: Symbol, name: String, getCompleter: (Symbol, Symbol) => SymbolLoader) {

This comment has been minimized.

@adriaanm

adriaanm Oct 28, 2016

Member

(Symbol, ModuleSymbol) => SymbolLoader seems within reach to avoid confusion in the future.

@adriaanm

adriaanm Oct 28, 2016

Member

(Symbol, ModuleSymbol) => SymbolLoader seems within reach to avoid confusion in the future.

Show outdated Hide outdated src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala
@@ -277,7 +281,7 @@ abstract class SymbolLoaders {
}
}
class ClassfileLoader(val classfile: AbstractFile) extends SymbolLoader with FlagAssigningCompleter {
class ClassfileLoader(val classfile: AbstractFile, clazz: Symbol, module: Symbol) extends SymbolLoader with FlagAssigningCompleter {

This comment has been minimized.

@adriaanm

adriaanm Oct 28, 2016

Member

module: ModuleSymbol

@adriaanm

adriaanm Oct 28, 2016

Member

module: ModuleSymbol

Show outdated Hide outdated src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala
def parse(file: AbstractFile, root: Symbol): Unit = {
debuglog("[class] >> " + root.fullName)
def parse(file: AbstractFile, clazz: Symbol, module: Symbol): Unit = {

This comment has been minimized.

@adriaanm

adriaanm Oct 28, 2016

Member

module: ModuleSymbol

@adriaanm

adriaanm Oct 28, 2016

Member

module: ModuleSymbol

This comment has been minimized.

@dragos

dragos Oct 28, 2016

Contributor

This allows callers to pass unrelated symbols. Maybe some warning in a comment is in order (I assume calls to companionModule & friends are also "forbidden" inside this method). If so, also worth a comment.

@dragos

dragos Oct 28, 2016

Contributor

This allows callers to pass unrelated symbols. Maybe some warning in a comment is in order (I assume calls to companionModule & friends are also "forbidden" inside this method). If so, also worth a comment.

Show outdated Hide outdated src/compiler/scala/tools/nsc/symtab/SymbolLoaders.scala
@@ -309,7 +313,7 @@ abstract class SymbolLoaders {
// errors. More concretely, the classfile parser calls "sym.companionModule", which calls
// "isModuleNotMethod" on the companion. After refchecks, this method forces the info, which
// may run the classfile parser. This produces the error.
enteringPhase(phaseBeforeRefchecks)(classfileParser.parse(classfile, root))
enteringPhase(phaseBeforeRefchecks)(classfileParser.parse(classfile, clazz, module))

This comment has been minimized.

@dragos

dragos Oct 28, 2016

Contributor

The comment seems outdated, the classfile parser doesn't call companionModule anymore. Maybe there's no need to call enteringPhase either?

@dragos

dragos Oct 28, 2016

Contributor

The comment seems outdated, the classfile parser doesn't call companionModule anymore. Maybe there's no need to call enteringPhase either?

This comment has been minimized.

@lrytz

lrytz Oct 28, 2016

Member

Hmm.. I think it's a bit risky to remove the phase travel here, the classfile parser and unpickler call various methods on symbols. I'll update the comment.

@lrytz

lrytz Oct 28, 2016

Member

Hmm.. I think it's a bit risky to remove the phase travel here, the classfile parser and unpickler call various methods on symbols. I'll update the comment.

This comment has been minimized.

@lrytz

lrytz Oct 28, 2016

Member

Though I see it was added not too long ago: 0ccdb15

@lrytz

lrytz Oct 28, 2016

Member

Though I see it was added not too long ago: 0ccdb15

This comment has been minimized.

@lrytz

lrytz Oct 28, 2016

Member

I'll give it a try.

@lrytz

lrytz Oct 28, 2016

Member

I'll give it a try.

lrytz added some commits Oct 28, 2016

For scala classfiles, only parse the scala signature annotation
Skipping other annotations not only saves some cycles / GC, but also
prevents some spurious warnings / errors related to cyclic dependencies
when parsing annotation arguments refering to members of the class.
Address review comments
Tighten some types (Symbol -> ClassSymbol / ModuleSymbol), use NonFatal
instead of catching Throwable.

Also don't run the classfile parser enteringPhase(phaseBeforeRefchecks)
anymore. This was added in 0ccdb15 but seems no longer required.
@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Oct 28, 2016

Member

LGTM!

Member

adriaanm commented Oct 28, 2016

LGTM!

@lrytz lrytz merged commit 0666377 into scala:2.12.x Oct 28, 2016

6 checks passed

cla @lrytz signed the Scala CLA. Thanks!
Details
combined All previous commits successful.
integrate-ide [3425] SUCCESS. Took 5 s.
Details
validate-main [3953] SUCCESS. Took 69 min.
Details
validate-publish-core [3823] SUCCESS. Took 15 min.
Details
validate-test [3334] SUCCESS. Took 53 min.
Details

@adriaanm adriaanm modified the milestone: 2.12.1 Oct 29, 2016

@SethTisue SethTisue changed the title from Frontend fixes for scala-dev#248 to 2.12 regression, backend crash: Cannot create ClassBType from non-class symbol Dec 5, 2016

@SethTisue SethTisue changed the title from 2.12 regression, backend crash: Cannot create ClassBType from non-class symbol to fix 2.12 regression, backend crash: Cannot create ClassBType from non-class symbol Dec 5, 2016

@SethTisue SethTisue changed the title from fix 2.12 regression, backend crash: Cannot create ClassBType from non-class symbol to fix 2.12 regression, backend crash: Cannot create ClassBType from non-class symbol; also fix SI-7139 Dec 5, 2016

@SethTisue SethTisue changed the title from fix 2.12 regression, backend crash: Cannot create ClassBType from non-class symbol; also fix SI-7139 to fix backend crash: Cannot create ClassBType from non-class symbol; also fix SI-7139 Dec 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment