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

Fix #13523: Survive missing Java inner annotation classfiles #15094

Merged
merged 1 commit into from
May 3, 2022

Conversation

griggt
Copy link
Contributor

@griggt griggt commented May 3, 2022

Co-authored-by: Seth Tisue seth@tisue.net

Fixes #13523

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.

So this is really Tom's fix and I served as reviewer / sounding board. We looked at Guillaume's previous fix together, and then he walked me through this fix, and we discussed it from every angle either of us could think of. Conclusion: LGTM 👍

(Minor point, I did suggest that the test be in a package rather than in the empty package, since the empty package is a special case, and to be sure that it still worked in the presence of a package prefix.)

Might be good to have one more reviewer, probably @smarter since this is so closely related to his previous fix (#9096).

@SethTisue SethTisue requested a review from smarter May 3, 2022 16:28
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM, thanks!

val result = atPhase(typerPhase)(getMember(owner, innerName.toTypeName))
val result = owner.denot.infoOrCompleter match
case _: StubInfo if hasAnnotation(entry.jflags) =>
requiredClass(innerName.toTypeName)
Copy link
Member

Choose a reason for hiding this comment

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

Since this is not obvious, could you add a comment explaining why this is needed and pointing the reader to the test case?

Co-authored-by: Seth Tisue <seth@tisue.net>
@griggt griggt enabled auto-merge May 3, 2022 17:42
@griggt griggt merged commit aff1e11 into scala:main May 3, 2022
@griggt griggt deleted the fix-13523 branch May 3, 2022 18:46
@Kordyjan Kordyjan added this to the 3.2.0 milestone Aug 1, 2023
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.

Bad symbolic reference when a java classfile refers to an annotation inner class not on the classpath
4 participants