Skip to content

Thoroughly typecheck Java sources for pickling#8692

Merged
retronym merged 1 commit intoscala:2.12.xfrom
hrhino:topic/pickle-java-crash
Feb 4, 2020
Merged

Thoroughly typecheck Java sources for pickling#8692
retronym merged 1 commit intoscala:2.12.xfrom
hrhino:topic/pickle-java-crash

Conversation

@hrhino
Copy link
Contributor

@hrhino hrhino commented Feb 3, 2020

Unlike .scala files, .java files are named and typed lazily: they will only be analyzed when necessary to typecheck a Scala source. However, under -Ypickle-java, pickler runs over them after typechecking, and can force infos that otherwise wouldn't be forced. If typing the Java source fails, the tree can contain erroneous types and symbols which will then crash pickler as it attempts to write out the pickle.

This patch makes -Ypickle-java eagerly typecheck Java sources during typer, so any errors will be emitted alongside Scala typer errors.

The enclosed test case previously crashed the compiler with a MatchError on ErrorType.

@hrhino hrhino requested a review from retronym February 3, 2020 23:12
@scala-jenkins scala-jenkins added this to the 2.12.12 milestone Feb 3, 2020
Unlike .scala files, .java files are named and typed lazily: they will
only be analyzed when necessary to typecheck a Scala source. However,
under -Ypickle-java, pickler runs over them after typechecking, and can
force infos that otherwise wouldn't be forced. If typing the Java source
fails, the tree can contain erroneous types and symbols which will then
crash pickler as it attempts to write out the pickle.

This patch makes -Ypickle-java eagerly typecheck Java sources during
typer, so any errors will be emitted alongside Scala typer errors.

The enclosed test case previously crashed the compiler with a MatchError
on ErrorType.
@hrhino hrhino force-pushed the topic/pickle-java-crash branch from 85dfec5 to 4099b6b Compare February 3, 2020 23:13
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.

Seems like a straight forward solution. The other possiblity would be to check for reporter.hasErrors after calling sym.info in putSymbol and bailing out of that phase, but that seems more fragile.

@hrhino
Copy link
Contributor Author

hrhino commented Feb 4, 2020

Thanks. That was my first go but then you have to bail on the first error (or buffer them somehow) whereas this hopefully lets you get all of the Java errors that we can detect

@retronym retronym merged commit f133c16 into scala:2.12.x Feb 4, 2020
@SethTisue SethTisue modified the milestones: 2.12.12, 2.12.11 Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants