Skip to content

fix(enums): reject incompatible Enum value-lookup arguments - #5417

Open
mangeshraut712 wants to merge 15 commits into
meta:mainfrom
mangeshraut712:fix/enum-call-rejects-none
Open

mangeshraut712 wants to merge 15 commits into
meta:mainfrom
mangeshraut712:fix/enum-call-rejects-none

Conversation

@mangeshraut712

Copy link
Copy Markdown
Contributor

Summary

EnumMeta.__call__ annotates its value parameter as Any, so value-lookup
calls like Category(None) against an int-valued enum produced no type error
even when a class-defined __init__ (or the inferred member value type) would
clearly reject the argument.

This change refines enum value-lookup calls after the existing metaclass call
path: if the enum class defines its own __init__, check the first parameter
against that signature; otherwise fall back to the non-Any member value type
from enum_value_lookup_on_class. Member construction via __new__ is left
alone — that is class-definition time, not value lookup (see SeFileType).

Test plan

  • test_enum_call_rejects_incompatible_value — custom __init__ rejects None
  • test_enum_call_rejects_none_for_int_members — inferred Literal[1, 2] rejects None
  • Existing test_enum_call* (incl. SeFileType metaclass/self) still pass

Fixes #5398

EnumMeta.__call__ types `value` as Any, so Category(None) and similar
calls produced no diagnostic even when a class-defined __init__ (or the
inferred member value type) clearly rejects the argument. Refine
value-lookup calls against that __init__ when present, otherwise against
the non-Any member value type, without routing through __new__ (member
construction at class definition time).

Fixes meta#5398
@meta-codesync

meta-codesync Bot commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

This pull request has been imported. If you are a Meta employee, you can view this in D127891464. (Because this pull request was imported automatically, there will not be any future comments.)

CI failed cargo fmt --check on the Enum call/lookup changes.
@github-actions github-actions Bot added size/l and removed size/l labels Oct 27, 2026
@github-actions

This comment has been minimized.

Literal member unions describe which values exist, but Enum(x) accepts
any value of the promoted base type at type-check time and raises only
when the concrete value is missing. Promote literals so `cls(val: str)`
stays valid, and accept an already-constructed member (`cls(cls.A)`).

Fixes CI red on test_access_value_on_mixed_type_enum and
test_enum_classmethod after the reject-none tip.
@github-actions github-actions Bot added size/l and removed size/l labels Oct 27, 2026
@github-actions github-actions Bot added size/l and removed size/l labels Oct 27, 2026
@github-actions github-actions Bot added size/l and removed size/l labels Oct 27, 2026
@github-actions

This comment has been minimized.

@mangeshraut712

Copy link
Copy Markdown
Contributor Author

All checks are green (tests, mypy_primer, CLA, import). This is ready for a maintainer pass; I can iterate quickly on review notes.

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

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

Thanks! This could be a good improvement, but I think there are two underlying problems based on AI review.

  1. Member construction and value lookup have different contracts. __init__ describes creating members during class definition; it does not run when calling MyEnum(value). We should remove the initializer-based lookup check rather than trimming it to the first parameter.

Just to explain further, AI believes (and it's usually right about his sort of thing) that the lookup path for an Enum class MyEnum is roughly:

MyEnum(value)
→ EnumMeta.__call__
→ Enum.__new__
→ existing member, or _missing_(value), or ValueError

which means that MyEnum's own constructors aren't actually relevant. I think this is correct - it's part of why Enum behavior is so unusual.

Inferring the lookup input solely from member value types is also too restrictive. A custom metaclass can implement a different call contract, missing can accept additional input types, and ordinary enum lookup uses equality—for example, an int-valued enum can successfully look up 1.0.

  1. Arguments are inferred twice, causing exponential work for nested calls. Please reuse inferred arguments between metaclass checking and refinement, or extend the existing flattening protection. In optimized builds, 20 nested enum calls exceeded 20 seconds on this revision versus approximately 0.18 seconds on its parent.

It would be awesome if we could add regression coverage for:

  • Tuple-valued members with multi-parameter __init__.
  • Lookup by an existing member and by value=, regardless of initializer parameter names.
  • __new__ transforming creation arguments into a different value type.
  • Custom metaclass and _missing_ input contracts.
  • Cross-numeric equality, such as looking up 1.0 against member value 1.
  • Deeply nested enum calls.

It's possible that some of the feedback above is wrong, I'm honestly not an expert on Enum so I'm relying on AI review (it's not possible for me as a maintainer to keep up anymore without this).

