Java analysis extraction fails to getCanonicalClassname for nested object
#355
Comments
|
cc @jvican |
|
I'm looking into this as soon as I'm done with the API work. This seems an issue originated from the fact that Zinc is more precise now than it was in X16, and it's exploring a path that causes a well-known behaviour in scalac. I need to diagnose whether:
|
stuhood
changed the title
getCanonicalClassname for nested objectgetCanonicalClassname for nested object
|
I'm having a look at this this week. My strategy is to check if I can reproduce this issue with sbt 1.0.0-RC2, which will involve fixing Twitter util's build. The reason why I want to reproduce with sbt is to be certain that this is an issue that affects both Pants and sbt. If I can reproduce, I'll start having a closer look at why this is happening and what we can do to fix it. |
|
For the record: I repro this on the official X20 release. |
|
@jvican : It should not require fixing Alternatively, as soon as I get pantsbuild/pants#4728 landed I should be able create a repro pants build. |
stuhood
pushed a commit
to pantsbuild/pants
that referenced
this issue
Jul 18, 2017
### Problem Pants is on an older version of zinc (one that does not use class-based name-hashing), and the modern zinc project is moving quickly thanks to @jvican and others. We had previously been on `X7` but it was reverted in #4510 because benchmarks showed that no incremental compilation was happening for scala code. ### Solution * Upgrade to zinc `1.0.0-X20` * Use the zinc `AnalysisMappers` API described on #4513 to make analysis files portable without parsing * Extract options parsing out of the `Settings` object and into its own module, to allow for reuse in multiple binary entrypoints * Refactor and split our zinc wrapper into `zinc-compiler` and `zinc-extractor` to support parsing the `product_deps_by_src` and `classes_by_source` products directly (in order to move toward making analysis a black box) * Switch to usage of new builder-pattern APIs for constructing zinc objects * Remove the `Loggers`/`Reporters` facades in favor of built in support for filtering log messages ### Result The new version of the zinc wrapper correctly supports incremental compile (with the exception of sbt/zinc#355), and the python portions of pants no longer require any internal knowledge of zinc analysis. The python half of this change will remove that code.
jvican
added a commit
to scalacenter/zinc
that referenced
this issue
Jul 20, 2017
jvican
added a commit
to scalacenter/zinc
that referenced
this issue
Jul 20, 2017
jvican
added a commit
to jvican/util
that referenced
this issue
Jul 20, 2017
|
So I haven't been able to reproduce this either in twitter-util with sbt 1.0.0-RC2. Here's the commit that makes the switch: jvican/util@3e925e3. The proof: This bug looks like an issue in the Pants side, but I'm not sure what could be wrong... 😞. |
|
AFAICT, it requires two compilation units: #374 does not contain a java file, so probably won't hit it. Will try to get a repro today. EDIT: Sorry, should have included more info in the original report, but: the specific module that fails to compile is |
|
The following commit, (when compiled with X20) is sufficient to repro: twitter/pants@0693b06 ... ie, one 3rdparty binary jar, and one source file. Should be easy to extract into an sbt project. |
jvican
added a commit
to scalacenter/zinc
that referenced
this issue
Jul 20, 2017
|
scalacenter@13f6fc6 reproduces it now. Thanks for those last pointers and the reproduction case for Pants. |
|
Weird. If the compiled scala file is refactored to the bare minimum: /// COPIED FROM TWITTER UTIL TO IDENTIFY ISSUE WITH MALFORMED NAME //
///////////////////////////////////////////////////////////////////////
package repro
import java.nio.ReadOnlyBufferException
import java.nio.charset.{Charset, StandardCharsets => JChar}
import scala.collection.immutable.VectorBuilder
abstract class Buf { outer =>
@throws(classOf[IllegalArgumentException])
def write(output: Array[Byte], off: Int): Unit
@throws(classOf[IllegalArgumentException])
@throws(classOf[ReadOnlyBufferException])
def write(output: java.nio.ByteBuffer): Unit
def length: Int
def slice(from: Int, until: Int): Buf
def get(index: Int): Byte
def process(from: Int, until: Int, processor: Buf.Processor): Int
protected def unsafeByteArrayBuf: Option[Buf.ByteArray]
}
object Buf {
abstract class Processor {
def apply(byte: Byte): Boolean
}
class ByteArray(
private[Buf] val bytes: Array[Byte],
private[Buf] val begin: Int,
private[Buf] val end: Int)
extends Buf {
def get(index: Int): Byte = ???
def process(from: Int, until: Int, processor: Processor): Int = ???
def write(buf: Array[Byte], off: Int): Unit = ???
def write(buffer: java.nio.ByteBuffer): Unit = ???
def slice(from: Int, until: Int): Buf = ???
def length: Int = end - begin
protected def unsafeByteArrayBuf: Option[Buf.ByteArray] = ???
}
}It does not crash. |
|
EDIT: No, false positive.
private class NoopBuf extends Buf {
def write(buf: Array[Byte], off: Int): Unit =
checkWriteArgs(buf.length, off)
def write(buffer: java.nio.ByteBuffer): Unit = ()
override val isEmpty = true
def length: Int = 0
def slice(from: Int, until: Int): Buf = {
checkSliceArgs(from, until)
this
}
protected def unsafeByteArrayBuf: Option[Buf.ByteArray] = None
def get(index: Int): Byte =
throw new IndexOutOfBoundsException(s"Index out of bounds: $index")
def process(from: Int, until: Int, processor: Processor): Int = {
checkSliceArgs(from, until)
-1
}
} |
jvican
added a commit
to scalacenter/zinc
that referenced
this issue
Jul 24, 2017
I've engineered this minimal test case from sbt#355. It's a well-known Scala issue: scala/bug#2034. I'm still thinking the best way to proceed. I know we can do some ugly workarounds to make this test not fail, but the only thing holding me off is correctness and appreciation (and hope) for an elegant solution. Especifically, if we use some of the workarounds, how are they going to affect Java analysis?
jvican
added a commit
to scalacenter/zinc
that referenced
this issue
Jul 24, 2017
I've engineered this minimal test case from sbt#355. It's a well-known Scala issue: scala/bug#2034. I'm still thinking the best way to proceed. I know we can do some ugly workarounds to make this test not fail, but the only thing holding me off is correctness and appreciation (and hope) for an elegant solution. Especifically, if we use some of the workarounds, how are they going to affect Java analysis?
|
I've engineered a minimal test case that shows the issue in adc2114. The code fails in latest Zinc because of this commit 29571f8, that restored Java analysis after the new analysis mechanism was introduced. I'm working on a fix. This is a legit compiler error, known for years: scala/bug#2034. I am thinking on the best way to solve this issue without affecting correctness. From a cursory analysis, it seems that the current |
jvican
added a commit
to scalacenter/zinc
that referenced
this issue
Jul 24, 2017
The binary name encoding for Scala has been broken for a while. It does not follow the Java source name rules because it does not append a `$` after the mirror class of an object, which already ends in `$`. Zinc works around this fix in the only way it can: it gets the enclosing class, which is obtained from the `InnerClasses` java classfile metadata, and it reconstructs the path from there. It circumvents, by design, the core issue: it doesn't call Java's name parser at all. This issue has been fixed in the JDK9, but unfortunately it's going to be tormenting us for a while because 2.13 does not target Java 9, and we're still at 2.12.x. This is the fix in JDK9: http://bugs.java.com/bugdatabase/view_bug.do?bug_id=8057919 In order to fully fix this issue, the Scala compiler should do the desired behaviour in 2.13.x. 2.12.x is already lost since this fix is binary incompatible. The appropriate JIRA ticket is here: scala/bug#2034.
jvican
added a commit
to scalacenter/zinc
that referenced
this issue
Jul 24, 2017
I've engineered this minimal test case from sbt#355. It's a well-known Scala issue: scala/bug#2034. I'm still thinking the best way to proceed. I know we can do some ugly workarounds to make this test not fail, but the only thing holding me off is correctness and appreciation (and hope) for an elegant solution. Especifically, if we use some of the workarounds, how are they going to affect Java analysis?
jvican
added a commit
to scalacenter/zinc
that referenced
this issue
Jul 24, 2017
The binary name encoding for Scala has been broken for a while. It does not follow the Java source name rules because it does not append a `$` after the mirror class of an object, which already ends in `$`. Zinc works around this fix in the only way it can: it gets the enclosing class, which is obtained from the `InnerClasses` java classfile metadata, and it reconstructs the path from there. It circumvents, by design, the core issue: it doesn't call Java's name parser at all. This issue has been fixed in the JDK9, but unfortunately it's going to be tormenting us for a while because 2.13 does not target Java 9, and we're still at 2.12.x. This is the fix in JDK9: http://bugs.java.com/bugdatabase/view_bug.do?bug_id=8057919 In order to fully fix this issue, the Scala compiler should do the desired behaviour in 2.13.x. 2.12.x is already lost since this fix is binary incompatible. The appropriate JIRA ticket is here: scala/bug#2034.
jvican
added a commit
to scalacenter/zinc
that referenced
this issue
Jul 24, 2017
I've engineered this minimal test case from sbt#355. It's a well-known Scala issue: scala/bug#2034. I'm still thinking the best way to proceed. I know we can do some ugly workarounds to make this test not fail, but the only thing holding me off is correctness and appreciation (and hope) for an elegant solution. Especifically, if we use some of the workarounds, how are they going to affect Java analysis?
jvican
added a commit
to scalacenter/zinc
that referenced
this issue
Jul 24, 2017
The binary name encoding for Scala has been broken for a while. It does not follow the Java source name rules because it does not append a `$` after the mirror class of an object, which already ends in `$`. Zinc works around this fix in the only way it can: it gets the enclosing class, which is obtained from the `InnerClasses` java classfile metadata, and it reconstructs the path from there. It circumvents, by design, the core issue: it doesn't call Java's name parser at all. This issue has been fixed in the JDK9, but unfortunately it's going to be tormenting us for a while because 2.13 does not target Java 9, and we're still at 2.12.x. This is the fix in JDK9: http://bugs.java.com/bugdatabase/view_bug.do?bug_id=8057919 In order to fully fix this issue, the Scala compiler should do the desired behaviour in 2.13.x. 2.12.x is already lost since this fix is binary incompatible. The appropriate JIRA ticket is here: scala/bug#2034.
|
@stuhood I believe scalacenter@134a8ba fixes this thing. I'm asking for feedback on my fix. I think it's the only possible one. If we really want to address this issue, we must fix it in upstream Scala, and that won't happen before 2.13.0. |
jvican
added a commit
to scalacenter/zinc
that referenced
this issue
Jul 24, 2017
The binary name encoding for Scala has been broken for a while. It does not follow the Java source name rules because it does not append a `$` after the mirror class of an object, which already ends in `$`. Zinc works around this fix in the only way it can: it gets the enclosing class, which is obtained from the `InnerClasses` java classfile metadata, and it reconstructs the path from there. It circumvents, by design, the core issue: it doesn't call Java's name parser at all. This issue has been fixed in the JDK9, but unfortunately it's going to be tormenting us for a while because 2.13 does not target Java 9, and we're still at 2.12.x. This is the fix in JDK9: http://bugs.java.com/bugdatabase/view_bug.do?bug_id=8057919 In order to fully fix this issue, the Scala compiler should do the desired behaviour in 2.13.x. 2.12.x is already lost since this fix is binary incompatible. The appropriate JIRA ticket is here: scala/bug#2034.
jvican
added a commit
to scalacenter/zinc
that referenced
this issue
Jul 25, 2017
The binary name encoding for Scala has been broken for a while. It does not follow the Java source name rules because it does not append a `$` after the mirror class of an object, which already ends in `$`. Zinc works around this fix in the only way it can: it gets the enclosing class, which is obtained from the `InnerClasses` java classfile metadata, and it reconstructs the path from there. It circumvents, by design, the core issue: it doesn't call Java's name parser at all. This issue has been fixed in the JDK9, but unfortunately it's going to be tormenting us for a while because 2.13 does not target Java 9, and we're still at 2.12.x. This is the fix in JDK9: http://bugs.java.com/bugdatabase/view_bug.do?bug_id=8057919 In order to fully fix this issue, the Scala compiler should do the desired behaviour in 2.13.x. 2.12.x is already lost since this fix is binary incompatible. The appropriate JIRA ticket is here: scala/bug#2034.
jvican
added a commit
to scalacenter/zinc
that referenced
this issue
Jul 25, 2017
The binary name encoding for Scala has been broken for a while. It does not follow the Java source name rules because it does not append a `$` after the mirror class of an object, which already ends in `$`. Zinc works around this fix in the only way it can: it gets the enclosing class, which is obtained from the `InnerClasses` java classfile metadata, and it reconstructs the path from there. It circumvents, by design, the core issue: it doesn't call Java's name parser at all. This issue has been fixed in the JDK9, but unfortunately it's going to be tormenting us for a while because 2.13 does not target Java 9, and we're still at 2.12.x. This is the fix in JDK9: http://bugs.java.com/bugdatabase/view_bug.do?bug_id=8057919 In order to fully fix this issue, the Scala compiler should do the desired behaviour in 2.13.x. 2.12.x is already lost since this fix is binary incompatible. The appropriate JIRA ticket is here: scala/bug#2034.
dwijnand
added a commit
that referenced
this issue
Jul 25, 2017
Fix #355: Handle malformed class name
|
GitHub is misbehaving, I think. I'm not getting some comments in this repo and this wasn't closed, even though the fix matches their commit format. Closing. |
|
The reason this wasn't auto-closed is because the fix is in the 1.0.0 branch, which isn't the default branch of the repo. |
stuhood
added a commit
to twitter/pants
that referenced
this issue
Jul 28, 2017
stuhood
pushed a commit
to pantsbuild/pants
that referenced
this issue
Jul 28, 2017
### Problem The current version of zinc used in pants suffers from sbt/zinc#355, and thus hasn't been pulled in and released via #4729 ### Solution Bump to zinc 1.0.0-RC3, which pulls in the fix for sbt/zinc#355.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
While attempting to compile Java code that depends on
com.twitter.io.Buf.Composite.Impl(defined here), zinc crashes with a stack like the following:This may be a red herring, because I was working in a branch that combines both #325 and #351 (a prerelease version: 1.0.0-X19-M1)EDIT: Confirm onX20as well, but the location of the crash looks completely unrelated to those changes, so thought it would be worth posting now.The text was updated successfully, but these errors were encountered: