Handle Rawtype more gracefully#1438
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a Checker Framework crash when PropagationTypeAnnotator encounters a wildcard related to a raw declared type (as reported in #792), and adds a regression test to prevent recurrence.
Changes:
- Add a regression test (
RawtypeCrash.java) that triggers the rawtype/wildcard scenario from the crash report. - Update
PropagationTypeAnnotator#getTypeParameterElementto returnnull(instead of throwing) when the enclosing declared type is raw. - Record the fix in the changelog as closing
eisop#792.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| framework/tests/viewpointtest/RawtypeCrash.java | Adds a minimal test case that previously crashed the framework during type annotation propagation. |
| framework/src/main/java/org/checkerframework/framework/type/typeannotator/PropagationTypeAnnotator.java | Avoids throwing BugInCF when attempting to map a wildcard to a type parameter in the presence of raw types. |
| docs/CHANGELOG.md | Notes the issue as closed in the upcoming release notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wmdietl
left a comment
There was a problem hiding this comment.
Thanks for looking into fixing this!
Isn't the code at https://github.com/aosen-xiong/checker-framework/blob/08171af48258a6d3a543f0ba85932dabd712dbb6/framework/src/main/java/org/checkerframework/framework/type/typeannotator/PropagationTypeAnnotator.java#L91 supposed to fix up raw types?
Is this crash an indication that the fix-up over there isn't doing the right things?
Maybe that code should do something different for wildcards?
Debugging the added test shows two visitWildcard iterations linked by scan(wildcard.getExtendsBound(), null). Iteration 1 (non-raw wildcard)wildcard: ? extends V (typeArgOfRawType=false) Iteration 2 (recursive, raw/synthesized wildcard)reached from iteration 1 via scan(extendsBound) This is a different wildcard instance/symbol from iteration 1 (different wrapper and underlying wildcard). |
|
Btw, the fix up for raw types are completely skipped in this case because of this check. |
| visitedNodes.put(wildcard, null); | ||
|
|
||
| // Raw wildcard args are already fixed up in visitDeclared. | ||
| // Recursive traversal through scan(wildcard.getExtendsBound(), ...) can |
There was a problem hiding this comment.
I don't understand what this comment is trying to say. The body of the then-block calls scan on both bounds of the wildcard. Is this intended? What is the problem with re-entering?
Is there any point in scanning these two bounds here? Will they ever propagate anything useful?
There was a problem hiding this comment.
I updated the comment.
| @@ -0,0 +1,4 @@ | |||
| public class RawtypeCrash<V> { | |||
There was a problem hiding this comment.
Is this problem specific to the viewpoint checker?
Would it be better to add to all-systems (possibly suppressing all warnings to just check for the crash)?
There was a problem hiding this comment.
I tried Nullness, Interning, and Tainting checkers and did not see this BugInCF crash from RawtypeCrash there.
Do you think we should keep it in the viewpoint checker tests or move it all-systems?
There was a problem hiding this comment.
If the only checker that is breaking with this code is the Viewpoint Checker, maybe it needs a change? Why does no other type checker fail with this code? Doesn't that indicate that the VP Checker is doing something wrong?
In any case, isn't this another argument to move it to all-systems, to ensure that all type systems work with this code?
There was a problem hiding this comment.
I tried adding all-systems to ViewpointTestCheckerTest. It exposes many additional viewpoint-checker failures/crashes, so enabling it wholesale is a follow-up task.
For this PR, I will keep the small duplicated viewpointtest/RawtypeCrash.java. The all-systems copy provides useful broad coverage, but by itself it does not exercise the viewpoint checker.
There was a problem hiding this comment.
Other failures are also discussed in #433.
Fixes #792
Fixes a crash in propagation for wildcard type arguments of raw types.
When
PropagationTypeAnnotatorvisits a raw declared type, it already fixes up the synthesized wildcard type arguments from the raw type’s declaration bounds. This PR avoids re-processing those raw-type wildcard arguments invisitWildcard, preventing the crash reported ineisop#792.