Usually I think the AI review notes are correct these days, but not always. Feel free to push back if you think there's an error in this feedback!

Value lookup (MyEnum(x)) goes through EnumMeta.__call__ -> Enum.__new__,
so the enum's own __init__/__new__ are irrelevant at call sites. Remove the
__init__-based check entirely and only refine against the member value type
when the call contract is stock EnumMeta: no custom metaclass, no user
_missing_, no user __new__, and a non-Any value type. Numeric values also
accept float/complex since lookup uses equality (1.0 == 1).

Infer the lookup argument once and pass the inferred type to the metaclass
__call__ check, avoiding exponential re-inference for nested enum calls.

Add regression tests for tuple values with multi-param __init__, lookup by
member and value=, __new__ changing the value type, custom metaclass and
_missing_, cross-numeric equality, and deeply nested calls.
…ts-none

# Conflicts:
#	pyrefly/lib/test/enums.rs
@mangeshraut712

Copy link
Copy Markdown
Contributor Author

@stroxler thanks for the review. I pushed changes for both points.

  1. I removed the __init__-based lookup check completely. The value argument is now only checked against the member value type when lookup uses stock EnumMeta. That means there is no custom metaclass, no user-defined _missing_, no user-defined __new__, and the value type is not Any. If any of those apply, the call goes back to the old unchecked behavior. Numeric values also accept float and complex, so Color(1.0) works.
  2. The lookup argument is now inferred once, and that inferred type goes to the metaclass __call__ check. This removes the re-inference on nested calls.

I added regression tests for every case you listed: tuple values with a multi-parameter __init__, lookup by member and by value=, __new__ changing the value type, a custom metaclass and _missing_, 1.0 against a value of 1, and deeply nested calls. I also merged main into the branch.

@github-actions github-actions Bot added size/xl and removed size/xl labels Nov 2, 2026
@github-actions

This comment has been minimized.

@rchen152 rchen152 added size/m and removed size/xl labels Nov 2, 2026
@github-actions github-actions Bot added size/m and removed size/m labels Nov 2, 2026
@github-actions

This comment has been minimized.

@mangeshraut712

Copy link
Copy Markdown
Contributor Author

@stroxler Thanks, this round's notes all checked out. Pushed c0eed6b and 189e3b3, then merged main (14ff662).

  • Exponential re-inference: the single positional / value= argument is now inferred once into arg_errors before the hint retries, and the resulting type is reused for the retries and the metaclass call. 20 layers of identity(E(...(1 + "x"))) take about 0.08s in a release build. Added test_enum_call_deeply_nested_error_linear_time.
  • Equality lookups: bool-valued members now accept int/float/complex, Decimal/Fraction-style numbers are accepted against numeric values, and container values (tuple/list/set/frozenset/dict) skip the check completely instead of trying to model element equality.
  • Memberless bases / type[Self]: no restriction when the call is through type[Self] or the class has no members, so Base.parse → cls(raw) is clean.
  • Dataclass mixins: a dataclass base is now recognised as the data type mixin, so E(Data(1)) is accepted.
  • Pre-inferred args: TypeOrExpr::Type arguments are checked too, so the deep identity(...) forms and value= now error the same way the shallow ones do.
  • Also accepted: object, Any, and unions containing them, since lookup goes through equality. That removes the apprise hits in mypy_primer. The remaining hits (operator, graphql-core, cloud-init, schemathesis passing str | None, rotki passing str to an int enum, zulip/meson passing wide unions) all look like real ValueErrors at runtime to me. If you'd rather keep this check quieter on existing code, tell me and I'll gate it differently.

Tests for every case are in test/enums.rs.

@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown

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

============================================================
SUMMARY
============================================================
Total: +8 new errors, -2 fixed errors
By preset: +8/-1 (default), +8/-1 (strict)

