Skip to content

fix: do not emit variance-mismatch for constrained typevars in protocols #5500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Aayushyaash wants to merge 4 commits into from
Closed

fix: do not emit variance-mismatch for constrained typevars in protocols #5500

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6154fed
test: add regression test for constrained typevar in protocol
Aayushyaash Oct 23, 2026
3fe3b04
fix: do not emit variance-mismatch for constrained typevars in protocols
Aayushyaash Oct 23, 2026
6976056
Merge branch 'meta:main' into fix-variance-mismatch-constrained-typevar
Aayushyaash Nov 3, 2026
e579257
fix: update conformance outputs and refine constrained typevar comment
Aayushyaash Nov 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions pyrefly/lib/alt/solve.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3977,16 +3977,21 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> {
// 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.

@stroxler stroxler Nov 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Aayushyaash Aayushyaash Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let metadata = self.get_metadata_for_class(class);
if metadata.is_protocol()
&& tparams
.as_vec()
.iter()
.any(|p| p.is_type_var() && p.variance() == PreInferenceVariance::Invariant)
&& tparams.as_vec().iter().any(|p| {
p.is_type_var()
&& p.variance() == PreInferenceVariance::Invariant
&& !matches!(p.restriction(), Restriction::Constraints(_))

@mangeshraut712 mangeshraut712 Nov 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious about the asymmetry with Restriction::Explicit (bounded) TypeVars: protocols_variance.py / the conformance report presumably still flags invariant bounded TypeVars as variance-mismatch (e.g. T = TypeVar("T", bound=X) used covariantly). Was the noise argument applied only to constrained TypeVars because a bounded TypeVar can soundly narrow its variance within the bound, or is this an intentional first step? If bounded TypeVars are also usually noise for this warning, the doc comment here might merit one sentence explaining why Constraints gets the carve-out rather than Explicit.

@stroxler stroxler Nov 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argument (originally from @DetachHead) is that variance is generally irrelevant to constrained type variables, because a constrained type variable solves to exactly one of its bounds.

As a result, subtyping is almost irrelevant (if the types are not subtypes of one another, like (str, bytes)), which makes variance irrelevant (in the sense that absent subtyping, variance doesn't even exist as a concept - for example Haskell and OCaml and Rust have and require no notion of variance) and means this warning produces unnecessary noise.

The argument is heuristic because it's possible to use a constrained type variable whose options are orderable in the type system, for example I've seen (int, float) before. In that case variance is technically still meaningful.

})
{
let inferred = self.infer_variance_ignoring_declared(class);
for tparam in tparams.as_vec() {
if !tparam.is_type_var() || tparam.variance() != PreInferenceVariance::Invariant {
if !tparam.is_type_var()
|| tparam.variance() != PreInferenceVariance::Invariant
|| matches!(tparam.restriction(), Restriction::Constraints(_))
{
continue;
}
let inferred_v = inferred.get(tparam.name());
Expand Down
15 changes: 15 additions & 0 deletions pyrefly/lib/test/variance_inference.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,21 @@ class Foo(Generic[T_co]):
"#,
);

testcase!(
test_constrained_typevar_protocol_variance_mismatch,
r#"
from typing import Protocol, TypeVar

T = TypeVar("T", str, bytes)

class Foo(Protocol[T]):
def value(self) -> T: ...

class Bar(Protocol[T]):
def consume(self, x: T) -> None: ...
"#,
);

testcase!(
test_shallow_contravariant_in_return,
r#"
Expand Down
Loading