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.

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

error when reading class file with unknown newer jdk version #18618

Merged
merged 2 commits into from
Nov 3, 2023

Conversation

bishabosha
Copy link
Member

@bishabosha bishabosha commented Sep 29, 2023

when reading a classfile causes a runtime exception, report the version of the classfile, and request that the user checks JDK compatibility.

Considerations:

  • should we test this?

currently we have no process for testing latest JDK, currently we still only test with JDK 16 and 8

fixes #18573

Release Notes

If there is an error reading a class file, we now report the version of the classfile, and a recommendation to check JDK compatibility:

error while loading String,
  class file /modules/java.base/java/lang/String.class is broken (version 65.0),
  please check the JDK compatibility of your Scala version (3.4.0-RC1-bin-SNAPSHOT-nonbootstrapped),
  reading aborted with class java.lang.RuntimeException:
  foo bar

This will improve the user experience for when the class file format changes in an unexpected way (for example in the JDK 21 release, which is now supported by Scala 3.3.1)

@bishabosha bishabosha requested review from sjrd and Kordyjan September 29, 2023 11:20
@bishabosha bishabosha added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Sep 29, 2023
@sjrd
Copy link
Member

sjrd commented Oct 4, 2023

Can we somehow only report that clear error message if there is actually an exception thrown while reading the class file?

What does Scala 2 do in this situation?

@bishabosha
Copy link
Member Author

What does Scala 2 do in this situation?

From what I see there is no capturing of the class file version, and all runtime exceptions will be wrapped in the same "class file 'foo' is broken" error as dotty.

@lrytz @SethTisue what do you think?

I also think it would be ideal to not pre-emptively error, but append an addendum to any error that might occur.

s"class file '${classfile}' has unknown version $majorVersion.$minorVersion, should be at least $JAVA_MAJOR_VERSION.$JAVA_MINOR_VERSION")
if majorVersion > JAVA_LATEST_MAJOR_VERSION then
throw new IOException(
s"class file '${classfile}' has unknown version $majorVersion.$minorVersion, and was compiled by a newer JDK than supported by this Scala version, please update to a newer Scala version.")
Copy link
Member Author

@bishabosha bishabosha Oct 4, 2023

Choose a reason for hiding this comment

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

We could also put in the error message which is the latest classfile major supported, and should we reverse map that to the corresponding JDK version? (more work for @Kordyjan release procedure)

@lrytz
Copy link
Member

lrytz commented Oct 4, 2023

Can you reproduce the error described in the ticket? It seems to work for me.

➜ sandbox sc3 -version
Scala compiler version 3.3.1 -- Copyright 2002-2023, LAMP/EPFL
➜ sandbox java -version
openjdk version "21" 2023-09-19
OpenJDK Runtime Environment (build 21+35-2513)
OpenJDK 64-Bit Server VM (build 21+35-2513, mixed mode, sharing)
➜ sandbox cat main.scala
import java.nio.file.StandardOpenOption

@main def Test = {
  val x =  StandardOpenOption.CREATE
}
➜ sandbox sc3 main.scala

@bishabosha
Copy link
Member Author

bishabosha commented Oct 4, 2023

Can you reproduce the error described in the ticket? It seems to work for me.

3.3.1 is patched for jdk 21, it should break reliably with 3.3.0, e.g.

$> cs launch org.scala-lang:scala3-compiler_3:3.3.0  --jvm zulu:21 -M dotty.tools.MainGenericCompiler -- -usejavacp foo.scala
error while loading StandardOpenOption,
class file /modules/java.base/java/nio/file/StandardOpenOption.class is broken, reading aborted with class java.lang.RuntimeException
bad constant pool index: 0 at pos: 1200
-- [E008] Not Found Error: foo.scala:4:30 --------------------------------------
4 |  val x =  StandardOpenOption.CREATE
  |           ^^^^^^^^^^^^^^^^^^^^^^^^^
  |   value CREATE is not a member of object java.nio.file.StandardOpenOption
2 errors found

@lrytz
Copy link
Member

lrytz commented Oct 4, 2023

👍 thanks

Older 2.13 crashes also with no user friendly information. In the past years this happened very rarely - maybe it will become more frequent though as the pace of changes in the JVM is increasing. Still, I think it would be better not to have a hard error based on the classfile version and instead provide better message to the user on crashes. We can avoid giving the user an error when it works, which seems to be the common case. Also we don't need to keep updating the version number as Java releases come out.

