Skip to content
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.

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

[red-knot] Emit an error if a bare Annotated or Literal is used in a type expression #14973

Merged
merged 2 commits into from
Dec 15, 2024

Conversation

AlexWaygood
Copy link
Member

Summary

These are both invalid annotations (the special forms must be parameterized -- Literal requires at least one argument, and Annotated at least two) but we currently don't emit errors on them:

from typing import Literal, Annotated

x: Literal
y: Annotated

This PR fixes that by modifying Type::in_type_expression to return a Result. The Ok variant of the result is a Type, and the Err variant is an error struct that wraps:

  • the necessary information for emitting a diagnostic from the TypeInferenceBuilder
  • the type that we should fallback to in the event of an error

Test Plan

New mdtests added, and TODOs in existing mdtests have been fixed.

@AlexWaygood AlexWaygood added the red-knot Multi-file analysis & type inference label Dec 14, 2024
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

github-actions bot commented Dec 14, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Nice!

crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
Comment on lines 4636 to 4653
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
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@AlexWaygood AlexWaygood merged commit 1389cb8 into main Dec 15, 2024
21 checks passed
@AlexWaygood AlexWaygood deleted the alex/bare-annotated branch December 15, 2024 02:00
dcreager added a commit that referenced this pull request Dec 16, 2024
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants