Add guardrails to prevent and correct wrong output from JavaTypeCorrect #339
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… try-with-resources
… try-with-resources
kelloggm
changed the title
theron-wang
approved these changes
Jul 29, 2024
| @@ -104,6 +107,24 @@ public Map<String, String> getClassAndUnresolvedInterface() { | |||
| * @return the map described above. | |||
| */ | |||
| public Map<String, String> getExtendedTypes() { | |||
| // Before returning, purge any entries that are obviously bad according to | |||
| // the following simple heuristic(s): | |||
| // * don't extend known-final classes from the JDK, like java.lang.String. | |||
There was a problem hiding this comment.
This comment sounds like java.lang.String isn't the only known-final class in the JDK, but the code below only includes that case. Are there supposed to be more or should this comment be adjusted for clarity?
There was a problem hiding this comment.
I thought we might want to add more in the future, so I future-proofed the comment. Your point is a good one, though - what we should do is look up a list, and then add that to JavaLangUtils. I'll do that now.
theron-wang
approved these changes
Jul 29, 2024
src/main/java/org/checkerframework/specimin/JavaTypeCorrect.java
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Incorporates #338, so don't review until that is merged.I'm not sure if we want to merge this. It avoids some bad behavior on the NA-176 test, and we could probably expand it in the future for other, similar kinds of problems. However, it definitely does not address the root cause: why is Specimin putting itself in a situation where javac insists that a synthetic class must be compatible with String?I'm going to continue my investigation, but I'm leaving this PR here because I may want to merge it later if the fix for the root cause is too complex and/or impracticle.This PR makes a series of changes to how our javac type correction algorithm (in the
JavaTypeCorrectclass) handles complex cases. That algorithm is fundamentally imprecise, so the changes in this PR should be thought of as incremental improvements to the existing design that avoid some of its worst pathologies. In particular, this PR:java.lang.String, which is final;SyntheticUnconstrainedTypetype variable to propagate beyond the return type to which it is applied; and,SyntheticUnconstrainedTypewithjava.lang.Object. Previously, these types would hang around and make the output non-compilable, since they'd been marked as unused.Together, these changes make NullAway-176 reproducible, based on my local testing.