Viewpoint test checker: lost receiver invoke poly receiver method#1173
Viewpoint test checker: lost receiver invoke poly receiver method#1173aosen-xiong wants to merge 28 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the Viewpoint Test Checker’s handling of @Lost by removing its special non-reflexive subtyping rule and instead reporting a dedicated error when @Lost appears on the left-hand side of assignments.
Changes:
- Update tests to expect the new
viewpointtest.lost.lhserror for invalid assignment LHS cases. - Add an assignment visitor check to flag
@Loston assignment LHS. - Remove the custom qualifier-hierarchy override that made
@Lostnon-reflexive, and update@Lostdocumentation accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| framework/tests/viewpointtest/VarargsConstructor.java | Adjusts expected error annotations in varargs constructor test. |
| framework/tests/viewpointtest/TestGetAnnotatedLhs.java | Updates expected errors to include viewpointtest.lost.lhs on assignments. |
| framework/tests/viewpointtest/LostNonReflexive.java | Updates expectations and adds a method/use-site to exercise new behavior. |
| framework/src/test/java/viewpointtest/quals/Lost.java | Updates @Lost Javadoc to reflect new semantics (invalid as assignment LHS). |
| framework/src/test/java/viewpointtest/ViewpointTestVisitor.java | Adds LHS assignment check for @Lost. |
| framework/src/test/java/viewpointtest/ViewpointTestQualifierHierarchy.java | Removes the non-reflexive @Lost subtyping override. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @param variableTree the assignment target to check | ||
| * @param errorTree the tree on which to report the error | ||
| */ | ||
| private void checkLostLhs(Tree variableTree, Tree errorTree) { |
There was a problem hiding this comment.
Wouldn't it be easier to just override commonAssignmentCheck? Are all these cases covered there?
What about the adapted parameter type becoming Lost? Is there no check needed there? Is there a test for this?
There was a problem hiding this comment.
Addressed. I followed the same split used by the Universe checker:
UniverseVisitor reports separate diagnostics for @Lost on assignment LHS vs adapted method/constructor parameters.
UniverseTypeValidator separately checks for @Lost in adapted type parameter bounds.
This PR now mirrors that structure for the Viewpoint test checker:
viewpointtest.lost.lhs for assignment targets, including compound assignments and increments/decrements.
viewpointtest.lost.parameter for method/constructor pseudo-assignments whose adapted parameter type contains @Lost.
viewpointtest.lost.in.bounds for adapted type parameter bounds containing @Lost, implemented in a new ViewpointTestTypeValidator.
I also added tests for the adapted-parameter and adapted-bound cases, including LostInBounds.
| } | ||
|
|
||
| void callMethod(@Top Methods m, @A Object a, @Bottom Object b) { | ||
| // Here, the upper bound of T adapts to @Lost, but because we pass no arguments, |
There was a problem hiding this comment.
@aosen-xiong This seems like a missing case? The type variable has a lost bound, but there is no error. The type variable doesn't appear in the signature, which makes the test odd, but the concern seems valid.
|
@aosen-xiong CI is failing. |
Yeah, I recently found out that a better strategy is to merge #1850 first. So I unassigned you from this one. |
Merge #1850 first.
This updates the viewpoint test checker coverage for receiver-dependent qualifiers.
I added a test case that checks that a method with a polymorphic receiver can still be invoked when viewpoint adaptation produces
@Lost. Rather than modelling this by making@Lostnon-reflexive in the qualifier hierarchy, this PR keeps the normal subtype reflexivity behaviour and rejects@Lostwhere it is actually invalid: on assignment left-hand sides like universe type did https://github.com/opprop/universe/blob/ee75b409f57b0aaff733e0b5a3682dd2768dccc9/src/main/java/universe/UniverseVisitor.java#L228-L230.I think this will be sound and more expressive.