-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Improve invalid-argument-type diagnostics where a union type was provided
#21044
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 ✅ |
|
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.
The secondary info message won't be shown with --output-format=concise or in the main diagnostic shown in editors (unless they supported related information which many dont).
Given that this is the most important bit, I'm inclined to make it part of the primary annotation message, although that's somewhat tricky
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 think we may have had this conversation before... I think we may need to rethink our diagnostic model here. It's often not feasible to encapsulate all relevant information in a single line, and mypy, for example, will often display secondary diagnostics even if you do not specify --pretty. (Mypy uses a concise output format by default; --pretty gives you annotations as part of diagnostics, similar to our output format.) E.g. https://mypy-play.net/?mypy=latest&python=3.12&gist=8322703266ffc5e8e2ccb9538e2487c3
The approach I'm taking here is also exactly what we already do in similar diagnostics, so I'm inclined to say that if we want to change this approach, we should do so for all our diagnostics together:
Lines 34 to 53 in 0169551
| error[invalid-argument-type]: Argument to function `f2` is incorrect | |
| --> src/mdtest_snippet.py:14:11 | |
| | | |
| 12 | # error: [too-many-positional-arguments] | |
| 13 | # error: [invalid-argument-type] | |
| 14 | x = f(3) | |
| | ^ Expected `str`, found `Literal[3]` | |
| | | |
| info: Function defined here | |
| --> src/mdtest_snippet.py:4:5 | |
| | | |
| 2 | return 0 | |
| 3 | | |
| 4 | def f2(name: str) -> int: | |
| | ^^ --------- Parameter declared here | |
| 5 | return 0 | |
| | | |
| info: Union variant `def f2(name: str) -> int` is incompatible with this call site | |
| info: Attempted to call union type `(def f1() -> int) | (def f2(name: str) -> int)` | |
| info: rule `invalid-argument-type` is enabled by default |
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.
It's probably worth changing it everywhere because using sub diagnostics with info won't show with --output-format=concise or in the editor. The reason I opened this issue was because I didn't had enough information in the editor to fix the diagnostic.
I'd be inclined to change the message to:
Expected `Sized`, element `Foo` of `str | Foo` is not assignable to `Sized`
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.
using sub diagnostics with info won't show with
--output-format=concise
yes, that's the thing I'm suggesting we might want to rethink when I said "we may need to rethink our diagnostic model here". I don't think that's a great outcome. I think it's going to end up with worse diagnostics than mypy for CLI users. It's often silly to try to cram all relevant information onto one line, and for things like overloads it's arguably impossible.
or in the editor
Whenever I need more information on a diagnostic rust-analyzer is showing me in the "Problems" tab in VSCode, I always click on the "see full diagnostic" option, and then a new tab opens up with all subdiagnostics displayed as well as the main diagnostic. Is it not possible for us to implement something similar for users of the ty server?
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.
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.
Though even for on-hover, we may want to avoid displaying all union elements in situations where the union is 60 elements long...?
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.
In general I think we infer unions more than any other Python type checker. This is not only because we treat intersections as first-class types, but also because we have concepts such as "class-literal types" and "function-literal types". To my knowledge, no other Python type checker treats these concepts as first-class types, and treating them as such seems to mean that huge unions are more common for us than for other type checkers
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.
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.
No other Python type checker attempts to highlight the problematic union element as part of the primary diagnostic for something like this, from what I can tell:
- Mypy: https://mypy-play.net/?mypy=latest&python=3.12&gist=e52205a0769db5dd033a163d85f561f8
- Pyright: https://pyright-play.net/?pyrightVersion=1.1.405&strict=true&code=CYUwZgBGAUAeBcECWA7ALhAPhAzmgTgJTwCwAUBJRADYgpyHlA
- Pyrefly: https://pyrefly.org/sandbox/?project=N4IgZglgNgpgziAXKOBDAdgEwEYHsAeAdAA4CeSImMYABGABT6I0ToAuNAPjXGwE4BKRAB10NcTVjpGA0SAA0IAK5tocEuUQgAxDQCqqqBDak6S9AGNVudHFGiqtMLj4BbVGwD66Ja%2Bww%2BRmZWNgEaAFoAPh5%2BETEJPhg2JT4xMGEQADlffz5mYHwAXwy5RTJEsChSQjZcVygKXQAFUgqqngwcAhoLG0gAcxSPCBtCUV0AZRgYGgALNjZiOEQAehXy6irCF36VmHQVzFwLOBXe9AGh6wO6FxpUADdUaFRsWB6%2BiEG%2BYZsaXGI13UojIbFmNnCDwCcBGYgAvDQMgBmQgARgATCV0CBCopUFYIFCAGLQGAUNBYPBEMg4oA
Pyright highlights it in a subdiagnostic, the same as my PR currently proposes
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's true. But no other type checker seems to truncate the union type :)
I'm fine merging this as is but I really think our approach to truncating unions is problematic
| )), | ||
| } | ||
| } | ||
| } |
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 assumed we would fix this somewhere else, e.g. when actually checking assignability in the first place. This code here only affects invalid-argument-type, but we should do the same thing in other situations, like for actual assignments to variables. Consider:
from typing import Sized
class Foo: ...
def g(
b: list[str]
| str
| dict[str, str]
| tuple[str, ...]
| bytes
| frozenset[str]
| set[str]
| Foo,
):
y: Sized = b # still only shows the truncated unionThere 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.
A more general solution would probably be #19580 (so that we always propagate up the reason why an assignability/subtyping relation did not hold). I do think we need something like that eventually, for better diagnostics in cases where Protocol subtyping doesn't hold (our diagnostics are currently terrible for protocols with lots of members — you can't tell which specific member of the protocol wasn't satisfied). But that would be a big change, and I'm not yet sure how it would fit in with Doug's work on the new constraint solver.
We already do ad-hoc special-casing of union types like this for several other diagnostics. I agree it's not great but I think it's the best we can do for now, and it's better than nothing in the short term.



Fixes astral-sh/ty#1411