Viewpoint test checker: lost receiver invoke poly receiver method - #1173
Viewpoint test checker: lost receiver invoke poly receiver method#1173aosen-xiong wants to merge 37 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.
|
@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. |
The viewpoint test checker obtained method type parameter bounds through AnnotatedTypeFactory#methodTypeVariablesFromUse, which fetched the raw bounds and then viewpoint-adapted them itself. "Use adapted bounds for method/constructor type arguments" (eisop#1850) performs that adaptation in AbstractViewpointAdapter#viewpointAdaptMethod and #viewpointAdaptConstructor, so the framework method duplicated it. Remove methodTypeVariablesFromUse and its BaseTypeVisitor call site, which restores the master implementation, and read the already-adapted bounds from methodFromUse and constructorFromUse. Constructor invocations are now checked as well; previously only method invocations reported viewpointtest.lost.in.bounds. Update the expectations in the tests added by eisop#1850, and delete ViewpointAdaptationBounds.java, whose cases those tests cover. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ViewpointTestQualifierHierarchy overrode isSubtypeQualifiers to make @lost non-reflexive. That override was removed in "Check lost qualifier on assignment LHS", leaving a subclass whose constructor only forwards to NoElementQualifierHierarchy. AnnotatedTypeFactory#createQualifierHierarchy already returns a NoElementQualifierHierarchy constructed with the same arguments, so delete the class along with the createQualifierHierarchy override that was its only caller. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lost participates in subtyping like any other qualifier in this test checker; the test exercises the positions where @lost is rejected, such as assignment targets and parameters. The old name described the qualifier hierarchy specialization that this branch removes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
I've reviewed this PR. Since #1850 is now merged, this PR's approach of making I've also gone ahead and pulled in |
|
@aosen-xiong Should the PR have a more general title? It seems to change a lot more than the title implies. If the receiver parameter is Poly and the actual receiver is Lost, is the call allowed? Would this result in an error, because Lost would be assigned to Lost? |
Okay, I will update the title and description.
Right, I think the poly qualifier needs to be resolved first before checking if lost is part of the signature. I will fix it. |
If we have a getter |
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.