Repository navigation
fix: do not emit variance-mismatch for constrained typevars in protocols - #5500
Aayushyaash wants to merge 4 commits into
Conversation
|
This pull request has been imported. If you are a Meta employee, you can view this in D129256923. (Because this pull request was imported automatically, there will not be any future comments.) |
This comment has been minimized.
This comment has been minimized.
stroxler
left a comment
There was a problem hiding this comment.
I think the conformance results need an update, and I think the comment wording is a little too strong here
| // with a narrower variance. We only check invariant TypeVars here | ||
| // because wrong variance on covariant/contravariant TypeVars is | ||
| // already caught by InvalidVariance at the usage site. | ||
| // Constrained TypeVars are excluded because variance cannot be declared on them. |
There was a problem hiding this comment.
Let's soften the wording here. Variance actually can be declared on them.
In many cases the variance doesn't matter, but if some types are subtypes of other it can, e.g. with (int, float) as a constraint there's a real distinction between covariance and contravariance.
I think it would be more precise to just say that invariance usually makes sense, so complaining about it is too noisy
There was a problem hiding this comment.
Updated the comment:
// Constrained TypeVars are excluded because invariance is a sound default for a fixed
// set of constraints, and the narrower-variance suggestion is usually noise.|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
Thanks for the review! |
stroxler
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
…ols (meta#5500) Summary: In Pyrefly, `check_variance_violations_and_mismatches` warns when an invariant `TypeVar` in a `Protocol` could be declared with a narrower variance to preserve structural subtyping. However, emitting `variance-mismatch` for constrained type variables produces a false positive because the suggested variance change is not valid for constrained TypeVars. This PR fixes the issue by excluding constrained type variables from the `variance-mismatch` diagnostic: 1. Updates the `metadata.is_protocol()` guard in `solve.rs` to ignore `Restriction::Constraints(_)`, avoiding unnecessary calls to `infer_variance_ignoring_declared` when all invariant parameters are constrained. 2. Skips `Restriction::Constraints(_)` inside the parameter loop while preserving `variance-mismatch` checks for unconstrained and bounded (`Restriction::Bound(_)`) type variables. Fixes meta#3655 - [x] I am a human, and I fully understand the code I am submitting. Pull Request resolved: meta#5500 Test Plan: Added unit tests in `pyrefly/lib/test/variance_inference.rs` (`test_constrained_typevar_protocol_variance_mismatch`) covering: - A protocol returning a constrained `TypeVar` (`def value(self) -> T: ...`) without triggering `variance-mismatch`. - A protocol accepting a constrained `TypeVar` in a parameter position (`def consume(self, x: T) -> None: ...`) without triggering `variance-mismatch`. - Ensuring bounded (`bound=...`) and unconstrained `TypeVar`s in protocols continue to be checked for variance mismatch. ## Validation ```bash cargo check cargo test -p pyrefly variance_inference -j 2 cargo test -p pyrefly test::protocol -j 2 cargo fmt -- --check ``` Reviewed By: stroxler Differential Revision: D129256923 fbshipit-source-id: ae0fce42c87e460c9482b303433cdfe5be677861
|
I'm not sure why this didn't auto-close, the commit landed in e01f27f |
Summary
In Pyrefly,
check_variance_violations_and_mismatcheswarns when an invariantTypeVarin aProtocolcould be declared with a narrower variance to preserve structural subtyping. However, emittingvariance-mismatchfor constrained type variables produces a false positive because the suggested variance change is not valid for constrained TypeVars.This PR fixes the issue by excluding constrained type variables from the
variance-mismatchdiagnostic:metadata.is_protocol()guard insolve.rsto ignoreRestriction::Constraints(_), avoiding unnecessary calls toinfer_variance_ignoring_declaredwhen all invariant parameters are constrained.Restriction::Constraints(_)inside the parameter loop while preservingvariance-mismatchchecks for unconstrained and bounded (Restriction::Bound(_)) type variables.Fixes #3655
Test Plan
Added unit tests in
pyrefly/lib/test/variance_inference.rs(test_constrained_typevar_protocol_variance_mismatch) covering:TypeVar(def value(self) -> T: ...) without triggeringvariance-mismatch.TypeVarin a parameter position (def consume(self, x: T) -> None: ...) without triggeringvariance-mismatch.bound=...) and unconstrainedTypeVars in protocols continue to be checked for variance mismatch.Validation