Don't bound a fresh capture type variable by unresolved inference variables - #8299
Conversation
…iables Fixes typetools#8298. When `Resolution.resolveWithCapture` instantiates a variable with a fresh type variable, it used the glb of all of the variable's upper bounds. For `takeExtends(wildcardBox(box))`, the capture variable `γ` for the return type `Box<? extends T>` has upper bounds `U` (via `T`) and `α`, the inference variable for `takeExtends`'s `X`, from `γ <: α`. `α` depends on the resolution of `γ`, so it is still unresolved, and the fresh type variable was bounded by `α` instead of `U`. Incorporation then needed `α <: U`, which was false: a `type.arguments.not.inferred` error, or a crash when `U` is F-bounded or the argument is `Box<String>`. javac records `γ <: α` only as a lower bound of `α`, so the capture's bounds are all proper when it is resolved. Now the fresh type variable's upper bound uses only the upper bounds whose inference variables are all being resolved together with it. Bounds that mention those variables are still needed, since the substitution replaces them. The omitted bounds stay in the bound set and are checked during incorporation.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: typetools/checker-framework/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe capture-based type-inference resolver now filters upper bounds whose inference variables are not all among the variables being resolved before computing a greatest lower bound. Regression tests cover unconstrained, F-bounded, Fixed issue severity: <fixed_issue_severity>Low</fixed_issue_severity> Priority: ➖ Normal Change: Bug fix Merge Risk: ⚪ Minimal · up to This fixes a type-inference crash involving wildcard capture. No concrete merge-blocking risk was found, and regression tests were added for the reported scenarios. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change affects a shared type-inference rule, but the inspected code keeps excluded bounds for later checking and does not introduce a new public entrypoint. No security issue was established. Some downstream coverage remains incomplete. Retained concerns Security review detailsSecurity Blast Radius
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2 participants
Fixes #8298.
When
Resolution.resolveWithCaptureinstantiates a variable with a fresh type variable, it used the glb of all of the variable's upper bounds. FortakeExtends(wildcardBox(box)), the capture variableγfor the return typeBox<? extends T>has upper boundsU(viaT) andα, the inference variable fortakeExtends'sX, fromγ <: α.αdepends on the resolution ofγ, so it is still unresolved, and the fresh type variable was bounded byαinstead ofU. Incorporation then neededα <: U, which was false: atype.arguments.not.inferrederror, or a crash whenUis F-bounded or the argument isBox<String>.javac records
γ <: αonly as a lower bound ofα, so the capture's bounds are all proper when it is resolved. Now the fresh type variable's upper bound uses only the upper bounds whose inference variables are all being resolved together with it. Bounds that mention those variables are still needed, since the substitution replaces them. The omitted bounds stay in the bound set and are checked during incorporation.