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.

Sign up for GitHub

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

Jump to bottom

[red-knot] Improve type inference for except handlers where a tuple of exception classes is caught #13646

Merged
merged 1 commit into from
Oct 7, 2024
Merged

[red-knot] Improve type inference for except handlers where a tuple of exception classes is caught #13646

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 54 additions & 4 deletions crates/red_knot_python_semantic/src/types/infer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1030,10 +1030,17 @@ impl<'db> TypeInferenceBuilder<'db> {
} else {
// TODO: anything that's a consistent subtype of
// `type[BaseException] | tuple[type[BaseException], ...]` should be valid;
// anything else should be invalid --Alex
// anything else is invalid and should lead to a diagnostic being reported --Alex
match node_ty {
Type::Any | Type::Unknown => node_ty,
Type::Class(class_ty) => Type::Instance(class_ty),
Type::Tuple(tuple) => UnionType::from_elements(
self.db,
tuple
.elements(self.db)
.iter()
.map(|ty| ty.into_class_type().map_or(Type::Todo, Type::Instance)),
),
_ => Type::Todo,
}
};
Expand Down Expand Up @@ -6151,11 +6158,54 @@ mod tests {
",
)?;

// For these TODOs we need support for `tuple` types:
let expected_diagnostics = &[
// TODO: Should be `RuntimeError | OSError` --Alex
"Revealed type is `RuntimeError | OSError`",
"Revealed type is `AttributeError | TypeError`",
];

assert_file_diagnostics(&db, "src/a.py", expected_diagnostics);

Ok(())
}

#[test]
fn except_handler_dynamic_exceptions() -> anyhow::Result<()> {
let mut db = setup_db();

db.write_dedented(
"src/a.py",
"
from typing_extensions import reveal_type

def foo(
x: type[AttributeError],
y: tuple[type[OSError], type[RuntimeError]],
z: tuple[type[BaseException], ...]
):
try:
w
except x as e:
reveal_type(e)
except y as f:
reveal_type(f)
except z as g:
reveal_type(g)
",
)?;

let expected_diagnostics = &[
// TODO: these `__class_getitem__` diagnostics are all false positives:
// (`builtins.type` is unique at runtime
// as it can be subscripted even though it has no `__class_getitem__` method)
"Cannot subscript object of type `Literal[type]` with no `__class_getitem__` method",
"Cannot subscript object of type `Literal[type]` with no `__class_getitem__` method",
"Cannot subscript object of type `Literal[type]` with no `__class_getitem__` method",
"Cannot subscript object of type `Literal[type]` with no `__class_getitem__` method",
Comment on lines +6197 to +6203
Copy link
Member Author

Choose a reason for hiding this comment

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

Cc. @charliermarsh -- this is a little edge case in the logic you added in #13579. I think the special-casing that allows e.g. type[int] at runtime is buried deep in the interpreter somewhere. According to all the normal rules, it shouldn't be subscriptable... but it is!

Python 3.12.4 (main, Jun 12 2024, 09:54:41) [Clang 15.0.0 (clang-1500.3.9.4)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> type[int]
type[int]
>>> type.__getitem__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: type object 'type' has no attribute '__getitem__'. Did you mean: '__getstate__'?
>>> type.__class_getitem__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: type object 'type' has no attribute '__class_getitem__'
>>> type.__dict__.keys()
dict_keys(['__new__', '__repr__', '__call__', '__getattribute__', '__setattr__', '__delattr__', '__init__', '__or__', '__ror__', 'mro', '__subclasses__', '__prepare__', '__instancecheck__', '__subclasscheck__', '__dir__', '__sizeof__', '__basicsize__', '__itemsize__', '__flags__', '__weakrefoffset__', '__base__', '__dictoffset__', '__name__', '__qualname__', '__bases__', '__mro__', '__module__', '__abstractmethods__', '__dict__', '__doc__', '__text_signature__', '__annotations__', '__type_params__'])

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this is literally hardcoded directly into PyObject_GetItem as a special case 🫣

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'm working on a PR now to add the special case to our semantic model...

Copy link
Member Author

Choose a reason for hiding this comment

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

#13667 (comment)

// Should be `AttributeError`:
"Revealed type is `@Todo`",
// Should be `OSError | RuntimeError`:
"Revealed type is `@Todo`",
// TODO: Should be `AttributeError | TypeError` --Alex
// Should be `BaseException`:
"Revealed type is `@Todo`",
];

Expand Down
Loading