Skip to content

[ty] Improve diagnostics for assert_type and assert_never #18050

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.

Sign up for GitHub

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

Merged
AlexWaygood merged 3 commits into from
May 13, 2025
Merged

[ty] Improve diagnostics for assert_type and assert_never #18050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a51b6a7
[ty] Improve diagnostics for `assert_type` and `assert_never`
AlexWaygood May 12, 2025
00f1107
address review
AlexWaygood May 12, 2025
9585434
a single diagnostic with multiple annotations
AlexWaygood May 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions crates/ty_python_semantic/resources/mdtest/directives/assert_never.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Basic functionality

<!-- snapshot-diagnostics -->

`assert_never` makes sure that the type of the argument is `Never`. If it is not, a
`type-assertion-failure` diagnostic is emitted.

Expand Down Expand Up @@ -58,7 +60,7 @@ def if_else_isinstance_error(obj: A | B):
elif isinstance(obj, C):
pass
else:
# error: [type-assertion-failure] "Expected type `Never`, got `B & ~A & ~C` instead"
# error: [type-assertion-failure] "Argument does not have asserted type `Never`"
assert_never(obj)

def if_else_singletons_success(obj: Literal[1, "a"] | None):
Expand All @@ -79,7 +81,7 @@ def if_else_singletons_error(obj: Literal[1, "a"] | None):
elif obj is None:
pass
else:
# error: [type-assertion-failure] "Expected type `Never`, got `Literal["a"]` instead"
# error: [type-assertion-failure] "Argument does not have asserted type `Never`"
assert_never(obj)

def match_singletons_success(obj: Literal[1, "a"] | None):
Expand All @@ -92,7 +94,7 @@ def match_singletons_success(obj: Literal[1, "a"] | None):
pass
case _ as obj:
# TODO: Ideally, we would not emit an error here
# error: [type-assertion-failure] "Expected type `Never`, got `@Todo"
# error: [type-assertion-failure] "Argument does not have asserted type `Never`"
assert_never(obj)

def match_singletons_error(obj: Literal[1, "a"] | None):
Expand All @@ -106,6 +108,6 @@ def match_singletons_error(obj: Literal[1, "a"] | None):
case _ as obj:
# TODO: We should emit an error here, but the message should
# show the type `Literal["a"]` instead of `@Todo(…)`.
# error: [type-assertion-failure] "Expected type `Never`, got `@Todo"
# error: [type-assertion-failure] "Argument does not have asserted type `Never`"
assert_never(obj)
```
2 changes: 2 additions & 0 deletions crates/ty_python_semantic/resources/mdtest/directives/assert_type.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Basic

<!-- snapshot-diagnostics -->

```py
from typing_extensions import assert_type

Expand Down
189 changes: 189 additions & 0 deletions ...ic/resources/mdtest/snapshots/assert_never.md_-_`assert_never`_-_Basic_functionality.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,189 @@
---
source: crates/ty_test/src/lib.rs
expression: snapshot
---
---
mdtest name: assert_never.md - `assert_never` - Basic functionality
mdtest path: crates/ty_python_semantic/resources/mdtest/directives/assert_never.md
---

# Python source files

## mdtest_snippet.py

```
1 | from typing_extensions import assert_never, Never, Any
2 | from ty_extensions import Unknown
3 |
4 | def _(never: Never, any_: Any, unknown: Unknown, flag: bool):
5 | assert_never(never) # fine
6 |
7 | assert_never(0) # error: [type-assertion-failure]
8 | assert_never("") # error: [type-assertion-failure]
9 | assert_never(None) # error: [type-assertion-failure]
10 | assert_never([]) # error: [type-assertion-failure]
11 | assert_never({}) # error: [type-assertion-failure]
12 | assert_never(()) # error: [type-assertion-failure]
13 | assert_never(1 if flag else never) # error: [type-assertion-failure]
14 |
15 | assert_never(any_) # error: [type-assertion-failure]
16 | assert_never(unknown) # error: [type-assertion-failure]
```

# Diagnostics

```
error[type-assertion-failure]: Argument does not have asserted type `Never`
--> src/mdtest_snippet.py:7:5
|
5 | assert_never(never) # fine
6 |
7 | assert_never(0) # error: [type-assertion-failure]
| ^^^^^^^^^^^^^-^
| |
| Inferred type of argument is `Literal[0]`
8 | assert_never("") # error: [type-assertion-failure]
9 | assert_never(None) # error: [type-assertion-failure]
|
info: `Never` and `Literal[0]` are not equivalent types
info: rule `type-assertion-failure` is enabled by default
Copy link
Member

@BurntSushi BurntSushi May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@AlexWaygood AlexWaygood May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@BurntSushi BurntSushi May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@AlexWaygood AlexWaygood May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

but it's probably worth it to keep the diagnostics concise, as you say.


```

