-
Notifications
You must be signed in to change notification settings - Fork 112
Add check for dataclass field default ordering #583
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add check for dataclass field default ordering #583
Conversation
… field with default
…e-dataclass-field-order
…rrides + defaults
These test cases rely on issue facebook#493 being fixed.
Let me know if there's a better place to put the validation logic, e.g., between lines 66-67. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks great overall! Just a couple minor comments.
"#, | ||
); | ||
|
||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these tests commented out? If they're broken, our convention is to add # E:
comments matching the current behavior, plus a bug = ...
line describing why the current behavior is buggy. Here's an example:
pyrefly/pyrefly/lib/test/simple.rs
Lines 1494 to 1503 in 747a4bc
testcase!( | |
bug = "Mapping.get with a default gives the wrong answer", | |
test_mapping_get, | |
r#" | |
from typing import assert_type | |
import os | |
compiler = os.environ.get("CXX", "cl") | |
assert_type(compiler, str) # E: assert_type(str | Any, str) failed | |
"#, | |
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really thorough set of test cases, thank you!
error::kind::ErrorKind::BadClassDefinition, | ||
None, | ||
format!( | ||
"DataClass field '{}' without a default may not follow DataClass field with a default", name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: dataclass is usually written all lower-case.
"DataClass field '{}' without a default may not follow DataClass field with a default", name | |
"Dataclass field '{}' without a default may not follow dataclass field with a default", name |
This PR introduces a validation step to raise an error when a dataclass field without a default follows a field with a default, addressing issue #492.
This ensures dataclasses are declared in the correct field order, aligning with Python's rules for
@dataclass
.The core change involves modifying the
get_dataclass_init
function when getting the dataclass synthesized fields to perform an ordering validation check (similar to #410). Due to the additional properties of a dataclass, additional checks forinit
,kw_only
, anddefault
have to be made.Separately, certain test cases that I've added rely on issue #493 being fixed. For now, I have commented those out - and will uncomment and verify them once the aforementioned issue has been resolved.