Projects with changes (7):
  operator: +2 -0
  rotki: +1 -0
  graphql-core: +1 -0
  schemathesis: +1 -0
  zulip: +1 -0
  meson: +2 -0
  core: +0 -2
============================================================

FULL DIFF DETAILS
------------------------------------------------------------

operator (https://github.com/canonical/operator)
+ ERROR ops/_private/harness.py:1301:42-58: Argument `str | None` is not assignable to parameter `value` with type `ChangeKind | str` [bad-argument-type]
+ ERROR ops/model.py:1270:37-81: Argument `str | None` is not assignable to parameter `value` with type `SecretRotate | str` [bad-argument-type]

rotki (https://github.com/rotki/rotki)
+ ERROR rotkehlchen/assets/asset.py:704:30-38: Argument `str` is not assignable to parameter `value` with type `ChainID | complex | float | int` [bad-argument-type]

graphql-core (https://github.com/graphql-python/graphql-core)
+ ERROR src/graphql/language/parser.py:549:34-55: Argument `str | None` is not assignable to parameter `value` with type `OperationType | str` [bad-argument-type]

schemathesis (https://github.com/schemathesis/schemathesis)
+ ERROR src/schemathesis/core/mutations.py:71:50-80: Argument `Any | None` is not assignable to parameter `value` with type `ParameterLocation | str` [bad-argument-type]

zulip (https://github.com/zulip/zulip)
+ ERROR zerver/tests/test_channel_permissions.py:815:63-72: Argument `bool | int | str` is not assignable to parameter `value` with type `StreamTopicsPolicyEnum | complex | float | int` [bad-argument-type]

meson (https://github.com/mesonbuild/meson)
+ ERROR mesonbuild/envconfig.py:254:42-45: Argument `bool | int | list[str] | str` is not assignable to parameter `value` with type `CMakeSkipCompilerTest | str` [bad-argument-type]
+ ERROR mesonbuild/interpreter/interpreter.py:1176:50-55: Argument `OctalInt | bool | int | list[str] | str` is not assignable to parameter `value` with type `FeatureValue | str` [bad-argument-type]

core (https://github.com/home-assistant/core)
- ERROR homeassistant/components/homeassistant_sky_connect/const.py:53:16-46: Object of class `tuple` has no attribute `usb_product_name` [missing-attribute]
- ERROR homeassistant/helpers/device_registry.py:1143:56-80: Unused `# type: ignore` comment [unused-type-ignore]

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

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

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

============================================================
SUMMARY
============================================================
Total: +8 new errors, -2 fixed errors
By preset: +8/-1 (default), +8/-1 (strict)

Projects with changes (7):
  operator: +2 -0
  rotki: +1 -0
  graphql-core: +1 -0
  schemathesis: +1 -0
  zulip: +1 -0
  meson: +2 -0
  core: +0 -2
============================================================

FULL DIFF DETAILS
------------------------------------------------------------

operator (https://github.com/canonical/operator)
+ ERROR ops/_private/harness.py:1301:42-58: Argument `str | None` is not assignable to parameter `value` with type `ChangeKind | str` [bad-argument-type]
+ ERROR ops/model.py:1270:37-81: Argument `str | None` is not assignable to parameter `value` with type `SecretRotate | str` [bad-argument-type]

rotki (https://github.com/rotki/rotki)
+ ERROR rotkehlchen/assets/asset.py:704:30-38: Argument `str` is not assignable to parameter `value` with type `ChainID | complex | float | int` [bad-argument-type]

graphql-core (https://github.com/graphql-python/graphql-core)
+ ERROR src/graphql/language/parser.py:549:34-55: Argument `str | None` is not assignable to parameter `value` with type `OperationType | str` [bad-argument-type]

schemathesis (https://github.com/schemathesis/schemathesis)
+ ERROR src/schemathesis/core/mutations.py:71:50-80: Argument `Any | None` is not assignable to parameter `value` with type `ParameterLocation | str` [bad-argument-type]

zulip (https://github.com/zulip/zulip)
+ ERROR zerver/tests/test_channel_permissions.py:815:63-72: Argument `bool | int | str` is not assignable to parameter `value` with type `StreamTopicsPolicyEnum | complex | float | int` [bad-argument-type]

meson (https://github.com/mesonbuild/meson)
+ ERROR mesonbuild/envconfig.py:254:42-45: Argument `bool | int | list[str] | str` is not assignable to parameter `value` with type `CMakeSkipCompilerTest | str` [bad-argument-type]
+ ERROR mesonbuild/interpreter/interpreter.py:1176:50-55: Argument `OctalInt | bool | int | list[str] | str` is not assignable to parameter `value` with type `FeatureValue | str` [bad-argument-type]

core (https://github.com/home-assistant/core)
- ERROR homeassistant/components/homeassistant_sky_connect/const.py:53:16-46: Object of class `tuple` has no attribute `usb_product_name` [missing-attribute]
- ERROR homeassistant/helpers/device_registry.py:1143:56-80: Unused `# type: ignore` comment [unused-type-ignore]

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

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

Unfortunately this needs another rebase now, it ran into some conflicts (we've been trying to clear the bulk of the backlog, which means more land races). I was originally going to try to fix-forward since the direction is good, but since it needs another rebase I think it's worth sharing some more feedback.

The biggest bit of feedback is that this is a pretty big change, and tricky to review (which also contributes to land races, as well as more rounds of review). As a rule it's actually better to have a smaller change, but one that it's easier to confirm is a strict improvement, and then make follow-ups rather than a more complex change with higher risk of false positives.

Here are some suggestions from another round of AI review that seem reasonable to me. Note that one suggestion is to maybe shrink the scope a bit.

Keep the implementation small

Move enum lookup policy into alt/class/enums.rs, leaving a thin constructor hook. Match positional versus value= shape once, reuse the inferred argument type, avoid recomputing lookup eligibility for each constructor hint, and avoid enumerating members twice. Prefer an explicit prepared-argument representation over a (Type, range, bool) tuple.

Keep the existing rich diagnostic pipeline. If improving the synthetic-parameter wording, preserve nullable-narrowing hints, assert ... is not None quick fixes, outer context, subset explanations, and joint qualification of same-named enum classes. Reusing the existing typed diagnostic machinery is preferable to emitting a standalone error.

The goal is no new false positives, not complete modeling of dynamic equality.

Preserve custom-metaclass contextual inference

flatten_enum currently applies to every single-argument enum call, before the custom-metaclass bailout. For a metaclass whose __call__ accepts tuple[list[object]], E(([1],)) is incorrectly inferred as tuple[list[int]] and rejected. Gate eager inference on eligibility for the stock lookup check, and infer/check the argument once outside constructor hint retries. Leave custom contracts untouched.

Avoid nominal checks for unmodeled equality domains

These valid lookups are newly rejected:

  • An enum with ONE = Decimal("1"), called with 1.
  • An enum with ONE = Fraction(1), called with 1.0.
  • An enum with A = b"a", called with bytearray(b"a").
  • A NamedTuple-valued enum called with the equivalent plain tuple.

Restrict refinement to clearly supported primitive value domains and conservatively skip other domains. Account for ordinary cross-numeric and bytes/bytearray equality within the supported domains. Don't grow a denylist to approximate general equality.

Promote explicit literals for the lookup contract too

This should accept a str lookup argument, just as writing A = "a" directly does:

raw: Literal["a"] = "a"
class E(Enum):
    A = raw
def lookup(value: str) -> E:
    return E(value)

Promote the value literals only for lookup checking; retain literal precision for .value.

Treat broad TypeVars consistently with object

An unbounded T argument is rejected where object is deliberately accepted. Check the TypeVar's upper bound, but preserve any concrete quantified narrowing such as str & T; otherwise broad-bound acceptance can silently erase a useful incompatible-argument diagnostic.

Don't equate dataclass-transform metadata with runtime dataclasses

dataclass_metadata().is_some() includes transform-derived and attrs metadata. A transform-only method mixin can leave an enum's runtime value as an integer, while this change infers the mixin class and rejects a valid integer lookup. Prefer splitting the dataclass-mixin change into a separate follow-up. If retained, distinguish actual runtime dataclasses, respect the configured Python version, and test .value and auto() as well as lookup calls. There's no need to model the short-lived initializer-based behavior of early Python 3.11 patches.

This branch has not been deployed

No deployments
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.

Enum value lookup accepts incompatible argument types without a diagnostic

3 participants