```
error[type-assertion-failure]: Argument does not have asserted type `Never`
--> src/mdtest_snippet.py:8:5
|
7 | assert_never(0) # error: [type-assertion-failure]
8 | assert_never("") # error: [type-assertion-failure]
| ^^^^^^^^^^^^^--^
| |
| Inferred type of argument is `Literal[""]`
9 | assert_never(None) # error: [type-assertion-failure]
10 | assert_never([]) # error: [type-assertion-failure]
|
info: `Never` and `Literal[""]` are not equivalent types
info: rule `type-assertion-failure` is enabled by default

```

```
error[type-assertion-failure]: Argument does not have asserted type `Never`
--> src/mdtest_snippet.py:9:5
|
7 | assert_never(0) # error: [type-assertion-failure]
8 | assert_never("") # error: [type-assertion-failure]
9 | assert_never(None) # error: [type-assertion-failure]
| ^^^^^^^^^^^^^----^
| |
| Inferred type of argument is `None`
10 | assert_never([]) # error: [type-assertion-failure]
11 | assert_never({}) # error: [type-assertion-failure]
|
info: `Never` and `None` are not equivalent types
info: rule `type-assertion-failure` is enabled by default

```

```
error[type-assertion-failure]: Argument does not have asserted type `Never`
--> src/mdtest_snippet.py:10:5
|
8 | assert_never("") # error: [type-assertion-failure]
9 | assert_never(None) # error: [type-assertion-failure]
10 | assert_never([]) # error: [type-assertion-failure]
| ^^^^^^^^^^^^^--^
| |
| Inferred type of argument is `list[Unknown]`
11 | assert_never({}) # error: [type-assertion-failure]
12 | assert_never(()) # error: [type-assertion-failure]
|
info: `Never` and `list[Unknown]` are not equivalent types
info: rule `type-assertion-failure` is enabled by default

```

```
error[type-assertion-failure]: Argument does not have asserted type `Never`
--> src/mdtest_snippet.py:11:5
|
9 | assert_never(None) # error: [type-assertion-failure]
10 | assert_never([]) # error: [type-assertion-failure]
11 | assert_never({}) # error: [type-assertion-failure]
| ^^^^^^^^^^^^^--^
| |
| Inferred type of argument is `dict[Unknown, Unknown]`
12 | assert_never(()) # error: [type-assertion-failure]
13 | assert_never(1 if flag else never) # error: [type-assertion-failure]
|
info: `Never` and `dict[Unknown, Unknown]` are not equivalent types
info: rule `type-assertion-failure` is enabled by default

```

```
error[type-assertion-failure]: Argument does not have asserted type `Never`
--> src/mdtest_snippet.py:12:5
|
10 | assert_never([]) # error: [type-assertion-failure]
11 | assert_never({}) # error: [type-assertion-failure]
12 | assert_never(()) # error: [type-assertion-failure]
| ^^^^^^^^^^^^^--^
| |
| Inferred type of argument is `tuple[()]`
13 | assert_never(1 if flag else never) # error: [type-assertion-failure]
|
info: `Never` and `tuple[()]` are not equivalent types
info: rule `type-assertion-failure` is enabled by default

```

```
error[type-assertion-failure]: Argument does not have asserted type `Never`
--> src/mdtest_snippet.py:13:5
|
11 | assert_never({}) # error: [type-assertion-failure]
12 | assert_never(()) # error: [type-assertion-failure]
13 | assert_never(1 if flag else never) # error: [type-assertion-failure]
| ^^^^^^^^^^^^^--------------------^
| |
| Inferred type of argument is `Literal[1]`
14 |
15 | assert_never(any_) # error: [type-assertion-failure]
|
info: `Never` and `Literal[1]` are not equivalent types
info: rule `type-assertion-failure` is enabled by default

```

```
error[type-assertion-failure]: Argument does not have asserted type `Never`
--> src/mdtest_snippet.py:15:5
|
13 | assert_never(1 if flag else never) # error: [type-assertion-failure]
14 |
15 | assert_never(any_) # error: [type-assertion-failure]
| ^^^^^^^^^^^^^----^
| |
| Inferred type of argument is `Any`
16 | assert_never(unknown) # error: [type-assertion-failure]
|
info: `Never` and `Any` are not equivalent types
info: rule `type-assertion-failure` is enabled by default

```

```
error[type-assertion-failure]: Argument does not have asserted type `Never`
--> src/mdtest_snippet.py:16:5
|
15 | assert_never(any_) # error: [type-assertion-failure]
16 | assert_never(unknown) # error: [type-assertion-failure]
| ^^^^^^^^^^^^^-------^
| |
| Inferred type of argument is `Unknown`
|
info: `Never` and `Unknown` are not equivalent types
info: rule `type-assertion-failure` is enabled by default

```
38 changes: 38 additions & 0 deletions ...ty_python_semantic/resources/mdtest/snapshots/assert_type.md_-_`assert_type`_-_Basic.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
---
source: crates/ty_test/src/lib.rs
expression: snapshot
---
---
mdtest name: assert_type.md - `assert_type` - Basic
mdtest path: crates/ty_python_semantic/resources/mdtest/directives/assert_type.md
---

