<!--
Thank you for contributing to Ruff/ty! To help us out with reviewing,
please consider the following:
- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title? (Please prefix
with `[ty]` for ty pull
requests.)
- Does this pull request include references to any relevant issues?
-->
## Summary
<!-- What's the purpose of the change? What does it do, and why? -->
## Test Plan
<!-- How was it tested? -->
<!--
Thank you for contributing to Ruff/ty! To help us out with reviewing,
please consider the following:
- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title? (Please prefix
with `[ty]` for ty pull
requests.)
- Does this pull request include references to any relevant issues?
-->
## Summary
<!-- What's the purpose of the change? What does it do, and why? -->
## Test Plan
<!-- How was it tested? -->
<!--
Thank you for contributing to Ruff/ty! To help us out with reviewing,
please consider the following:
- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title? (Please prefix
with `[ty]` for ty pull
requests.)
- Does this pull request include references to any relevant issues?
-->
## Summary
<!-- What's the purpose of the change? What does it do, and why? -->
## Test Plan
<!-- How was it tested? -->
Summary
--
This PR fixes the issues revealed in #22744 by adding an additional
branch to
the lambda body formatting that checks if the body `needs_parentheses`
before
falling back on the `Parentheses::Never` case. I also updated the
`ExprNamed::needs_parentheses` implementation to match the one from
#8465.
Test Plan
--
New test based on the failing cases in #22744. I also checked out #22744
and
checked that the tests pass after applying the changes from this PR.
<!--
Thank you for contributing to Ruff/ty! To help us out with reviewing,
please consider the following:
- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title? (Please prefix
with `[ty]` for ty pull
requests.)
- Does this pull request include references to any relevant issues?
-->
## Summary
<!-- What's the purpose of the change? What does it do, and why? -->
## Test Plan
<!-- How was it tested? -->
<!--
Thank you for contributing to Ruff/ty! To help us out with reviewing,
please consider the following:
- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title? (Please prefix
with `[ty]` for ty pull
requests.)
- Does this pull request include references to any relevant issues?
-->
## Summary
<!-- What's the purpose of the change? What does it do, and why? -->
## Test Plan
<!-- How was it tested? -->
<!--
Thank you for contributing to Ruff/ty! To help us out with reviewing,
please consider the following:
- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title? (Please prefix
with `[ty]` for ty pull
requests.)
- Does this pull request include references to any relevant issues?
-->
## Summary
<!-- What's the purpose of the change? What does it do, and why? -->
## Test Plan
<!-- How was it tested? -->
I didn't dig into where this change came from, but 1) it's a small
change and 2) this one tends to be noisy since there are so many things
that match `array`.
The previous commits made this change deliciously simple. The key point
is that we don't collect a completion if it has no way of being ranked
into the top 1000 that we return.
Before:
```
$ ./target/profiling/ty_completion_bench ~/astral/relatedclones/scratch-home-assistant/homeassistant/scratch.py 1 -q --iters 30
total elapsed for initial completions request: 554.746713ms
total elapsed: 5.435318983s, time per completion request: 181.177299ms
After:
```
$ ./target/profiling/ty_completion_bench ~/astral/relatedclones/scratch-home-assistant/homeassistant/scratch.py 1 -q --iters 30
total elapsed for initial completions request: 526.743638ms
total elapsed: 4.268009725s, time per completion request: 142.26699ms
```
This is an especially nice speed-up for the cached case, where lots of
time (proportionally) was being spent sorting potentially a very large
list of completions.
There is potential future wins here. For example, every auto-import
completion has an `import` statement generated for it. We could instead
defer to that work until the very end when we convert the max-heap into
a `Vec`. Since before then, the completion might get dropped and there's
no point in doing extra work. But that will require more refactoring
(and possibly another `CompletionIntermediate` type? blech).
This is effectively what's going to happen when we switch to a max-heap
for collecting completions. This commit isolates that behavioral change
to ensure nothing unexpected happens. (I generally wouldn't expect it
to, since that would imply that imports/qualifications care about
results beyond the first 1,000.)
It turns out this wasn't actually doing much and was just papering over
the possibility of duplicates being returned from `SemanticModel`. So
this was doing a fair bit of work for no good reason.
Instead, we push deduplication down into semantic completions directly.
This will run over a (typically) much smaller number of completions.
With that said, this doesn't seem to lead to improvement in ad hoc
benchmarking. However, perf wasn't really the main motivation here: this
change is primarily to prep for switching to a max-heap. This
deduplication was problematic because it was being done *after* sorting,
which meant it dependend on having the entire set of completions in
memory. But with a max-heap, we'll only keep around the top-K
completions, so deduplication has to be done "earlier" (if at all).
This should be a non-behavior-changing commit that tweaks how we rank
completions. We now use a wrapper type and include the module name for
more precise sorting when relevance and symbol name are identical.
The purpose of this is to prepare the code to switch to a max-heap.
Instead of locking and unlocking the mutex for every symbol,
we gather what we can and only lock the mutex after processing
a module.
Before:
```
$ ./target/profiling/ty_completion_bench ~/astral/relatedclones/scratch-home-assistant/homeassistant/scratch.py 1 -q --iters 30
total elapsed for initial completions request: 579.057075ms
total elapsed: 5.627666455s, time per completion request: 187.588881ms
```
After:
```
$ ./target/profiling/ty_completion_bench ~/astral/relatedclones/scratch-home-assistant/homeassistant/scratch.py 1 -q --iters 30
total elapsed for initial completions request: 565.911487ms
total elapsed: 5.254306446s, time per completion request: 175.143548ms
```
This is a very naive/simplistic "improvement." We can probably do
better.
A surprising amount of time is spent sorting and comparing
completions. This commit does a very simple thing: it switches
the sorts to unstable and optimizes some comparisons to avoid
retrieving data we don't need for sorting.
Before:
```
$ ./target/profiling/ty_completion_bench ~/astral/relatedclones/scratch-home-assistant/homeassistant/scratch.py 1 -q --iters 30
total elapsed for initial completions request: 603.491595ms
total elapsed: 7.36554807s, time per completion request: 245.518269ms
```
After:
```
$ ./target/profiling/ty_completion_bench ~/astral/relatedclones/scratch-home-assistant/homeassistant/scratch.py 1 -q --iters 30
total elapsed for initial completions request: 570.040193ms
total elapsed: 5.740541847s, time per completion request: 191.351394ms
```
This is mostly just a stripped down version of
`ty_completion_eval`. Basically, you point it at
a Python file, give it a byte offset and it does
the rest. It also lets one repeat the completion
request after the initial request for benchmarking
the cached case.
Summary
--
I thought the fix unsafety example in the [rule
docs](https://docs.astral.sh/ruff/rules/unnecessary-builtin-import/#fix-safety)
looked a bit suspicious
while I was going through more potential default rules today
([playground](https://play.ruff.rs/f1a8b73d-6277-4414-b918-44bbba2863c2)):
```py
def str(x):
return x
from builtins import str
str(1) # `"1"` with the import, `1` without
```
Changing the behavior in this way seemed to go beyond fix unsafety and
into bug
territory. Sure enough, there was an existing bug report in #16182.
This PR fixes#16182 (and the fix safety example) by checking that the
builtin
import that the rule flags is actually shadowing a builtin binding. I
also left
an exception for `from builtins import *`, but we could consider
ignoring that
case too.
I initially tried reusing `SemanticModel::resolve_name` and
`only_binding`, but
they are specific to `ExprName`s because that seems to be all that is
inserted
into the `SemanticModel::resolved_names` map.
Test Plan
--
New tests based on #16182 and the fix safety docs
## Summary
Fixes https://github.com/astral-sh/ty/issues/2565.
This PR adds support for `if type(x) is Y` narrowing where `Y` is a
subclass-of type, type-alias type, or typevar type.
## Test Plan
mdtests
## Summary
fixes: https://github.com/astral-sh/ty/issues/1838
This PR adds basic support for overloaded function when used to
specialize a `ParamSpec` type variable.
Following cases are still remaining:
1. Updating the specialization with the matching overload after the
paramspec sub-call logic
2. Updating the specialization with the matching overload using the
return type
Both of these cases are present in the mdtest file.
## Test Plan
Update mdtest with new cases.
<!--
Thank you for contributing to Ruff/ty! To help us out with reviewing,
please consider the following:
- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title? (Please prefix
with `[ty]` for ty pull
requests.)
- Does this pull request include references to any relevant issues?
-->
## Summary
<!-- What's the purpose of the change? What does it do, and why? -->
## Test Plan
<!-- How was it tested? -->
<!--
Thank you for contributing to Ruff/ty! To help us out with reviewing,
please consider the following:
- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title? (Please prefix
with `[ty]` for ty pull
requests.)
- Does this pull request include references to any relevant issues?
-->
## Summary
<!-- What's the purpose of the change? What does it do, and why? -->
## Test Plan
<!-- How was it tested? -->
<!--
Thank you for contributing to Ruff/ty! To help us out with reviewing,
please consider the following:
- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title? (Please prefix
with `[ty]` for ty pull
requests.)
- Does this pull request include references to any relevant issues?
-->
## Summary
<!-- What's the purpose of the change? What does it do, and why? -->
## Test Plan
<!-- How was it tested? -->
<!--
Thank you for contributing to Ruff/ty! To help us out with reviewing,
please consider the following:
- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title? (Please prefix
with `[ty]` for ty pull
requests.)
- Does this pull request include references to any relevant issues?
-->
## Summary
<!-- What's the purpose of the change? What does it do, and why? -->
## Test Plan
<!-- How was it tested? -->
<!--
Thank you for contributing to Ruff/ty! To help us out with reviewing,
please consider the following:
- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title? (Please prefix
with `[ty]` for ty pull
requests.)
- Does this pull request include references to any relevant issues?
-->
## Summary
<!-- What's the purpose of the change? What does it do, and why? -->
## Test Plan
<!-- How was it tested? -->
<!--
Thank you for contributing to Ruff/ty! To help us out with reviewing,
please consider the following:
- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title? (Please prefix
with `[ty]` for ty pull
requests.)
- Does this pull request include references to any relevant issues?
-->
## Summary
<!-- What's the purpose of the change? What does it do, and why? -->
## Test Plan
<!-- How was it tested? -->
---------
Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
<!--
Thank you for contributing to Ruff/ty! To help us out with reviewing,
please consider the following:
- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title? (Please prefix
with `[ty]` for ty pull
requests.)
- Does this pull request include references to any relevant issues?
-->
## Summary
<!-- What's the purpose of the change? What does it do, and why? -->
## Test Plan
<!-- How was it tested? -->
<!--
Thank you for contributing to Ruff/ty! To help us out with reviewing,
please consider the following:
- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title? (Please prefix
with `[ty]` for ty pull
requests.)
- Does this pull request include references to any relevant issues?
-->
## Summary
<!-- What's the purpose of the change? What does it do, and why? -->
## Test Plan
<!-- How was it tested? -->
## Summary
Negative subscripts are also indicative of a thing being subcriptable:
```python
class Subscriptable:
def __getitem__(self, key: int) -> int:
return 42
class NotSubscriptable: ...
def _(x: list[Subscriptable | NotSubscriptable]):
if not isinstance(x[-1], NotSubscriptable):
# After narrowing, x[-1] excludes NotSubscriptable, which means subscripting works
reveal_type(x[-1]) # revealed: Subscriptable & ~NotSubscriptable
reveal_type(x[-1][0]) # revealed: int
```
## Summary
Fixes some TODOs introduced in #22672 around cases like the following:
```python
from collections import namedtuple
from dataclasses import dataclass
NT = namedtuple("NT", "x y")
# error: [invalid-dataclass] "Cannot use `dataclass()` on a `NamedTuple` class"
dataclass(NT)
```
On main, `dataclass(NT)` emits `# error: [no-matching-overload]`
instead, which is wrong -- the overload does match! On main, the logic
proceeds as follows:
1. `dataclass` has two overloads:
- `dataclass(cls: type[_T], ...) -> type[_T]`
- `dataclass(cls: None = None, ...) -> Callable[[type[_T]], type[_T]]`
2. When `dataclass(NT)` is called:
- Arity check: Both overloads accept one positional argument, so both
pass.
- Type checking on first overload: `NT` matches `type[_T]`... but then
`invalid_dataclass_target()` runs and adds `InvalidDataclassApplication`
error
- Type checking on second overload: `NT` doesn't match `None`, so we
have a type error.
3. After type checking, both overloads have errors.
4. `matching_overload_index()` filters by
`overload.as_result().is_ok()`, which checks if `errors.is_empty()`.
Since both overloads have errors, neither matches...
5. We emit the "No overload matches arguments" error.
Instead, we now differentiate between non-matching errors, and errors
that occur when we _do_ match, but the call has some other semantic
failure.
## Summary
Consider `x: str | bytes` and then `x.split(" ")`. Because we have a
union, and at least one variant errors (`bytes` expects a `Buffer`, not
a `str`), we call `binding.report_diagnostics` for each variant. For the
`str` variant, it has two overloads that both match arity, but only one
actually matches the signature... So
`matching_overload_before_type_checking` is `None` (because they both
match arity), but we don't actually have an error, and we fall through
to `NO_MATCHING_OVERLOAD`.
If one variant succeeds, we should avoid reporting errors for it, even
if not _all_ variants matched.
