Return false if validation fails in commonAssignmentCheck

[aosen-xiong]

No description provided.

[wmdietl]

Is any code actually using the returned value of commonAssignmentCheck and checkArrayInitialization? Can you look whether any subclasses actually use the result?
If there is a use, can you write a test case that fails before this change?

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates commonAssignmentCheck control flow to reflect a failed assignment check when an unexpected varType kind is encountered.

Changes:

• Switches the return value from true to false in the unexpected-type branch of commonAssignmentCheck.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[aosen-xiong]

Are the return values used? Yes.

In BaseTypeVisitor itself:

• commonAssignmentCheck(Tree, ...): return trueResult && falseResult; (conditional-expression branch).
• commonAssignmentCheck(AnnotatedTypeMirror, ExpressionTree, ...): result = checkArrayInitialization(...) && result; and result = commonAssignmentCheck(varType, valueType, ...) && result;.

Subclass overrides that compose with &&:

• LowerBoundVisitor.commonAssignmentCheck: result = super.commonAssignmentCheck(varTree, valueTree, errorKey, extraArgs) && result;
• LessThanVisitor.commonAssignmentCheck: same pattern.
• UpperBoundVisitor.commonAssignmentCheck: same pattern.
• AliasingVisitor.commonAssignmentCheck: boolean result = super.commonAssignmentCheck(varTree, valueExp, errorKey, extraArgs);

Test that fails before the change: I added one and pushed it to this branch. Take a typical conflicting-annotations assignment:

@CommonInvalid Object o = new Object();

When commonAssignmentCheck(Tree, ...) runs, validateType reports type.invalid and returns false. The bug then makes the method return true instead of false.

The new CommonAssignmentChecker (under framework/src/test/java/org/checkerframework/framework/testchecker/commonassignment/) is a subclass that emits the sentinel warning commonassignment.parent.succeeded only when super returns true. With the fix reverted, CI on commit [2887593773](2887593773) shows two unexpected commonassignment.parent.succeeded warnings. With the fix restored on [7ff9362da3](7ff9362da3), CI passes.

The test checker currently lacks full Javadoc, so I added a SKIP-REQUIRE-JAVADOC marker. Do you want to keep the test? If yes, I'll fill in the Javadoc and remove the marker. If not, happy to delete the test and ship just the one-line fix.

[wmdietl]

Thanks for the extensive test. It seems like a very specialized test. I'm not sure we need to keep it around. Do you think we'll have any use for it in the future?
Is there no existing checker where we could cause a type.invalid error?

[aosen-xiong]

Thanks for the extensive test. It seems like a very specialized test. I'm not sure we need to keep it around. Do you think we'll have any use for it in the future? Is there no existing checker where we could cause a type.invalid error?

Thanks, that makes sense. I replaced the dedicated commonassignment checker with a smaller test in the existing h1h2checker.

h1h2checker already had H1Invalid; I changed its validator to report that via reportInvalidType, so it now exercises the same validateType(...) == false path that this PR fixes. The new regression test only adds CommonAssignmentReturn.java, and the visitor emits a sentinel warning if super.commonAssignmentCheck(Tree, ...) incorrectly returns true after the invalid LHS was rejected.

So yes, we can use an existing checker, and the specialized checker is gone.

[wmdietl]

Nice way to test it! One question about a warning we no longer get.

[wmdietl]

Does it make sense that the cast.unsafe.constructor.invocation warning is gone?