-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Improve generic call expression inference #21210
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
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
09d2478 to
2e13754
Compare
|
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-assignment |
4 | 3 | 0 |
invalid-argument-type |
0 | 3 | 0 |
invalid-return-type |
0 | 1 | 2 |
possibly-missing-attribute |
0 | 3 | 0 |
no-matching-overload |
2 | 0 | 0 |
unsupported-operator |
0 | 2 | 0 |
index-out-of-bounds |
0 | 1 | 0 |
non-subscriptable |
0 | 1 | 0 |
unused-ignore-comment |
0 | 1 | 0 |
| Total | 6 | 15 | 2 |
CodSpeed Performance ReportMerging #21210 will degrade performances by 7.82%Comparing Summary
Benchmarks breakdown
Footnotes
|
9ad8d67 to
67143b9
Compare
67143b9 to
24e37e9
Compare
24e37e9 to
54dec10
Compare
54dec10 to
e967176
Compare
e967176 to
0dc3902
Compare
0dc3902 to
a0144fd
Compare
|
I imagine we could avoid the performance regression by special casing Also note that a lot of the diagnostic improvements won't be seen in this PR directly, but #20933 (which is currently stacked on top of this PR). |
MichaReiser
left a comment
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.
Do we understand what's causing the performance regression? Is it that we need to clone bindings or is it something else?
0f6ddfb to
2b79ab0
Compare
2b79ab0 to
0629a65
Compare
|
Most of the performance regressions have been resolved. There is an unavoidable regression when assigning the result of a generic call to a union, as we have to attempt inference multiple times for each element of the union. |
|
Can you take a look at the new diagnostics in the ecosystem report? Just taking a quick glance, I see in HomeAssistant a few occurrences of "object of type |
|
It looks like those failures are due to I added a failing test to track those. |
a786a14 to
46b938b
Compare
46b938b to
966b438
Compare
AlexWaygood
left a comment
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.
I'm not too familiar with this machinery, but from what I can tell this all looks reasonable to me, and I definitely approve of all of the semantics changes I see in the tests! This is, as ever, very cool work
crates/ty_python_semantic/resources/mdtest/assignment/annotations.md
Outdated
Show resolved
Hide resolved
crates/ty_python_semantic/resources/mdtest/assignment/annotations.md
Outdated
Show resolved
Hide resolved
carljm
left a comment
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.
Oops, I had a couple draft comments from a review I started on Friday but hadn't completed yet. Nothing important, just things to consider for future.
| python-version = "3.12" | ||
| ``` | ||
|
|
||
| `generic_list.py`: |
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: should these just be another heading layer? Our usual approach is to reserve filenames for tests that actually need to be multi-file (because they are testing cross-module behavior / imports) and use headings when we are just trying to group related tests together.
| return [x] | ||
|
|
||
| a = f(1) | ||
| reveal_type(a) # revealed: list[Literal[1]] |
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.
We should probably do literal promotion here?
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.
that might be fixed in #21320 :-)
## Summary Resolves astral-sh/ty#1228. This PR is stacked on #21210.
* origin/main: (38 commits) [ty] Make implicit submodule imports only occur in global scope (#21370) [ty] introduce local variables for `from` imports of submodules in `__init__.py(i)` (#21173) [`ruff`] Ignore `str()` when not used for simple conversion (`RUF065`) (#21330) [ty] implement `typing.NewType` by adding `Type::NewTypeInstance` [ty] supress inlay hints for `+1` and `-1` (#21368) [ty] Use type context for inference of generic constructors (#20933) [ty] Improve generic call expression inference (#21210) [ty] supress some trivial expr inlay hints (#21367) [`configuration`] Fix unclear error messages for line-length values exceeding `u16::MAX` (#21329) [ty] Fix incorrect inference of `enum.auto()` for enums with non-`int` mixins, and imprecise inference of `enum.auto()` for single-member enums (#20541) [`refurb`] Detect empty f-strings (`FURB105`) (#21348) [ty] provide `import` completion when in `from <name> <name>` statement (#21291) [ty] elide redundant inlay hints for function args (#21365) Fix syntax error false positive on alternative `match` patterns (#21362) Add a new "Opening a PR" section to the contribution guide (#21298) [`flake8-simplify`] Fix SIM222 false positive for `tuple(generator) or None` (`SIM222`) (#21187) Rebuild ruff binary instead of sharing it across jobs (#21361) [ty] Fix `--exclude` and `src.exclude` merging (#21341) [ty] Add support for properties that return `Self` (#21335) Add upstream linter URL to `ruff linter --output-format=json` (#21316) ...
Summary
Implements astral-sh/ty#1356 and astral-sh/ty#136 (comment).