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

Improve error message for mismatched tasty versions, allow configuration of header unpickler #18828

Merged
merged 7 commits into from
Nov 3, 2023

Conversation

bishabosha
Copy link
Member

@bishabosha bishabosha commented Nov 2, 2023

fixes #18427

Adds configuration ability to TastyHeaderUnpickler, why? tasty-core is intended to be a generic library, so for its error messages to suddenly assume the consumer is a scala compiler would be a breaking change, so we instead by default use a generic configuration (the old "tooling" style) and allow to plug-in a "scala compiler" configuration

Also the configuration allows us to easily test the content of error messages.

see #18828 (comment) for backporting considerations

Release Notes

Scala 3.4 improves the error message when a signature from the classpath comes from an incompatible TASTy version. It has improved readability and provides a more useful diagnostic of what a user can do to fix the problem.

Sample error 1 (reading old experimental tasty):

error while loading Tuple,
  TASTy file /scala/Tuple.tasty could not be read, failing with:
  Backward incompatible TASTy file has version 28.4-experimental-1, produced by Scala 3.3.3-RC1-NIGHTLY,
  expected stable TASTy from 28.0 to 28.3, or exactly 28.4-experimental-2.
  The source of this file should be recompiled by the same nightly or snapshot Scala 3.3 compiler.
  Usually this means that the library dependency containing this file should be updated.
  Please refer to the documentation for information on TASTy versioning:
  https://docs.scala-lang.org/scala3/reference/language-versions/binary-compatibility.html

Sample error 2 (reading newer stable scala):

error while loading Tuple,
  TASTy file /scala/Tuple.tasty could not be read, failing with:
  Forward incompatible TASTy file has version 28.3, produced by Scala 3.3.1,
  expected stable TASTy from 28.0 to 28.2.
  To read this TASTy file, use a Scala 3.3.0 compiler or newer.
  Please refer to the documentation for information on TASTy versioning:
  https://docs.scala-lang.org/scala3/reference/language-versions/binary-compatibility.html

@bishabosha
Copy link
Member Author

bishabosha commented Nov 2, 2023

it would be good to backport the content of the messages, however the TastyHeaderUnpickler changes would break the semantic versioning forward compatibility of tasty-core, so I propose that for a backporting in LTS line the tasty-core module is left alone, and we should duplicate the improved TastyHeaderUnpickler into the compiler module (thanks to @sjrd for the suggestion)

@bishabosha bishabosha added this to the 3.4.0 milestone Nov 2, 2023
Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Great tests!

compiler/src/dotty/tools/dotc/typer/Typer.scala Outdated Show resolved Hide resolved
i"""TASTy file ${tastyFile.canonicalPath} is broken, reading aborted with ${e.getClass}
| ${Option(e.getMessage).getOrElse("")}"""
if (ctx.debug) e.printStackTrace()
throw IOException(message)
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep the cause, here:

Suggested change
throw IOException(message)
throw IOException(message, e)

Copy link
Member Author

Choose a reason for hiding this comment

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

ClassfileParser also does not keep the cause

Copy link
Member

Choose a reason for hiding this comment

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

Meh ... OK.

classRoot.classSymbol.rootTreeOrProvider = unpickler
moduleRoot.classSymbol.rootTreeOrProvider = unpickler
checkTastyUUID(tastyFile, tastyBytes)
catch case e: RuntimeException =>
Copy link
Member

Choose a reason for hiding this comment

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

Why RuntimeException and not Exception or NonFatal?

Copy link
Member Author

@bishabosha bishabosha Nov 2, 2023

Choose a reason for hiding this comment

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

its what the ClassfileParser does, i.e. what TASTy unpicking used to do before we did the TASTy-first classpath change

tasty/src/dotty/tools/tasty/TastyHeaderUnpickler.scala Outdated Show resolved Hide resolved
tasty/src/dotty/tools/tasty/TastyHeaderUnpickler.scala Outdated Show resolved Hide resolved
Comment on lines +164 to +138
// - backwards incompatible major, in which case the library should be recompiled by the minimum stable minor
// version supported by this compiler
// - any experimental in an older minor, in which case the library should be recompiled by the stable
// compiler in the same minor.
// - older experimental in the same minor, in which case the compiler is also experimental, and the library
// should be recompiled by the current compiler
// - forward incompatible, in which case the compiler must be upgraded to the same version as the file.
Copy link
Member

Choose a reason for hiding this comment

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

💯

@bishabosha bishabosha force-pushed the change-message-unknown-tasty branch from c73c612 to 88dd1ca Compare November 2, 2023 17:03
@bishabosha bishabosha requested a review from SethTisue November 2, 2023 17:07
@bishabosha bishabosha added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Nov 2, 2023
@bishabosha
Copy link
Member Author

@sjrd I tweaked the first line of the message because it was repetitive when combined with the additional text added in SymbolLoaders when reporting the error.

@sjrd
Copy link
Member

sjrd commented Nov 3, 2023

@bishabosha Do you want a review by someone else? If yes can you assign them, please?

@sjrd sjrd assigned bishabosha and unassigned sjrd Nov 3, 2023
@bishabosha bishabosha removed their assignment Nov 3, 2023
@bishabosha
Copy link
Member Author

I was wondering if this might be controversial

@bishabosha bishabosha merged commit a18ff54 into scala:main Nov 3, 2023
18 checks passed
@bishabosha bishabosha deleted the change-message-unknown-tasty branch November 3, 2023 16:20
@bishabosha bishabosha added the release-notes Should be mentioned in the release notes label Nov 8, 2023
@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.

unhelpful error when TASTy version is not compatible
3 participants