Skip to content

Commit

Permalink
support enum _value_
Browse files Browse the repository at this point in the history
Summary:
This diff implements typechecking for enum member values in the solver for BindingClassField.

The rules implemented are:
1. enum members can't be annotated
2. members should be checked against any explicit annotation for `_value_`

There are two special cases:
- attributes that aren't initialized on the class are considered non-members
- attributes initialized with a tuple, it's unpacked and passed to the constructor. right now we ignore tuples, but the typing spec mentions that handling it is optional.

https://typing.readthedocs.io/en/latest/spec/enums.html#member-values

Reviewed By: stroxler

Differential Revision: D68020271

fbshipit-source-id: c5e9abff71c8a495a3fd1fcc0bb2b49af7dbc4d4
  • Loading branch information
yangdanny97 authored and facebook-github-bot committed Jan 10, 2025
1 parent 2a4e271 commit 28a2ec9
Show file tree
Hide file tree
Showing 8 changed files with 116 additions and 29 deletions.
20 changes: 20 additions & 0 deletions pyre2/conformance/third_party/conformance.exp
Original file line number Diff line number Diff line change
Expand Up @@ -8854,6 +8854,16 @@
"stop_column": 42,
"stop_line": 68
},
{
"code": -2,
"column": 5,
"concise_description": "The value for enum member `GREEN` must match the annotation of the _value_ attribute.",
"description": "The value for enum member `GREEN` must match the annotation of the _value_ attribute.",
"line": 78,
"name": "PyreError",
"stop_column": 10,
"stop_line": 78
},
{
"code": -2,
"column": 24,
Expand Down Expand Up @@ -8996,6 +9006,16 @@
"stop_column": 39,
"stop_line": 38
},
{
"code": -2,
"column": 5,
"concise_description": "Enum member `DOG` may not be annotated directly. Instead, annotate the _value_ attribute.",
"description": "Enum member `DOG` may not be annotated directly. Instead, annotate the _value_ attribute.",
"line": 50,
"name": "PyreError",
"stop_column": 8,
"stop_line": 50
},
{
"code": -2,
"column": 27,
Expand Down
2 changes: 0 additions & 2 deletions pyre2/conformance/third_party/conformance.result
Original file line number Diff line number Diff line change
Expand Up @@ -616,11 +616,9 @@
],
"enums_member_names.py": [],
"enums_member_values.py": [
"Line 78: Expected 1 errors",
"Line 90: Unexpected errors ['Could not find import of `_enums_member_values`, Could not find path for `_enums_member_values`']"
],
"enums_members.py": [
"Line 50: Expected 1 errors",
"Line 82: Expected 1 errors",
"Line 83: Expected 1 errors",
"Line 84: Expected 1 errors",
Expand Down
6 changes: 3 additions & 3 deletions pyre2/conformance/third_party/results.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"pass": 9,
"fail": 124,
"pass_rate": 0.07,
"differences": 1421,
"differences": 1419,
"passing": [
"annotations_coroutines.py",
"directives_no_type_check.py",
Expand Down Expand Up @@ -62,8 +62,8 @@
"directives_type_ignore_file1.py": 1,
"enums_behaviors.py": 3,
"enums_expansion.py": 7,
"enums_member_values.py": 2,
"enums_members.py": 18,
"enums_member_values.py": 1,
"enums_members.py": 17,
"exceptions_context_managers.py": 4,
"generics_base_class.py": 3,
"generics_basic.py": 14,
Expand Down
32 changes: 27 additions & 5 deletions pyre2/pyre2/bin/alt/answers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -974,19 +974,41 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
)
}

fn solve_class_field(&self, binding: &BindingClassField) -> Arc<ClassField> {
let ty = if let Some(ann) = &binding.1 {
fn solve_class_field(&self, field: &BindingClassField) -> Arc<ClassField> {
let value_ty = self.solve_binding(&field.value);
if let Some(enum_) = self.get_enum_from_key(field.class.to_owned())
&& enum_.get_member(&field.name).is_some()
&& matches!(field.initialization, ClassFieldInitialization::Class)
{
if field.annotation.is_some() {
self.error(field.range, format!("Enum member `{}` may not be annotated directly. Instead, annotate the _value_ attribute.", field.name));
}
if let Some(enum_value_attr) = self
.get_class_attribute_with_targs(enum_.class_type(), &Name::new_static("_value_"))
{
if !matches!(*value_ty, Type::Tuple(_))
&& !self.solver().is_subset_eq(
&value_ty,
&enum_value_attr.value,
self.type_order(),
)
{
self.error(field.range, format!("The value for enum member `{}` must match the annotation of the _value_ attribute.", field.name));
}
}
}
let ty = if let Some(ann) = &field.annotation {
let ann = self.get_idx(*ann);
match &ann.ty {
Some(ty) => Arc::new(ty.clone()),
None => self.solve_binding(&binding.0),
None => value_ty,
}
} else {
self.solve_binding(&binding.0)
value_ty
};
Arc::new(ClassField {
ty: ty.deref().clone(),
initialization: binding.2,
initialization: field.initialization,
})
}

Expand Down
13 changes: 12 additions & 1 deletion pyre2/pyre2/bin/alt/classes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,10 @@ impl Enum {
None
}
}

pub fn class_type(&self) -> &ClassType {
&self.0
}
}

