[red-knot] Emit an error if a bare Annotated or Literal is used in a type expression#14973
[red-knot] Emit an error if a bare Annotated or Literal is used in a type expression#14973AlexWaygood merged 2 commits intomainfrom
Annotated or Literal is used in a type expression#14973Conversation
There was a problem hiding this comment.
I moved this file from resources/mdtest/literal/literal.md to resources/mdtest/annotations/literal.md. That's where I expected to find it -- the other tests in the resources/mdtest/literal folder are all to do with checking we accurately infer types for objects created using Python literals -- ast::Dict expressions, etc.
…n a type expression
3ff7dca to
4297cf9
Compare
|
crates/red_knot_python_semantic/resources/mdtest/annotations/annotated.md
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/annotations/annotated.md
Show resolved
Hide resolved
| fn infer_invalid_type_expression( | ||
| &mut self, | ||
| expression: &ast::Expr, | ||
| error: InvalidTypeExpressionError<'db>, | ||
| ) -> Type<'db> { | ||
| let InvalidTypeExpressionError { | ||
| fallback_type, | ||
| invalid_expressions, | ||
| } = error; | ||
| for error in invalid_expressions { | ||
| self.diagnostics.add_lint( | ||
| &INVALID_TYPE_FORM, | ||
| expression.into(), | ||
| format_args!("{}", error.reason()), | ||
| ); | ||
| } | ||
| fallback_type | ||
| } |
There was a problem hiding this comment.
I feel like I slightly prefer the pattern we use in CallOutcome etc, where we have a method on the result type (or could just be on the error variant) that takes in a DiagnosticsBuilder and a node and emits the right diagnostics, returning the fallback type. This makes handling the result a bit more ergonomic (though you do have to explicitly pass in the diagnostics builder), and it prevents proliferating infer_* methods in TypeInferenceBuilder (which is already too large) that don't really fit the pattern of "infer a type for an AST node."
I realize this means passing AST nodes into types.rs, but I see the result/outcome types as kind of an intermediate layer between type inference builder and Type operations, that brings together nodes, diagnostics, and type operation results. I already moved CallOutcome out of types.rs; we could move more of those result/outcome types out of types.rs to clarify this layering.
This is all kind of musing, though; open to arguments for the pattern you used here.
There was a problem hiding this comment.
I do feel like a Result works really well here. There really are only two possibilities -- either it's okay, and we don't need to emit a diagnostic, or it isn't, and we do. If I were to write a custom *Outcome-like enum here, I think it would end up looking exactly like Result, so reinventing the wheel feels a little silly. And I just don't know what I'd call the enum 😆
But I totally take the point that the logic for unwrapping the error struct into a type (and emitting a diagnostic in the process) doesn't need to be on the TypeInferenceBuilder. I've moved it into an InvalidTypeExpressionError::into_fallback_type() method instead. I agree it's much nicer that way, and means we'll be able to easily move the enum into another submodule in the future if we want to.
26d8fb3 to
774ee81
Compare
* main: (25 commits) [`pydocstyle`] Skip leading whitespace for `D403` (#14963) Update pre-commit dependencies (#15008) Check diagnostic refresh support from client capability (#15014) Update Rust crate colored to v2.2.0 (#15010) Update dependency monaco-editor to v0.52.2 (#15006) Update Rust crate thiserror to v2.0.7 (#15005) Update Rust crate serde to v1.0.216 (#15004) Update Rust crate libc to v0.2.168 (#15003) Update Rust crate fern to v0.7.1 (#15002) Update Rust crate chrono to v0.4.39 (#15001) Update Rust crate bstr to v1.11.1 (#15000) Update NPM Development dependencies (#14999) Update dependency ruff to v0.8.3 (#15007) Pin mdformat plugins in pre-commit (#14992) Use stripping block (`|-`) for page descriptions (#14980) [`perflint`] Fix panic in `perf401` (#14971) Improve the documentation of E201/E202 (#14983) [ruff_python_ast] Add name and default functions to TypeParam. (#14964) [red-knot] Emit an error if a bare `Annotated` or `Literal` is used in a type expression (#14973) [red-knot] Fix bugs relating to assignability of dynamic `type[]` types (#14972) ...
Summary
These are both invalid annotations (the special forms must be parameterized --
Literalrequires at least one argument, andAnnotatedat least two) but we currently don't emit errors on them:This PR fixes that by modifying
Type::in_type_expressionto return aResult. TheOkvariant of the result is aType, and theErrvariant is an error struct that wraps:TypeInferenceBuilderTest Plan
New mdtests added, and TODOs in existing mdtests have been fixed.