# Python source files

## mdtest_snippet.py

```
1 | from typing_extensions import assert_type
2 |
3 | def _(x: int):
4 | assert_type(x, int) # fine
5 | assert_type(x, str) # error: [type-assertion-failure]
```

# Diagnostics

```
error[type-assertion-failure]: Argument does not have asserted type `str`
--> src/mdtest_snippet.py:5:5
|
3 | def _(x: int):
4 | assert_type(x, int) # fine
5 | assert_type(x, str) # error: [type-assertion-failure]
| ^^^^^^^^^^^^-^^^^^^
| |
| Inferred type of argument is `int`
|
info: `str` and `int` are not equivalent types
info: rule `type-assertion-failure` is enabled by default

```
11 changes: 4 additions & 7 deletions crates/ty_python_semantic/src/types/diagnostic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use crate::types::string_annotation::{
RAW_STRING_TYPE_ANNOTATION,
};
use crate::types::{protocol_class::ProtocolClassLiteral, KnownFunction, KnownInstanceType, Type};
use ruff_db::diagnostic::{Annotation, Diagnostic, Severity, Span, SubDiagnostic};
use ruff_db::diagnostic::{Annotation, Diagnostic, Severity, SubDiagnostic};
use ruff_python_ast::{self as ast, AnyNodeRef};
use ruff_text_size::{Ranged, TextRange};
use rustc_hash::FxHashSet;
Expand Down Expand Up @@ -1543,7 +1543,7 @@ pub(super) fn report_invalid_return_type(
return;
};

let return_type_span = Span::from(context.file()).with_range(return_type_range.range());
let return_type_span = context.span(return_type_range);

let mut diag = builder.into_diagnostic("Return type does not match returned value");
diag.set_primary_message(format_args!(
Expand Down Expand Up @@ -1849,16 +1849,13 @@ pub(crate) fn report_duplicate_bases(
),
);
sub_diagnostic.annotate(
Annotation::secondary(
Span::from(context.file()).with_range(bases_list[*first_index].range()),
)
.message(format_args!(
Annotation::secondary(context.span(&bases_list[*first_index])).message(format_args!(
"Class `{duplicate_name}` first included in bases list here"
)),
);
for index in later_indices {
sub_diagnostic.annotate(
Annotation::primary(Span::from(context.file()).with_range(bases_list[*index].range()))
Annotation::primary(context.span(&bases_list[*index]))
.message(format_args!("Class `{duplicate_name}` later repeated here")),
);
}
Expand Down
49 changes: 39 additions & 10 deletions crates/ty_python_semantic/src/types/infer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4808,12 +4808,27 @@ impl<'db> TypeInferenceBuilder<'db> {
&TYPE_ASSERTION_FAILURE,
call_expression,
) {
builder.into_diagnostic(format_args!(
"Actual type `{}` is not the same \
as asserted type `{}`",
actual_ty.display(self.db()),
asserted_ty.display(self.db()),
));
let mut diagnostic =
builder.into_diagnostic(format_args!(
"Argument does not have asserted type `{}`",
asserted_ty.display(self.db()),
));
diagnostic.annotate(
Annotation::secondary(self.context.span(
&call_expression.arguments.args[0],
))
.message(format_args!(
"Inferred type of argument is `{}`",
actual_ty.display(self.db()),
)),
);
diagnostic.info(
format_args!(
"`{asserted_type}` and `{inferred_type}` are not equivalent types",
asserted_type = asserted_ty.display(self.db()),
inferred_type = actual_ty.display(self.db()),
)
);
}
}
}
Expand All @@ -4825,10 +4840,24 @@ impl<'db> TypeInferenceBuilder<'db> {
&TYPE_ASSERTION_FAILURE,
call_expression,
) {
builder.into_diagnostic(format_args!(
"Expected type `Never`, got `{}` instead",
actual_ty.display(self.db()),
));
let mut diagnostic = builder.into_diagnostic(
"Argument does not have asserted type `Never`",
);
diagnostic.annotate(
Annotation::secondary(self.context.span(
&call_expression.arguments.args[0],
))
.message(format_args!(
"Inferred type of argument is `{}`",
actual_ty.display(self.db())
)),
);
diagnostic.info(
format_args!(
"`Never` and `{inferred_type}` are not equivalent types",
inferred_type = actual_ty.display(self.db()),
)
);
}
}
}
Expand Down
Loading