fn is_unbound_function(ty: &Type) -> bool {
Expand Down Expand Up @@ -683,10 +687,17 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
/// Given an identifier, see whether it is bound to an enum class. If so,
/// return the enum, otherwise return `None`.
pub fn get_enum_from_name(&self, name: Identifier) -> Option<Enum> {
self.get_enum_from_key(
self.bindings()
.key_to_idx(&Key::Usage(ShortIdentifier::new(&name))),
)
}

pub fn get_enum_from_key(&self, key: Idx<Key>) -> Option<Enum> {
// TODO(stroxler): Eventually, we should raise type errors on generic Enum because
// this doesn't make semantic sense. But in the meantime we need to be robust against
// this possibility.
match self.get(&Key::Usage(ShortIdentifier::new(&name))).deref() {
match self.get_idx(key).deref() {
Type::ClassDef(class) => self.get_enum(&self.promote_to_class_type_silently(class)),
_ => None,
}
Expand Down
17 changes: 10 additions & 7 deletions pyre2/pyre2/bin/binding/binding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ assert_eq_size!(KeyLegacyTypeParam, [usize; 1]);
assert_eq_size!(Binding, [usize; 9]);
assert_eq_size!(BindingAnnotation, [usize; 9]);
assert_eq_size!(BindingClassMetadata, [usize; 8]);
assert_eq_size!(BindingClassField, [usize; 10]);
assert_eq_size!(BindingClassField, [usize; 15]);
assert_eq_size!(BindingLegacyTypeParam, [u32; 1]);

pub trait Keyed: Hash + Eq + Clone + DisplayWith<ModuleInfo> + Debug + Ranged + 'static {
Expand Down Expand Up @@ -637,15 +637,18 @@ pub enum ClassFieldInitialization {
/// either the class body or in method (like `__init__`) that we recognize as
/// defining instance attributes.
#[derive(Clone, Debug)]
pub struct BindingClassField(
pub Binding,
pub Option<Idx<KeyAnnotation>>,
pub ClassFieldInitialization,
);
pub struct BindingClassField {
pub class: Idx<Key>,
pub name: Name,
pub value: Binding,
pub annotation: Option<Idx<KeyAnnotation>>,
pub range: TextRange,
pub initialization: ClassFieldInitialization,
}

impl DisplayWith<Bindings> for BindingClassField {
fn fmt(&self, f: &mut fmt::Formatter<'_>, ctx: &Bindings) -> fmt::Result {
write!(f, "class field {}", self.0.display_with(ctx))
write!(f, "class field {}", self.value.display_with(ctx))
}
}

Expand Down
33 changes: 22 additions & 11 deletions pyre2/pyre2/bin/binding/bindings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ impl ClassBodyInner {
/// Information about an instance attribute coming from a `self` assignment
/// in a method.
#[derive(Clone, Debug)]
struct InstanceAttribute(Binding, Option<Idx<KeyAnnotation>>);
struct InstanceAttribute(Binding, Option<Idx<KeyAnnotation>>, TextRange);

#[derive(Clone, Debug)]
struct MethodInner {
Expand Down Expand Up @@ -843,9 +843,10 @@ impl<'a> BindingsBuilder<'a> {
&& matches!(&*x.value, Expr::Name(name) if name.id == self_name.id)
{
if !method.instance_attributes.contains_key(&x.attr.id) {
method
.instance_attributes
.insert(x.attr.id.clone(), InstanceAttribute(binding, annotation));
method.instance_attributes.insert(
x.attr.id.clone(),
InstanceAttribute(binding, annotation, x.attr.range()),
);
}
return true;
}
Expand Down Expand Up @@ -1267,7 +1268,14 @@ impl<'a> BindingsBuilder<'a> {
} else {
ClassFieldInitialization::Instance
};
let binding = BindingClassField(flow_type, info.ann.dupe(), initialization);
let binding = BindingClassField {
class: definition_key,
name: name.clone(),
value: flow_type,
annotation: info.ann.dupe(),
range: stat_info.loc,
initialization,
};
fields.insert(name.clone());
self.table.insert(
KeyClassField(ShortIdentifier::new(&x.name), name.clone()),
Expand All @@ -1282,12 +1290,15 @@ impl<'a> BindingsBuilder<'a> {
if !fields.contains(&name) {
fields.insert(name.clone());
self.table.insert(
KeyClassField(ShortIdentifier::new(&x.name), name),
BindingClassField(
attribute.0,
attribute.1,
ClassFieldInitialization::Instance,
),
KeyClassField(ShortIdentifier::new(&x.name), name.clone()),
BindingClassField {
class: definition_key,
name,
value: attribute.0,
annotation: attribute.1,
range: attribute.2,
initialization: ClassFieldInitialization::Instance,
},
);
}
}
Expand Down
22 changes: 22 additions & 0 deletions pyre2/pyre2/bin/test/enums.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,25 @@ for e in E:
assert_type(e, E)
"#,
);

testcase!(
test_value_annotation,
r#"
from enum import Enum
class MyEnum(Enum):
_value_: int
X = 1
Y = "FOO" # E: The value for enum member `Y` must match the annotation of the _value_ attribute.
"#,
);

testcase!(
test_member_annotation,
r#"
from enum import Enum
class MyEnum(Enum):
X: int = 1 # E: Enum member `X` may not be annotated directly. Instead, annotate the _value_ attribute.
"#,
);

0 comments on commit 28a2ec9

Please sign in to comment.