[ty] Avoid inferring types for invalid binary expressions in string annotations#21911
[ty] Avoid inferring types for invalid binary expressions in string annotations#21911
Conversation
charliermarsh
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
AlexWaygood
changed the title
| // Avoid inferring the types of invalid binary expressions that have been | ||
| // parsed from a string annotation, as they are not present in the semantic | ||
| // index. | ||
| if !self.deferred_state.in_string_annotation() { | ||
| self.infer_binary_expression(binary, TypeContext::default()); | ||
| } |
There was a problem hiding this comment.
We used to have the requirement that inference had to run for every expression in the AST, or otherwise we might panic when requesting a type for that expression. We have changed that and now fall back to Unknown in case no type has been inferred.
Since we're not going to infer useful types here anyway, I think we can also just remove this entirely.
There was a problem hiding this comment.
That leads to a worse experience for server users when they hover over sub-expressions in the invalid type expression, no? I think we should still aim to store a type for each sub-expression wherever possible
There was a problem hiding this comment.
That leads to a worse experience for server users when they hover over sub-expressions in the invalid type expression, no?
I mean... maybe? But the whole thing will be red-squiggly underlined anyway. It's completely wrong. Why is it helpful that they see Literal[4] if they hover over the 4 in x: "3+4"? Is Literal[4] even more correct than Unknown here?
There was a problem hiding this comment.
I don't think that's really a realistic example of an invalid type expression that someone is likely to use. If somebody is using the appeal library, for example, they might be making use of that library's special DSL for annotations (which is incompatible with type checkers, and so is going to result in lots of invalid-type-form violations). Who knows if this user even has invalid-type-form enabled in their ty config; maybe they're only interested in our server functionality. I don't see why the invalid type expression should mean that we don't offer any on-hover information or go-to-definition support for any symbols in this user's annotations.
There was a problem hiding this comment.
To be clear I'm specifically objecting to f2e50c4. I'm fine with not attempting to infer the sub-expressions of invalid stringized annotations if it makes us crash. That seems like a pretty rare edge case. It's the extension of that to "all invalid binary expressions in type expressions" that I'm uncomfortable with.
I just feel that as a general principle, we should continue to attempt to infer types for all sub-expressions wherever possible (on a best-effort basis)
There was a problem hiding this comment.
I'm still not convinced that it's better to run infer_binary_expression here just for the sake of having "some" type (because we're very explicitly in a type expression, and not in a value expression here).
But I certainly don't feel as strongly as you do, so let's just revert: #21914
…otations" (#21914) See discussion here: #21911 (comment)
* origin/main: (29 commits) Document range suppressions, reorganize suppression docs (#21884) Ignore ruff:isort like ruff:noqa in new suppressions (#21922) [ty] Handle `Definition`s in `SemanticModel::scope` (#21919) [ty] Attach salsa db when running ide tests for easier debugging (#21917) [ty] Don't show hover for expressions with no inferred type (#21924) [ty] avoid unions of generic aliases of the same class in fixpoint (#21909) [ty] Squash false positive logs for failing to find `builtins` as a real module [ty] Uniformly use "not supported" in diagnostics (#21916) [ty] Reduce size of ty-ide snapshots (#21915) [ty] Adjust scope completions to use all reachable symbols [ty] Rename `all_members_of_scope` to `all_end_of_scope_members` [ty] Remove `all_` prefix from some routines on UseDefMap Enable `--document-private-items` for `ruff_python_formatter` (#21903) Remove `BackwardsTokenizer` based `parenthesized_range` references in `ruff_linter` (#21836) [ty] Revert "Do not infer types for invalid binary expressions in annotations" (#21914) Skip over trivia tokens after re-lexing (#21895) [ty] Avoid inferring types for invalid binary expressions in string annotations (#21911) [ty] Improve overload call resolution tracing (#21913) [ty] fix missing heap_size on Salsa query (#21912) [ty] Support implicit type of `cls` in signatures (#21771) ...
* origin/main: (36 commits) [ty] Defer all parameter and return type annotations (#21906) [ty] Fix workspace symbols to return members too (#21926) Document range suppressions, reorganize suppression docs (#21884) Ignore ruff:isort like ruff:noqa in new suppressions (#21922) [ty] Handle `Definition`s in `SemanticModel::scope` (#21919) [ty] Attach salsa db when running ide tests for easier debugging (#21917) [ty] Don't show hover for expressions with no inferred type (#21924) [ty] avoid unions of generic aliases of the same class in fixpoint (#21909) [ty] Squash false positive logs for failing to find `builtins` as a real module [ty] Uniformly use "not supported" in diagnostics (#21916) [ty] Reduce size of ty-ide snapshots (#21915) [ty] Adjust scope completions to use all reachable symbols [ty] Rename `all_members_of_scope` to `all_end_of_scope_members` [ty] Remove `all_` prefix from some routines on UseDefMap Enable `--document-private-items` for `ruff_python_formatter` (#21903) Remove `BackwardsTokenizer` based `parenthesized_range` references in `ruff_linter` (#21836) [ty] Revert "Do not infer types for invalid binary expressions in annotations" (#21914) Skip over trivia tokens after re-lexing (#21895) [ty] Avoid inferring types for invalid binary expressions in string annotations (#21911) [ty] Improve overload call resolution tracing (#21913) ...
3 participants
Summary
Closes astral-sh/ty#1847.