[ty] Improve diagnostics for assert_type and assert_never
#18050
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
|
Member
|
Thanks for looking into this. I think I'd prefer if we don't repeat the inferred type in the sub diagnostic message and diagnostic. I'm leaning towards only having it in the annotation but I'm not sure if that's possible. The other issue that I see is that the actual type is now omitted when using |
Member
Author
|
Hmm, your feedback points contradict each other @MichaReiser 😆
I agree the duplication seems unfortunate, but I was worried that doing what you suggest here would make the diagnostic summary (which is what is displayed for the concise output format) pretty useless. I do think we should care about what
Yeah, I agree it's not ideal (see above). We do already have this issue for lots of other diagnostic, as you say, though. |
Member
I don't think it changes the concise output because the concise output only displays information from the primary diagnostic, not from the secondary diagnostic (as you can see in the mdtests)
We can improve this. Again, the actual type isn't part of the primary message. That's why I think we shouldn't repeat the actual type in the sub-diagnostic either by removing it from the annotation or from the message (I'd prefer that) |
Member
Author
|
@MichaReiser what would you think about this change (relative to my PR): Diff--- a/crates/ty_python_semantic/src/types/infer.rs
+++ b/crates/ty_python_semantic/src/types/infer.rs
@@ -4814,14 +4814,8 @@ impl<'db> TypeInferenceBuilder<'db> {
asserted_ty.display(self.db())
));
- let mut subdiagnostic = SubDiagnostic::new(
- Severity::Info,
- format_args!(
- "`{}` is not an equivalent type to `{}`",
- actual_ty.display(self.db()),
- asserted_ty.display(self.db())
- ),
- );
+ let mut subdiagnostic =
+ SubDiagnostic::new(Severity::Info, "");
subdiagnostic.annotate(
Annotation::primary(self.context.span(
&call_expression.arguments.args[0],
@@ -4848,13 +4842,8 @@ impl<'db> TypeInferenceBuilder<'db> {
"Argument does not have expected type `Never`",
);
- let mut subdiagnostic = SubDiagnostic::new(
- Severity::Info,
- format_args!(
- "`{}` is not an equivalent type to `Never`",
- actual_ty.display(self.db()),
- ),
- );
+ let mut subdiagnostic =
+ SubDiagnostic::new(Severity::Info, "");
subdiagnostic.annotate(
Annotation::primary(self.context.span(
&call_expression.arguments.args[0],which would render like this? Screenshot
|
Member
|
To make sure I understand the screenshot: Does the first diagnostic show the old format? Now that I'm seeing it, it sort of feels odd that the sub diagnostic has no message. Maybe it is better to remove the annotation message? Do you have @BurntSushi any recommendation here? |
Member
Author
no, both diagnostics in the new screenshot are what would be shown with the changes to my PR that I proposed in that diff. One diagnostic is for |
Member
|
Hmm okay. I'm a bit confused because it still repeats the actual (and even expected) type for I think I'd go with:
But curious to hear from Andrew. |
Member
Author
Ah shoot, yes, I think that was a stale screenshot! Sorry for the confusion. Here's a new one:
|
Member
Author
|
@MichaReiser I pushed some updates which I think (at least partially) might address your comments? WDYT? I've edited the new screenshots into the PR description |
AlexWaygood
commented
May 12, 2025
Member
Author
There was a problem hiding this comment.
(none of the changes in this file are substantive. Just some driveby simplifications.)
Member
|
Thanks |
BurntSushi
reviewed
May 12, 2025
Member
BurntSushi
left a comment
BurntSushi
left a comment
There was a problem hiding this comment.
I left a suggestion that I think might help here.
As for the concise message format, I think it will prove very challenging to use one set of messages that balances both the concise and the verbose formats. They have fundamentally different goals and present information very different, and I worry that by trying to balance the two, we will wind up in a situation where both outputs are bad. Instead, I'd rather accept that the concise output is less helpful than it could be.
| 8 | assert_never("") # error: [type-assertion-failure] | ||
| 9 | assert_never(None) # error: [type-assertion-failure] | ||
| | | ||
| info: rule `type-assertion-failure` is enabled by default |
Member
There was a problem hiding this comment.
I think this output is not ideal. In particular, it repeats the entire code snippet here. I'd suggest just this:
error[type-assertion-failure]: Argument does not have expected type `Never`
--> src/mdtest_snippet.py:7:5
|
5 | assert_never(never) # fine
6 |
7 | assert_never(0) # error: [type-assertion-failure]
| ^ Inferred type is `Literal[0]`
8 | assert_never("") # error: [type-assertion-failure]
9 | assert_never(None) # error: [type-assertion-failure]
|
I think this has better information density, has less duplication and contains all relevant context.
Member
Author
There was a problem hiding this comment.
Yes, I agree that there's some unfortunate duplication here. I'd love to only have a single annotation that only highlights the arguments passed in rather than the whole call.
Did you see the concern I mentioned in my PR summary about how diagnostic ranges interact with suppression comments? That's why I held off from doing that in this PR. If the range of the diagnostic is only the range of the arguments passed into the call, rather than the whole call, it's going to mean that comments like this will not suppress the diagnostic:
assert_type( # type: ignore
very_very_very_long_variable_name,
very_very_very_long_type,
)I think it's especially a concern for assert_type, which requires two arguments rather than one (so the call is more likely to be multiline).
Member
There was a problem hiding this comment.
Oh I see, I did miss that. In particular:
I feel like in general we might want to separate the concepts of "range the diagnostic has for suppression purposes" and "primary range that is highlighted when the diagnostic is rendered on the terminal"? But that's out of scope for this PR.
I would tentatively agree with that... But there's definitely downsides with that because it makes creating a diagnostic somewhat confusing.
What I'd suggest instead is to have two annotations on the top-level diagnostic. The primary annotation can be the range you want for suppression comments to work well. Then a secondary annotation for the specific argument that is invalid. IDK exactly how it would render, but something like this:
error[type-assertion-failure]: Argument does not have expected type `Never`
--> src/mdtest_snippet.py:7:5
|
5 | assert_never(never) # fine
6 |
7 | assert_never(0) # error: [type-assertion-failure]
| ^^^^^^^^^^^^^^^
| |
| |
| ------^ Inferred type is `Literal[0]`
8 | assert_never("") # error: [type-assertion-failure]
9 | assert_never(None) # error: [type-assertion-failure]
|
In this case, I left the message on the primary annotation blank. I'm not sure if that's the right call or not, but it seems reasonable.
Member
Author
There was a problem hiding this comment.
What I'd suggest instead is to have two annotations on the top-level diagnostic.
I've pushed this change. The rendering looks a bit odd to me, in particular the way _ underlines are surrounded by ^ underlines:
but it's probably worth it to keep the diagnostics concise, as you say.
BurntSushi
approved these changes
May 13, 2025
Member
BurntSushi
left a comment
BurntSushi
left a comment
There was a problem hiding this comment.
Yeah I agree the rendering is not ideal here. But I think it's probably the least bad option available to us at the moment.
AlexWaygood
force-pushed
the
alex/assert-type-range
branch
from
eda8ae2 to
9585434
Compare
dcreager
added a commit
that referenced
this pull request
May 13, 2025
…eep-dish * origin/main: [ty] __file__ is always a string inside a Python module (#18071) [ty] Recognize submodules in self-referential imports (#18005) [ty] Add a note to the diagnostic if a new builtin is used on an old Python version (#18068) [ty] Add tests for `else` branches of `hasattr()` narrowing (#18067) [ty] Improve diagnostics for `assert_type` and `assert_never` (#18050) [ty] contribution guide (#18061) [ty] Implement `DataClassInstance` protocol for dataclasses. (#18018) [ruff_python_ast] Fix redundant visitation of test expressions in elif clause statements (#18064)
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
Further work towards astral-sh/ty#209.
I didn't make exactly the change suggested in that issue (only highlight the argument range) because I was worried that the behaviour of
type: ignorecomments might be confusing if the range of the diagnostic only covered part of the call. E.g. I'd expect this to work, as a user:I feel like in general we might want to separate the concepts of "range the diagnostic has for suppression purposes" and "primary range that is highlighted when the diagnostic is rendered on the terminal"? But that's out of scope for this PR.
Instead, this PR adds a subdiagnostic that draws the user's attention to the range of the argument passed in.
Test Plan
Before:
After:
(I also added snapshots)