Skip to content

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

Closed
Aayushyaash wants to merge 4 commits into
meta:mainfrom
Aayushyaash:fix-variance-mismatch-constrained-typevar
Closed

Aayushyaash wants to merge 4 commits into
meta:mainfrom
Aayushyaash:fix-variance-mismatch-constrained-typevar

Conversation

@Aayushyaash

Copy link
Copy Markdown
Contributor

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 #3655

  • I am a human, and I fully understand the code I am submitting.

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 TypeVars in protocols continue to be checked for variance mismatch.

Validation

cargo check
cargo test -p pyrefly variance_inference -j 2
cargo test -p pyrefly test::protocol -j 2
cargo fmt -- --check

@meta-codesync

meta-codesync Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

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.)

@github-actions

This comment has been minimized.

@stroxler stroxler left a comment

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.

I think the conformance results need an update, and I think the comment wording is a little too strong here

Comment thread pyrefly/lib/alt/solve.rs Outdated
// 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.

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

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.

@github-actions github-actions Bot added size/xs and removed size/xs labels Nov 3, 2026
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@Aayushyaash

Copy link
Copy Markdown
Contributor Author

Thanks for the review!
Updated the comment and regenerated the conformance output snapshots via conformance_output.py. All local tests and lint checks are green.

mangeshraut712

This comment was marked as low quality.

@stroxler stroxler left a comment

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.

Review automatically exported from Phabricator review in Meta.

jorenham pushed a commit to jorenham/pyrefly that referenced this pull request Nov 4, 2026
…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
@stroxler

stroxler commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

I'm not sure why this didn't auto-close, the commit landed in e01f27f

@stroxler stroxler closed this Nov 4, 2026
@Aayushyaash
Aayushyaash deleted the fix-variance-mismatch-constrained-typevar branch November 6, 2026 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

variance-mismatch false positive on constrained typevar in protocol

3 participants