➜ sandbox scv 2.13.10 A.scala
error:
  bad constant pool index: 0 at pos: 48445
     while compiling: <no file>
        during phase: globalPhase=<no phase>, enteringPhase=<some phase>
     library version: version 2.13.10
    compiler version: version 2.13.10
  reconstructed args:

  last tree to typer: EmptyTree
       tree position: <unknown>
            tree tpe: <notype>
              symbol: null
           call site: <none> in <none>

== Source file context for tree position ==

error: scala.reflect.internal.FatalError:
  bad constant pool index: 0 at pos: 48445
     while compiling: <no file>
        during phase: globalPhase=<no phase>, enteringPhase=<some phase>
     library version: version 2.13.10
    compiler version: version 2.13.10
  reconstructed args:

  last tree to typer: EmptyTree
       tree position: <unknown>
            tree tpe: <notype>
              symbol: null
           call site: <none> in <none>

== Source file context for tree position ==


	at scala.reflect.internal.Reporting.abort(Reporting.scala:69)
	at scala.reflect.internal.Reporting.abort$(Reporting.scala:65)
	at scala.reflect.internal.SymbolTable.abort(SymbolTable.scala:28)
	at scala.tools.nsc.symtab.classfile.ClassfileParser$ConstantPool.errorBadIndex(ClassfileParser.scala:408)
	at scala.tools.nsc.symtab.classfile.ClassfileParser$ConstantPool.getExternalName(ClassfileParser.scala:263)

@som-snytt
Copy link
Contributor

This happens to me frequently, during casual testing. Spurious version mismatches, including tasty format of stale files I don't care about during casual testing, are a mild annoyance. Thanks for any mitigation or amelioration.

@bishabosha
Copy link
Member Author

bishabosha commented Oct 5, 2023

Also we don't need to keep updating the version number as Java releases come out

In this case we can still say "class file 'foo' is broken... the class file version was 65.0, please check the JDK compatibility of your Scala version (3.x.y)."

@Kordyjan Kordyjan removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Oct 10, 2023
Copy link
Member

@SethTisue SethTisue left a comment

Choose a reason for hiding this comment

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

Kordyjan removed the backport:nominated label last week

Curious why? I would have thought this was a strong candidate for backporting.

My observation on Discord and other community forums is that yes, this definitely actually happens to users quite often, and the users are often newcomers trying to run Scala for the first time. These newcomers often simply do not share our veterans’ awareness that it's important to aggressively update compiler and build tool versions if you want to use a recent JDK. It's rather common that their package manager gives them a brand new JDK but the learning materials they are using specify older Scala and/or sbt versions.

As for the PR in general, it looks good to me, and I agree with Lukas's remarks.

@bishabosha
Copy link
Member Author

bishabosha commented Oct 17, 2023

Curious why? I would have thought this was a strong candidate for backporting.

I would guess because it makes a hard error when none would have existed before, so far LTS compatibility is basically "must be identical unless a flag is used"

I think its pretty clear from the comments that instead we should passively track the version, then report it whenever there is the "broken" class file error - which would match the strict LTS compatibility rules

@bishabosha bishabosha requested a review from SethTisue October 31, 2023 17:20
@bishabosha
Copy link
Member Author

bishabosha commented Oct 31, 2023

@SethTisue with the latest change, we only report the JDK version along with the broken version message, this will produce something like:

error while loading String,
  class file /modules/java.base/java/lang/String.class is broken (version 65.0),
  please check the JDK compatibility of your Scala version (3.4.0-RC1-bin-SNAPSHOT-nonbootstrapped),
  reading aborted with class java.lang.RuntimeException:
  foo bar

and with this change it is compatible with LTS

@bishabosha bishabosha added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Oct 31, 2023
@bishabosha
Copy link
Member Author

rebased

@bishabosha bishabosha merged commit 8c602b3 into scala:main Nov 3, 2023
18 checks passed
@bishabosha bishabosha deleted the warn-unknown-jdk branch November 3, 2023 13:25
@bishabosha bishabosha added the release-notes Should be mentioned in the release notes label Nov 8, 2023
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
WojciechMazur added a commit that referenced this pull request Jul 1, 2024
…n" to LTS (#20862)

Backports #18618 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
@WojciechMazur WojciechMazur removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler should provide better error messages when using unsupported JDK version
7 participants