Repository navigation
fix(enums): reject incompatible Enum value-lookup arguments - #5417
mangeshraut712 wants to merge 15 commits into
Conversation
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
|
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.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
|
All checks are green (tests, mypy_primer, CLA, import). This is ready for a maintainer pass; I can iterate quickly on review notes. |
stroxler
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
stroxler
left a comment
There was a problem hiding this comment.
Thanks! This could be a good improvement, but I think there are two underlying problems based on AI review.
- Member construction and value lookup have different contracts.
__init__describes creating members during class definition; it does not run when callingMyEnum(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.
- 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
|
@stroxler thanks for the review. I pushed changes for both points.
I added regression tests for every case you listed: tuple values with a multi-parameter |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@stroxler Thanks, this round's notes all checked out. Pushed c0eed6b and 189e3b3, then merged main (14ff662).
Tests for every case are in |
|
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] |
|
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
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
There was a problem hiding this comment.
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 with1. - An enum with
ONE = Fraction(1), called with1.0. - An enum with
A = b"a", called withbytearray(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.
Summary
EnumMeta.__call__annotates itsvalueparameter asAny, so value-lookupcalls like
Category(None)against an int-valued enum produced no type erroreven when a class-defined
__init__(or the inferred member value type) wouldclearly 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 parameteragainst that signature; otherwise fall back to the non-
Anymember value typefrom
enum_value_lookup_on_class. Member construction via__new__is leftalone — that is class-definition time, not value lookup (see
SeFileType).Test plan
test_enum_call_rejects_incompatible_value— custom__init__rejectsNonetest_enum_call_rejects_none_for_int_members— inferredLiteral[1, 2]rejectsNonetest_enum_call*(incl. SeFileType metaclass/self) still passFixes #5398