[ty] do nothing with store_expression_type if inner_expression_inference_state is Get#21718
[ty] do nothing with store_expression_type if inner_expression_inference_state is Get#21718carljm merged 2 commits intoastral-sh:mainfrom
store_expression_type if inner_expression_inference_state is Get#21718Conversation
…erence_state` is `Get`
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
| fn store_expression_type(&mut self, expression: &ast::Expr, ty: Type<'db>) { | ||
| if self.deferred_state.in_string_annotation() { | ||
| if self.deferred_state.in_string_annotation() | ||
| || self.inner_expression_inference_state.is_get() |
There was a problem hiding this comment.
Could we use multi_inference_state::Ignore instead of having two modes that represent "read-only" type inference?
There was a problem hiding this comment.
drive-by context: I am likely to modify this check for in_string_annotation to do the "store (Expr, Expr) for string annotation types" thing we've been discussing.
There was a problem hiding this comment.
Could we use
multi_inference_state::Ignoreinstead of having two modes that represent "read-only" type inference?
I understand that MultiInferenceState::Ignore is an option to perform the same calculation twice and discard the second result.
Since all type inference for expressions is skipped while in InnerExpressionInferenceState::Get, I think it should be used whenever possible (for example, when only diagnostics are needed, such as the handling in infer_subscript_type_expression for union types).
But I'm not sure whether this can be assumed in all cases where MultiInferenceState::Ignore is used.
There was a problem hiding this comment.
I think it is necessary for MultiInferenceState to redo inference, since it is doing it with different type context. So I think we need both.
mtshiba
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
This looks correct, thank you.
* origin/main: (41 commits) [ty] Carry generic context through when converting class into `Callable` (#21798) [ty] Add more tests for renamings (#21810) [ty] Minor improvements to `assert_type` diagnostics (#21811) [ty] Add some attribute/method renaming test cases (#21809) Update mkdocs-material to 9.7.0 (Insiders now free) (#21797) Remove unused whitespaces in test cases (#21806) [ty] fix panic when instantiating a type variable with invalid constraints (#21663) [ty] fix build failure caused by conflicts between #21683 and #21800 (#21802) [ty] do nothing with `store_expression_type` if `inner_expression_inference_state` is `Get` (#21718) [ty] increase the limit on the number of elements in a non-recursively defined literal union (#21683) [ty] normalize typevar bounds/constraints in cycles (#21800) [ty] Update completion eval to include modules [ty] Add modules to auto-import [ty] Add support for module-only import requests [ty] Refactor auto-import symbol info [ty] Clarify the use of `SymbolKind` in auto-import [ty] Redact ranking of completions from e2e LSP tests [ty] Tweaks tests to use clearer language [ty] Update evaluation results [ty] Make auto-import ignore symbols in modules starting with a `_` ...
5 participants
Summary
Fixes astral-sh/ty#1688
Test Plan
N/A