[red-knot] Lookup of __new__
#17733
Merged
[red-knot] Lookup of __new__
#17733
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
sharkdp
changed the title
__new__ and __init__ calls__new__
sharkdp
force-pushed
the
david/new-and-init-calls
branch
2 times, most recently
from
3191359 to
d1e9046
Compare
sharkdp
force-pushed
the
david/new-and-init-calls
branch
from
d1e9046 to
e0cc310
Compare
dcreager
reviewed
Apr 30, 2025
crates/red_knot_python_semantic/resources/mdtest/call/constructor.md
Outdated
Show resolved
Hide resolved
|
|
||
| match new_method { | ||
| Symbol::Type(new_method, boundness) => { | ||
| let result = new_method.try_call(db, CallArgumentTypes::clone(argument_types)); |
Member
There was a problem hiding this comment.
nit: If it's not too invasive, can we update try_call to take in &mut CallArgumentTypes? That would let you avoid the clone here. (I did that in a previous PR to try_call_dunder_with_policy) It's probably not performance-critical, just seems like conceptually unnecessary work.
Contributor
Author
There was a problem hiding this comment.
I did that in 7768b9b. It's a bit awkward if you want to call it with a temporary CallArgumentTypes:
x.try_call(db, &mut CallArgumentTypes::positional(…)…)
I'm not entirely sure why the call API takes in mutable references to CallArgumentTypes in functions like Bindings::check_types etc.? For performance reasons.. since we want to make .with_self efficient? It looks a bit weird, because as a caller, I don't know if it's guaranteed that my call arguments are left unmodified.
What do you think?
Member
There was a problem hiding this comment.
For performance reasons.. since we want to make
.with_selfefficient?
That's right, so that we're not cloning the non-bound arguments each time we prepend a bound self.
It looks a bit weird, because as a caller, I don't know if it's guaranteed that my call arguments are left unmodified.
That's a good point. It does signify that we're using internal state in a way that isn't reentrant, but doesn't describe that it comes back to you unmodified. (Or rather, with any modifications reverted.)
I can see two other ways to handle with_self, either of which would drop the mut requirement:
-
Eat the cost of copying the non-bound arguments
-
Use something like
struct CallArgumentTypes { bound_self: Option<Type>, non_bound_arguments: Rc<[Type]>, }
I'd be fine with either of those modifications, but think that should be a separate PR, and that this one should try to remain consistent with our current handling.
Contributor
Author
There was a problem hiding this comment.
Ok, thanks. I'll note this down as a (lower priority) TODO for myself.
AlexWaygood
reviewed
Apr 30, 2025
crates/red_knot_python_semantic/resources/mdtest/call/constructor.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/call/constructor.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/call/constructor.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/call/constructor.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/call/constructor.md
Outdated
Show resolved
Hide resolved
AlexWaygood
reviewed
Apr 30, 2025
AlexWaygood
approved these changes
Apr 30, 2025
Member
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
This looks excellent, thank you
dcreager
added a commit
that referenced
this pull request
May 1, 2025
* main: [red-knot] Preliminary `NamedTuple` support (#17738) [red-knot] Add tests for classes that have incompatible `__new__` and `__init__` methods (#17747) Update dependency vite to v6.2.7 (#17746) [red-knot] Update call binding to return all matching overloads (#17618) [`airflow`] apply Replacement::AutoImport to `AIR312` (#17570) [`ruff`] Add fix safety section (`RUF028`) (#17722) [syntax-errors] Detect single starred expression assignment `x = *y` (#17624) py-fuzzer: fix minimization logic when `--only-new-bugs` is passed (#17739) Fix example syntax for pydocstyle ignore_var_parameters option (#17740) [red-knot] Update salsa to prevent panic in custom panic-handler (#17742) [red-knot] Ban direct instantiation of generic protocols as well as non-generic ones (#17741) [red-knot] Lookup of `__new__` (#17733) [red-knot] Check decorator consistency on overloads (#17684) [`flake8-use-pathlib`] Avoid suggesting `Path.iterdir()` for `os.listdir` with file descriptor (`PTH208`) (#17715) [red-knot] Check overloads without an implementation (#17681) Expand Semantic Syntax Coverage (#17725) [red-knot] Check for invalid overload usages (#17609)
sharkdp
added a commit
that referenced
this pull request
May 2, 2025
## Summary Remove mutability in parameter types for a few functions such as `with_self` and `try_call`. I tried the `Rc`-approach with cheap cloning [suggest here](#17733 (comment)) first, but it turns out we need a whole stack of prepended arguments (there can be [both `self` *and* `cls`](https://github.com/astral-sh/ruff/blob/3cf44e401a64658c17652cd3a17c897dc50261eb/crates/red_knot_python_semantic/resources/mdtest/call/constructor.md?plain=1#L113)), and we would need the same construct not just for `CallArguments` but also for `CallArgumentTypes`. At that point we're cloning `VecDeque`s anyway, so the overhead of cloning the whole `VecDeque` with all arguments didn't seem to justify the additional code complexity. ## Benchmarks Benchmarks on tomllib, black, jinja, isort seem neutral.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Model the lookup of
__new__without going throughType::try_call_dunder. The__new__method is only looked up on the constructed type itself, not on the meta-type.This now removes ~930 false positives across the ecosystem (vs 255 for #17662). It introduces 30 new false positives related to the construction of enums via something like
Color = enum.Enum("Color", ["RED", "GREEN"]). This is expected, because we don't handle custom metaclass__call__methods. The fact that we previously didn't emit diagnostics there was a coincidence (we incorrectly calledEnumMeta.__new__, and since we don't fully understand its signature, that happened to work withstr,listarguments).closes #17462
Test Plan
Regression test