Skip to content

fix Ignored issubclass(categories, enum.Enum) check #3283#3339

Open
asukaminato0721 wants to merge 1 commit intofacebook:mainfrom
asukaminato0721:3283
Open

fix Ignored issubclass(categories, enum.Enum) check #3283#3339
asukaminato0721 wants to merge 1 commit intofacebook:mainfrom
asukaminato0721:3283

Conversation

@asukaminato0721
Copy link
Copy Markdown
Contributor

Summary

Fixes #3283

isinstance(x, type) preserves known class-object types so negative issubclass narrowing distributes over union members instead of giving up when the union also contains non-class values.

Test Plan

add test

@meta-cla meta-cla Bot added the cla signed label May 8, 2026
@github-actions github-actions Bot added the size/s label May 8, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 8, 2026

Diff from mypy_primer, showing the effect of this PR on open source code:

bokeh (https://github.com/bokeh/bokeh)
- ERROR src/bokeh/core/property/bases.py:480:30-49: `str` is not assignable to variable `type_param` with type `Property[Any] | type[Property[Any]]` [bad-assignment]
+ ERROR src/bokeh/core/property/instance.py:99:29-48: `type | type[T]` is not assignable to variable `instance_type` with type `type[Serializable]` [bad-assignment]
- ERROR src/bokeh/core/property/instance.py:110:16-29: Returned type `type | type[Serializable]` is not assignable to declared return type `type[T]` [bad-return]
+ ERROR src/bokeh/core/property/instance.py:110:16-29: Returned type `type[Serializable]` is not assignable to declared return type `type[T]` [bad-return]
- ERROR src/bokeh/document/document.py:755:32-40: Argument `dict[str | type[_Operator], Any] | type[Model]` is not assignable to parameter `selector` with type `dict[str | type[_Operator], Any]` in function `Document.select` [bad-argument-type]
- ERROR src/bokeh/model/model.py:553:32-40: Argument `dict[str | type[_Operator], Any] | type[Model]` is not assignable to parameter `selector` with type `dict[str | type[_Operator], Any]` in function `Model.select` [bad-argument-type]
- ERROR src/bokeh/util/callback_manager.py:99:35-40: Cannot index into `dict[str, list[EventCallback]]` [bad-index]
- ERROR src/bokeh/util/callback_manager.py:101:36-41: Argument `str | type[Event]` is not assignable to parameter `element` with type `str` in function `set.add` [bad-argument-type]

pytest (https://github.com/pytest-dev/pytest)
- ERROR src/_pytest/raises.py:425:20-23: Returned type `type[BaseException]` is not assignable to declared return type `type[BaseExcT_1]` [bad-return]

materialize (https://github.com/MaterializeInc/materialize)
+ ERROR misc/python/materialize/cli/mzcompose.py:169:47-61: Type `type[Never]` is not iterable [not-iterable]

beartype (https://github.com/beartype/beartype)
- ERROR beartype/_util/text/utiltextlabel.py:556:41-44: Argument `type` is not assignable to parameter `hint` with type `TypeForm[Any]` in function `beartype._util.hint.pep.proposal.pep484585.generic.pep484585gentest.is_hint_pep484585_generic_subbed` [bad-argument-type]
- ERROR beartype/_util/text/utiltextlabel.py:585:38-41: Argument `type` is not assignable to parameter `hint` with type `TypeForm[Any]` in function `beartype._util.hint.pep.proposal.pep544.is_hint_pep544_protocol` [bad-argument-type]

pytest-autoprofile (https://gitlab.com/TTsangSC/pytest-autoprofile)
- ERROR src/pytest_autoprofile/_test_utils.py:372:24-49: `Collection[type[Warning]] | type[Warning]` is not assignable to variable `warnings` with type `Collection[type[Warning]]` [bad-assignment]

static-frame (https://github.com/static-frame/static-frame)
- ERROR static_frame/core/store_config.py:349:31-44: `StoreConfigBase` is not assignable to variable `default` with type `TVStoreConfig | None` [bad-assignment]

pydantic (https://github.com/pydantic/pydantic)
+ ERROR pydantic/v1/fields.py:637:66-76: Expected class object, got `Any` [invalid-argument]

@asukaminato0721 asukaminato0721 marked this pull request as ready for review May 8, 2026 12:51
Copilot AI review requested due to automatic review settings May 8, 2026 12:51
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes incorrect narrowing for patterns combining isinstance(x, type) with issubclass(...), specifically ensuring that known class-object types are preserved and that negative issubclass narrowing correctly distributes over union members (so unions containing both class and non-class values don’t cause narrowing to “give up”).

Changes:

  • Preserve class-object types when narrowing isinstance(x, type) so existing type[T] / bare class refs don’t lose precision.
  • Distribute negative issubclass narrowing over union members, allowing class-like members to be removed while keeping non-class members intact.
  • Add a regression test covering the isinstance(..., type) and issubclass(...) pattern with an else branch expectation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
pyrefly/lib/alt/narrow.rs Adjusts isinstance and negative issubclass narrowing to preserve class-object types and distribute correctly over unions.
pyrefly/lib/test/narrow.rs Adds a regression test reproducing #3283 and asserting correct else-branch narrowing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 8, 2026

Primer Diff Classification

❌ 3 regression(s) | ✅ 4 improvement(s) | 7 project(s) total | +4, -11 errors

3 regression(s) across pytest, materialize, pydantic. error kinds: bad-return, not-iterable, invalid-argument. caused by distribute_over_union(), is_never(). 4 improvement(s) across bokeh, beartype, pytest-autoprofile, static-frame.

Project Verdict Changes Error Kinds Root Cause
bokeh ✅ Improvement +2, -6 bad-argument-type, bad-assignment pyrefly/lib/alt/narrow.rs
pytest ❌ Regression -1 bad-return distribute_over_union()
materialize ❌ Regression +1 not-iterable is_never()
beartype ✅ Improvement -2 bad-argument-type distribute_over_union()
pytest-autoprofile ✅ Improvement -1 bad-assignment pyrefly/lib/alt/narrow.rs
static-frame ✅ Improvement -1 bad-assignment config_type()
pydantic ❌ Regression +1 invalid-argument distribute_over_union()
Detailed analysis

❌ Regression (3)

pytest (-1)

The analysis is factually correct. The _parse_exc method at line 419-425 has the signature def _parse_exc(self, exc: type[BaseExcT_1] | types.GenericAlias, expected: str) -> type[BaseExcT_1]. After the isinstance(exc, type) check on line 422, the type of exc should be narrowed to type[BaseExcT_1] (the intersection of type[BaseExcT_1] | types.GenericAlias with type). Since type[BaseExcT_1] is a subtype of type, the narrowed type should preserve the TypeVar parameterization, giving type[BaseExcT_1], which matches the declared return type type[BaseExcT_1]. The error where pyrefly widens this to type[BaseException] (losing the TypeVar information and using only the bound) is indeed a false positive caused by the isinstance narrowing not properly preserving the TypeVar parameterization. The analysis correctly identifies this as a bug in pyrefly's type narrowing logic.
Attribution: The change to distribute_over_union() in pyrefly/lib/alt/narrow.rs (lines 401-405) adds a special case: when isinstance checks against type and the left side is already Type(_) | ClassDef(_), it returns the left type unchanged. This preserves type[BaseExcT_1] instead of widening it to type[BaseException], fixing the false positive.

materialize (+1)

This is a type inference failure (regression). On line 168-169, the code checks isinstance(action.choices, type) and issubclass(action.choices, enum.Enum), and inside that if block, action.choices should be narrowed to type[enum.Enum]. Enum classes are iterable (iterating yields their members), so [e.name for e in action.choices] is perfectly valid. Pyrefly is instead inferring type[Never] for action.choices in the positive branch, which is a clear inference failure. The Never type in the error message confirms this. The PR's changes to pyrefly/lib/alt/narrow.rs in the negative issubclass narrowing path appear to have a side effect on positive narrowing — specifically, the instance_result.is_never() check at line 643 returns Never directly, which may be leaking into the positive branch or the narrowing is incorrectly subtracting enum.Enum from type[enum.Enum] to produce type[Never].
Attribution: The change in pyrefly/lib/alt/narrow.rs in the distribute_over_union section modifies how negative issubclass narrowing works. The new code at lines 635-649 distributes the subtraction over union members. When left is type[enum.Enum] and the issubclass check succeeds (positive branch), the negative branch (else) should have type[enum.Enum] removed. But the issue is that in the positive branch (line 169), action.choices should be narrowed to type[enum.Enum], which IS iterable. Instead, pyrefly is inferring type[Never], indicating the narrowing logic is subtracting too aggressively — it's subtracting enum.Enum from enum.Enum and getting Never, then wrapping it back as type[Never]. The instance_result.[is_never()](https://github.com/facebook/pyrefly/blob/main/pyrefly/lib/alt/narrow.rs) check at line 643 returns Never directly instead of preserving the original type in the positive branch. However, this error is on line 169 which is INSIDE the if block (the positive branch of isinstance(action.choices, type) and issubclass(action.choices, enum.Enum)), so action.choices should be type[enum.Enum]. The type[Never] suggests pyrefly is incorrectly narrowing the positive branch.

pydantic (+1)

This is a false positive introduced by the PR. self.type_ is Any, and after isinstance(self.type_, type) narrows it, it should be usable as a class argument to isinstance. The PR's narrowing changes appear to have broken this path for Any types, causing pyrefly to reject valid code that both mypy and pyright accept.
Attribution: The change to distribute_over_union() in pyrefly/lib/alt/narrow.rs for isinstance narrowing, specifically the new code block at lines 401-405 that preserves Type(_) | ClassDef(_) but doesn't handle Any properly. When self.type_: Any is narrowed through isinstance(self.type_, type), the result is no longer recognized as a valid class object for the subsequent isinstance(None, self.type_) call.

✅ Improvement (4)

bokeh (+2, -6)

This is a net improvement. The PR fixes a real narrowing bug (#3283) where isinstance(x, type) followed by negative issubclass wasn't distributing correctly over union members. The 6 removed errors are false positives that resulted from imprecise narrowing — e.g., type | type[Serializable] instead of just type[Serializable], and incorrect union types flowing into function arguments. The 2 new errors in instance.py are genuine type issues: the class Object[T] (where T is bound to object) declares a local instance_type: type[Serializable] which is incompatible with the generic type[T], and then returns type[Serializable] where type[T] is expected. Pyright agrees with both new errors. The code has a real annotation mismatch — the local variable annotation is too specific for the generic context. Net: 6 false positives removed, 2 true positives added = clear improvement.
Attribution: The changes in pyrefly/lib/alt/narrow.rs caused all these changes. Specifically: (1) The new code at line 401-405 that preserves Type(_) | ClassDef(_) when the isinstance target is type — this makes isinstance(x, type) preserve known class-object types. (2) The refactored negative issubclass narrowing at lines 635-649 that now uses distribute_over_union to narrow each union member individually, with untype_opt applied per-member rather than to the whole union. This allows negative issubclass to correctly eliminate specific union members.

beartype (-2)

These were false positives. The label_type function declares cls: type, and passes cls to functions expecting TypeForm[Any]. At runtime, a type object (i.e., a class) is indeed a valid type form that can be used as a type hint. The # pyright: ignore comment on line 556 confirms that pyright also flagged this same issue, indicating this is a known false positive across multiple type checkers. The fundamental issue is that type checkers were not treating type as assignable to TypeForm[Any]. Note that TypeForm is from the draft PEP 747 and its exact compatibility rules with type vary across type checker implementations. The PR's changes improved pyrefly's handling so that type values are now correctly treated as compatible with TypeForm[Any], removing these spurious errors.
Attribution: The change to distribute_over_union() in pyrefly/lib/alt/narrow.rs (lines 401-405) preserves Type::Type(_) | Type::ClassDef(_) when narrowing through isinstance(x, type). This improved type preservation likely allows type to be correctly recognized as assignable to TypeForm[Any] in downstream type checking, removing the false positive bad-argument-type errors.

pytest-autoprofile (-1)

The removed bad-assignment error was a false positive. The error indicates that a value of type Collection[type[Warning]] | type[Warning] was being assigned to a variable typed as Collection[type[Warning]]. Without the source code, we cannot confirm the exact pattern, but the PR likely fixes how pyrefly handles type narrowing or assignability in the context of union types. The fix may involve better narrowing of union types (e.g., after an isinstance or issubclass check that should eliminate type[Warning] from the union), or it may involve improved handling of how union members are checked for assignability. The error was a false positive because the code logic likely ensures that by the point of assignment, the type should have been narrowed appropriately.
Attribution: The change to the distribute_over_union logic in pyrefly/lib/alt/narrow.rs (lines 635-651) is responsible. Previously, negative issubclass narrowing called self.untype_opt on the entire union and then subtracted, which could fail when the union contained non-class members. The new code distributes over union members individually: for each member, it attempts untype_opt, and if that fails (non-class member), it preserves the member unchanged. If subtraction succeeds completely (result is Never), the member is removed. This allows type[Warning] to be properly subtracted while preserving Collection[type[Warning]].

static-frame (-1)

This is a false positive removal. The code at line 349 assigns config_type() to default where config_type: type[TVStoreConfig] (derived from the types of values in config_map: Mapping[Any, TVStoreConfig]). The result config_type() is of type TVStoreConfig, which is perfectly assignable to TVStoreConfig | None. Pyrefly was previously failing to preserve the precise type through the isinstance(config_type, type) guard, causing it to lose the TypeVar binding and see only StoreConfigBase. The PR fix in narrow.rs preserves Type/ClassDef types through isinstance(x, type) checks, fixing this false positive.
Attribution: The change in pyrefly/lib/alt/narrow.rs in the distribute_over_union section (lines 398-403) adds special handling for isinstance(x, type) checks: when the right side is type and the left side is already a Type or ClassDef, it preserves the left type. This means that after isinstance(config_type, type), config_type retains its type[TVStoreConfig] type instead of being widened. The second change (lines 632-652) restructures negative issubclass narrowing to use distribute_over_union, which allows per-member narrowing of union types. Together, these changes allow the type of config_type to be properly tracked through the isinstance+issubclass guard, so config_type() correctly produces TVStoreConfig rather than StoreConfigBase.

Suggested fixes

Summary: The PR's negative issubclass narrowing changes cause regressions in materialize (type[Never] instead of type[enum.Enum]) and pydantic (Any not handled after isinstance(x, type)), while pytest's -1 appears to be a correctly fixed false positive.

1. In the distribute_over_union closure for negative issubclass narrowing in pyrefly/lib/alt/narrow.rs (lines 643-645), when instance_result.is_never(), the code returns instance_result (i.e., Never) directly. This is correct for the negative/else branch (the member should be eliminated), but the same distribute_over_union result is also used to compute the positive branch type. When Never is returned as a union member, it produces type[Never] in the positive branch. The fix: when instance_result.[is_never()](https://github.com/facebook/pyrefly/blob/main/pyrefly/lib/alt/narrow.rs), return Type::never() (which distribute_over_union should filter out as a union member), but ensure the positive branch separately computes its narrowed type without going through this negative-branch logic. Alternatively, change line 643-645 from if instance_result.[is_never()](https://github.com/facebook/pyrefly/blob/main/pyrefly/lib/alt/narrow.rs) { instance_result } to if instance_result.[is_never()](https://github.com/facebook/pyrefly/blob/main/pyrefly/lib/alt/narrow.rs) { Type::never() } — but more importantly, verify that the positive branch of issubclass narrowing is computed independently and doesn't reuse this negative-branch result.

Files: pyrefly/lib/alt/narrow.rs
Confidence: medium
Affected projects: materialize
Fixes: bad-argument-type
The materialize error shows action.choices becoming type[Never] inside the positive branch of isinstance+issubclass. The new distribute_over_union for negative issubclass returns Never when subtraction eliminates a member, but this Never value appears to leak into the positive branch computation. Fixing this eliminates 1 pyrefly-only error in materialize.

2. In the isinstance narrowing special case at lines 401-405 of pyrefly/lib/alt/narrow.rs, add handling for Type::Any (or whatever represents Any): when the right side is builtin type and the left side is Any, return Type::type_of(Type::Any) (i.e., type[Any]). Currently the guard only matches Type::Type(_) | Type::ClassDef(_), so Any falls through to generic isinstance narrowing which loses the property that the result is a valid class object. The fix: extend the match to Type::Type(_) | Type::ClassDef(_) | Type::Any.

Files: pyrefly/lib/alt/narrow.rs
Confidence: high
Affected projects: pydantic
Fixes: bad-argument-type
In pydantic, self.type_ is Any, and after isinstance(self.type_, type), it should be type[Any] which is valid as a class argument to isinstance(). The current special case doesn't handle Any, so the narrowed type is not recognized as a valid class. This eliminates 1 pyrefly-only error in pydantic.


Was this helpful? React with 👍 or 👎

Classification by primer-classifier (7 LLM)

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.

Ignored issubclass(categories, enum.Enum) check

2 participants