[ty] Fix __setattr__ call check precedence during attribute assignment#18347
[ty] Fix __setattr__ call check precedence during attribute assignment#18347sharkdp merged 6 commits intoastral-sh:mainfrom
Conversation
|
thejchap
force-pushed
the
feature/setattr-return-type
branch
4 times, most recently
from
5172ba6 to
22620d5
Compare
thejchap
changed the title
|
@sharkdp this is ready for a look, left a few notes in the description |
|
re: this comment/example from #17974:
from typing import Never
class Frozen:
existing: int = 1
def __setattr__(self, name, value) -> Never:
raise AttributeError("Attributes can not be modified")
instance = Frozen()
instance.non_existing = 2 # fails at runtime
instance.existing = 2 # also fails at runtimei think this would be different if it were actually a frozen dataclass - in that case it should be: from typing import Never
from dataclasses import dataclass
@dataclass(frozen=True)
class Frozen:
existing: int = 1
instance = Frozen()
instance.non_existing = 2 # unresolved attribute
instance.existing = 2 # invalid assignmentor at least that's how mypy handles it if that's the case, then this first implementation won't work - we'd get back an invalid-assignment for seems like the logic would need to be something like:
|
## Summary Came across this while debugging some ecosystem changes in #18347. I think the meta-type of a typevar-annotated variable should be equal to `type`, not `<class 'object'>`. ## Test Plan New Markdown tests.
sharkdp
force-pushed
the
feature/setattr-return-type
branch
from
22620d5 to
4265911
Compare
|
Thank you for working on this! I think that we need to understand the ecosystem changes more closely and write some tests here. While taking a look at the |
| self.context.report_lint(&INVALID_ASSIGNMENT, target) | ||
| { | ||
| builder.into_diagnostic(format_args!( | ||
| Err(CallDunderError::PossiblyUnbound(_)) => true, |
There was a problem hiding this comment.
I think this might be causing the changes in beartype, which look surprising to me. Do we need to change this branch, now that we call __setattr__ unconditionally? We should probably add some tests with unions of classes that do/don't have a __setattr__ method.
There was a problem hiding this comment.
i will add some tests with unions and look into this a little more - but also, did this comment/concern make sense at all? i just want to confirm that this is the right approach generally (doing the unconditional call)
doing the __setattr__ call on every attribute assignment would result in (i think) the wrong diagnostics in this example:
from typing import Never
from dataclasses import dataclass
@dataclass(frozen=True)
class Frozen:
existing: int = 1
instance = Frozen()
instance.non_existing = 2 # would expect unresolved attribute, but unconditional call would result in invalid assignment
instance.existing = 2 # invalid assignment as expectedThere was a problem hiding this comment.
Yes, I think it's good to bring this up, but I think we can handle this.
For an arbitrary class with a __setattr__ method, we don't know what the body of that custom __setattr__ method does. So I think it's fine to unconditionally call it, see if it returns Never (there could be multiple overloads and maybe it only returns Never conditionally — it would be nice to add a test for this), and show the invalid-assignment diagnostic if that's the case.
On the other hand, for synthesized __setattr__ methods of frozen dataclasses, we understand how the body looks like. It returns an AttributeError if the attribute doesn't exist, and it returns a FrozenInstanceError if it's an existing attribute. It seems reasonable to emit "unresolved attribute" for the first case, and invalid-assignment (potentially with a customized error message for frozen instances) in the second case.
sounds good! thanks - i will investigate the remaining changes after your rebase
🤦♂️ thanks - was a little hasty there - appreciate that... |
Please let me know if you need help with this. |
thejchap
force-pushed
the
feature/setattr-return-type
branch
from
4265911 to
ee5be7e
Compare
thejchap
force-pushed
the
feature/setattr-return-type
branch
from
ee5be7e to
e504514
Compare
sharkdp
left a comment
sharkdp
left a comment
There was a problem hiding this comment.
Sorry that this took so long. I brought this up to date, made a few modifications and added some new tests. I also reviewed the ecosystem changes and except for the beartype thing which I don't fully understand, I think the changes are good.
Thank you very much!
|
@sharkdp thank you! |
Of course, thank you for your contribution. Are you interested in following this up with a modified version of https://github.com/thejchap/ruff/pull/2/files? If not, that's also completely fine. |
|
@sharkdp yep, I'll revisit that |
## Summary Synthesize a `__setattr__` method with a return type of `Never` for frozen dataclasses. https://docs.python.org/3/library/dataclasses.html#frozen-instances https://docs.python.org/3/library/dataclasses.html#dataclasses.FrozenInstanceError ### Related astral-sh/ty#111 #17974 (comment) #18347 (comment) ## Test Plan New Markdown tests --------- Co-authored-by: David Peter <mail@david-peter.de>
3 participants
Summary
Related:
Previously, when validating an attribute assignment, a
__setattr__call check was only done if the attribute wasn't found as either a class member or instance memberThis PR changes the
__setattr__call check to be attempted first, prior to the "normal mechanism", as a defined__setattr__should take precedence over setting an attribute on the instance dictionary directly.if the return type of
__setattr__isNever, aninvalid-assignmentdiagnostic is emittedOnce this is merged, a subsequent PR will synthesize a
__setattr__method with aNeverreturn type for frozen dataclasses.Ecosystem changes
it looks like support needs to be added for writing special attributes on
Callables (at least I wasn't able to find tests that asserted these are supported). These diagnostics are getting emitted due to aCallDunderError::PossiblyUnbound.This seems like an expected side-effect given the change in this PR - would it make sense to add a separate issue to track this?
it's not obvious to me that the other ecosystem changes are related to this PR - would need to understand better how the comparison is happening
Test Plan
Existing tests + mypy_primer