Skip to content

Commit 350bac2

Browse files
yangdanny97meta-codesync[bot]
authored andcommitted
Preserve literal member type in enums
Summary: Instead of promoting away the enum member's literal type, we preserve the unpromoted literal & change VisitMut to skip it. Then, in alt/class/enums.rs we modify some logic to decide when to promote away the literal & when to keep it. The special enum attr resolution is weirdly complex because it deals with a bunch of different conditions like django and mixins. I'm not a big fan of how complex it is but it seems necessary. closes facebook#2741 Reviewed By: grievejia Differential Revision: D96157102 fbshipit-source-id: 69ae410dc1afcec9e3a4c290f7724a6560bdb38f
1 parent 0562bf5 commit 350bac2

6 files changed

Lines changed: 289 additions & 131 deletions

File tree

conformance/third_party/conformance.exp

Lines changed: 2 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -5075,39 +5075,6 @@
50755075
}
50765076
],
50775077
"enums_member_values.py": [
5078-
{
5079-
"code": -2,
5080-
"column": 12,
5081-
"concise_description": "assert_type(int, Literal[1]) failed",
5082-
"description": "assert_type(int, Literal[1]) failed",
5083-
"line": 21,
5084-
"name": "assert-type",
5085-
"severity": "error",
5086-
"stop_column": 43,
5087-
"stop_line": 21
5088-
},
5089-
{
5090-
"code": -2,
5091-
"column": 12,
5092-
"concise_description": "assert_type(int, Literal[1]) failed",
5093-
"description": "assert_type(int, Literal[1]) failed",
5094-
"line": 22,
5095-
"name": "assert-type",
5096-
"severity": "error",
5097-
"stop_column": 41,
5098-
"stop_line": 22
5099-
},
5100-
{
5101-
"code": -2,
5102-
"column": 16,
5103-
"concise_description": "assert_type(int, Literal[1, 3]) failed",
5104-
"description": "assert_type(int, Literal[1, 3]) failed",
5105-
"line": 26,
5106-
"name": "assert-type",
5107-
"severity": "error",
5108-
"stop_column": 50,
5109-
"stop_line": 26
5110-
},
51115078
{
51125079
"code": -2,
51135080
"column": 16,
@@ -5144,8 +5111,8 @@
51445111
{
51455112
"code": -2,
51465113
"column": 5,
5147-
"concise_description": "Enum member `GREEN` has type `str`, must match the `_value_` attribute annotation of `int`",
5148-
"description": "Enum member `GREEN` has type `str`, must match the `_value_` attribute annotation of `int`",
5114+
"concise_description": "Enum member `GREEN` has type `Literal['green']`, must match the `_value_` attribute annotation of `int`",
5115+
"description": "Enum member `GREEN` has type `Literal['green']`, must match the `_value_` attribute annotation of `int`",
51495116
"line": 78,
51505117
"name": "bad-assignment",
51515118
"severity": "error",

crates/pyrefly_types/src/literal.rs

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ use pyrefly_derive::TypeEq;
1313
use pyrefly_derive::Visit;
1414
use pyrefly_derive::VisitMut;
1515
use pyrefly_util::assert_words;
16+
use pyrefly_util::visit::VisitMut;
1617
use ruff_python_ast::ExprBooleanLiteral;
1718
use ruff_python_ast::ExprBytesLiteral;
1819
use ruff_python_ast::ExprFString;
@@ -57,15 +58,27 @@ pub enum Lit {
5758
}
5859

5960
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
60-
#[derive(Visit, VisitMut, TypeEq)]
61+
#[derive(Visit, TypeEq)]
6162
pub struct LitEnum {
6263
pub class: ClassType,
6364
pub member: Name,
6465
/// Raw type assigned to name in class def.
6566
/// We store the raw type so we can return it when the value or _value_ attribute is accessed.
67+
/// NOTE: Intentionally excluded from VisitMut so that type transformations like
68+
/// `promote_implicit_literals` don't mutate this metadata field, which must remain
69+
/// stable for `.value` resolution.
6670
pub ty: Type,
6771
}
6872

73+
/// Manual VisitMut that skips the `ty` field. The `ty` is semantic metadata for `.value`
74+
/// resolution and must not be mutated by recursive type transformations (e.g. literal promotion).
75+
/// We still recurse into `class` so that type arguments on generic enums are visited.
76+
impl VisitMut<Type> for LitEnum {
77+
fn recurse_mut(&mut self, f: &mut dyn FnMut(&mut Type)) {
78+
self.class.visit_mut(f);
79+
}
80+
}
81+
6982
impl Display for Lit {
7083
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
7184
match self {

pyrefly/lib/alt/class/class_field.rs

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1762,25 +1762,27 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
17621762
// Determine the final type, promoting literals when appropriate.
17631763
// Skip literal promotion for NNModule types: their fields are captured
17641764
// constructor args that must preserve literal types for shape inference.
1765-
let ty = if matches!(value_ty, Type::NNModule(_)) {
1766-
value_ty
1765+
let (ty, unpromoted_ty) = if matches!(value_ty, Type::NNModule(_)) {
1766+
(value_ty, None)
17671767
} else {
17681768
let mut has_implicit_literal = value_ty.is_implicit_literal();
17691769
if !has_implicit_literal && matches!(initialization, ClassFieldInitialization::Method) {
17701770
value_ty.universe(&mut |current_type_node| {
17711771
has_implicit_literal |= current_type_node.is_implicit_literal();
17721772
});
17731773
}
1774+
// Save any unpromoted literal types, we need them for enums
17741775
if annotation
17751776
.as_ref()
17761777
.and_then(|ann| ann.ty.as_ref())
17771778
.is_none()
17781779
&& matches!(read_only_reason, None | Some(ReadOnlyReason::NamedTuple))
17791780
&& has_implicit_literal
17801781
{
1781-
value_ty.promote_implicit_literals(self.stdlib)
1782+
let pre = value_ty.clone();
1783+
(value_ty.promote_implicit_literals(self.stdlib), Some(pre))
17821784
} else {
1783-
value_ty
1785+
(value_ty, None)
17841786
}
17851787
};
17861788

@@ -1864,6 +1866,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
18641866
name,
18651867
direct_annotation.as_ref(),
18661868
&ty,
1869+
unpromoted_ty.as_ref(),
18671870
field_definition,
18681871
descriptor.is_some(),
18691872
range,
@@ -2105,6 +2108,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
21052108
name: &Name,
21062109
direct_annotation: Option<&Annotation>,
21072110
ty: &Type,
2111+
unpromoted_ty: Option<&Type>,
21082112
field_definition: &ClassFieldDefinition,
21092113
is_descriptor: bool,
21102114
range: TextRange,
@@ -2114,7 +2118,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
21142118
class,
21152119
name,
21162120
direct_annotation,
2117-
ty,
2121+
unpromoted_ty.unwrap_or(ty),
21182122
field_definition,
21192123
is_descriptor,
21202124
range,
@@ -3993,11 +3997,21 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
39933997
field: &Name,
39943998
ancestor: (&str, &str),
39953999
) -> bool {
3996-
let member = self.get_class_member_with_defining_class(cls, field);
3997-
match member {
3998-
Some(member) => member.is_defined_on(ancestor.0, ancestor.1),
3999-
None => false,
4000-
}
4000+
self.field_defining_class_matches(cls, field, |c| {
4001+
c.has_toplevel_qname(ancestor.0, ancestor.1)
4002+
})
4003+
}
4004+
4005+
/// Check whether the defining class of `field` on `cls` satisfies `predicate`.
4006+
/// Returns false if the field does not exist.
4007+
pub fn field_defining_class_matches(
4008+
&self,
4009+
cls: &Class,
4010+
field: &Name,
4011+
predicate: impl FnOnce(&Class) -> bool,
4012+
) -> bool {
4013+
self.get_class_member_with_defining_class(cls, field)
4014+
.is_some_and(|member| predicate(&member.defining_class))
40014015
}
40024016

40034017
/// Get the class's `__new__` method.

0 commit comments

Comments
 (0)