[ty] Improve several "Did you mean?" suggestions#21597
Conversation
AlexWaygood
added
ty
Diagnostic diff on typing conformance testsChanges were detected when running ty on typing conformance tests--- old-output.txt 2025-11-23 20:23:14.805065538 +0000
+++ new-output.txt 2025-11-23 20:23:18.493085157 +0000
@@ -78,7 +78,7 @@
annotations_forward_refs.py:47:10: error[invalid-type-form] Invalid subscript of object of type `list[Unknown | <class 'int'>]` in type expression
annotations_forward_refs.py:49:10: error[invalid-type-form] Variable of type `Literal[1]` is not allowed in a type expression
annotations_forward_refs.py:54:11: error[fstring-type-annotation] Type expressions cannot use f-strings
-annotations_forward_refs.py:55:11: error[invalid-type-form] Variable of type `<module 'types'>` is not allowed in a type expression
+annotations_forward_refs.py:55:11: error[invalid-type-form] Module `types` is not valid in a type expression
annotations_forward_refs.py:80:14: error[unresolved-reference] Name `ClassF` used when not defined
annotations_forward_refs.py:82:11: error[invalid-type-form] Variable of type `Literal[""]` is not allowed in a type expression
annotations_forward_refs.py:87:9: error[invalid-type-form] Variable of type `def int(self) -> None` is not allowed in a type expression
@@ -110,7 +110,7 @@
annotations_typeexpr.py:99:10: error[invalid-type-form] Unary operations are not allowed in type expressions
annotations_typeexpr.py:100:10: error[invalid-type-form] Boolean operations are not allowed in type expressions
annotations_typeexpr.py:101:10: error[fstring-type-annotation] Type expressions cannot use f-strings
-annotations_typeexpr.py:102:10: error[invalid-type-form] Variable of type `<module 'types'>` is not allowed in a type expression
+annotations_typeexpr.py:102:10: error[invalid-type-form] Module `types` is not valid in a type expression
callables_annotation.py:25:5: error[missing-argument] No argument provided for required parameter 2
callables_annotation.py:26:11: error[invalid-argument-type] Argument is incorrect: Expected `str`, found `Literal[2]`
callables_annotation.py:27:15: error[too-many-positional-arguments] Too many positional arguments: expected 2, got 3
@@ -1001,6 +1001,6 @@
typeddicts_usage.py:24:17: error[invalid-assignment] Invalid assignment to key "year" with declared type `int` on TypedDict `Movie`: value of type `Literal["1982"]`
typeddicts_usage.py:28:17: error[missing-typed-dict-key] Missing required key 'name' in TypedDict `Movie` constructor
typeddicts_usage.py:28:18: error[invalid-key] Unknown key "title" for TypedDict `Movie`: Unknown key "title"
-typeddicts_usage.py:40:24: error[invalid-type-form] The special form `typing.TypedDict` is not allowed in type expressions. Did you mean to use a concrete TypedDict or `collections.abc.Mapping[str, object]` instead?
+typeddicts_usage.py:40:24: error[invalid-type-form] The special form `typing.TypedDict` is not allowed in type expressions
Found 1004 diagnostics
WARN A fatal error occurred while checking some files. Not all project files were analyzed. See the diagnostics list above for details.
|
| ) { | ||
| diag.info("Did you mean to use `|`?"); | ||
| } |
There was a problem hiding this comment.
we don't have any tests for this subdiagnostic and I'm not convinced it's likely to be helpful. If a user has int + str in a type expression, I think it's very unclear what they meant; if they have int - bool, I think it's likely that they were trying to write a negation type rather than a union type; etc.
There was a problem hiding this comment.
We could say something about that only | is allowed to define a union.
There was a problem hiding this comment.
We could, but we already link to the full type expression grammar in a separate subdiagnostic (which covers that), and I think it's very unclear that they were actually trying too define a union at all
AlexWaygood
requested review from
carljm,
dcreager and
sharkdp
as code owners
|
|
Screenshot of some of the new diagnostics, since snapshots don't have colour: |
I think I've a preference towards not doing that because it makes the |
| "Did you mean to use the module's member \ | ||
| `{module_name_final_part}.{module_name_final_part}` instead?" | ||
| )); | ||
| // TODO: showing a diff (and even having an autofix) would be even better |
There was a problem hiding this comment.
This should be "easy" once my unused suppression PR lands.
| )); | ||
| } else if let InvalidTypeExpression::TypedDict = self { | ||
| diagnostic.help( | ||
| "You might have meant to use a concrete TypedDict \ |
There was a problem hiding this comment.
I think I know what it is but it might not be clear to all users what a concrete TypedDict is.
There was a problem hiding this comment.
A type that actually is a TypedDict, rather than the special form typing.TypedDict itself.
from typing import TypedDict
class MyTypedDict(TypedDict):
x: int
obj: MyTypedDictinstead of
from typing import TypedDict
obj: TypedDictI take your point that this might be unclear to some people, but I'm not sure what clearer wording here might be... "An actual TypedDict" feels just as bad. And I can't put that whole example in the subdiagnostic!
There was a problem hiding this comment.
Okay, I actually assumed it's something different 😅
But I agree it's tricky. I tried to produce a similar error in TypeScript but they don't have this issue because it doesn't have marker types similar to TypedDict
If we are confident in our suggestion, I like the change here. I always look at the primary annotation before I even read the main diagnostic message, and if it's directly "actionable", that's perfect. |
|
Yeah, I agree with @sharkdp. If we have a direct suggestion for how to fix the diagnostic, that feels like one of the most important and useful parts of the diagnostic display to me. I think it's really useful to have that highlighted prominently, and I like that with secondary annotations we can clearly indicate that we might only be suggesting to change part of the diagnostic range, not necessarily the whole range.
That's true of course, but to me it seems like very useful information that I'd want as part of the concise diagnostic. And I don't think the new concise diagnostic messages we see in mypy_primer are that verbose? |
|
I wonder if the solution here really is to just attach a |
|
I'll land this for now as an incremental improvement. It's easy to make further adjustments later and we can easily re-evaluate if and when we add fixes for some or all of these diagnostics |
* main: [ty] Implement `typing.override` (astral-sh#21627) [ty] Avoid expression reinference for diagnostics (astral-sh#21267) [ty] Improve autocomplete suppressions of keywords in variable bindings [ty] Only suggest completions based on text before the cursor Implement goto-definition and find-references for global/nonlocal statements (astral-sh#21616) [ty] Inlay Hint edit follow up (astral-sh#21621) [ty] Implement lsp support for string annotations (astral-sh#21577) [ty] Add 'remove unused ignore comment' code action (astral-sh#21582) [ty] Refactor `CheckSuppressionContext` to use `DiagnosticGuard` (astral-sh#21587) [ty] Improve several "Did you mean?" suggestions (astral-sh#21597) [ty] Add more and update existing projects in `ty_benchmark` (astral-sh#21536) [ty] fix ty playground initialization and vite optimization issues (astral-sh#21471)
3 participants
Summary
helpseverity for suggestions when they do appear as subdiagnostics, rather than theinfoseverityTest Plan
Snapshots