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] Add a diagnostic for raise statements used with non-exceptions #15038

Closed
AlexWaygood opened this issue Dec 17, 2024 · 4 comments · Fixed by #15042
Closed

[red-knot] Add a diagnostic for raise statements used with non-exceptions #15038

AlexWaygood opened this issue Dec 17, 2024 · 4 comments · Fixed by #15042
Labels
help wanted Contributions especially welcome red-knot Multi-file analysis & type inference

Comments

@AlexWaygood
Copy link
Member

AlexWaygood commented Dec 17, 2024

Red-knot currently doesn't emit a diagnostic on this snippet:

raise 42

But we should, since 42 is not a valid object to be used in a raise statement:

>>> raise 42
Traceback (most recent call last):
  File "<python-input-0>", line 1, in <module>
    raise 42
TypeError: exceptions must derive from BaseException

The rule is that the object used in a raise statement must have a type that is assignable to the type BaseException | type[BaseException], since both of the following work:

>>> raise ValueError
Traceback (most recent call last):
  File "<python-input-1>", line 1, in <module>
    raise ValueError
ValueError
>>> raise ValueError()
Traceback (most recent call last):
  File "<python-input-2>", line 1, in <module>
    raise ValueError()
ValueError

We need to add some extra logic to this branch here:

fn infer_raise_statement(&mut self, raise: &ast::StmtRaise) {
let ast::StmtRaise {
range: _,
exc,
cause,
} = raise;
self.infer_optional_expression(exc.as_deref());
self.infer_optional_expression(cause.as_deref());
}

I think this will probably need to be a new rule added to https://github.com/astral-sh/ruff/blob/main/crates/red_knot_python_semantic/src/types/diagnostic.rs ? I don't think it fits neatly into any other rule. The alternative might be to make the existing INVALID_EXCEPTION_CAUGHT rule broader (and rename it).

@AlexWaygood AlexWaygood added help wanted Contributions especially welcome red-knot Multi-file analysis & type inference labels Dec 17, 2024
@MichaReiser
Copy link
Member

The alternative might be to make the existing INVALID_EXCEPTION_CAUGHT rule broader (and rename it).

That was also my first reaction. It's kind of the same problem.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Dec 17, 2024

The alternative might be to make the existing INVALID_EXCEPTION_CAUGHT rule broader (and rename it).

That was also my first reaction. It's kind of the same problem.

The same genre of problem, for sure. Though there's a subtle difference between the rules for raising exceptions and the rules for catching exceptions:

  • If you're raising an exception, the type must be assignable to BaseException | type[BaseException]
  • but if you're catching an exception the type must be assignable to type[BaseException] | tuple[type[BaseException], ...]

If we bundle them together into one rule, it might be harder to explain these details in the rule's documentation.

@AlexWaygood
Copy link
Member Author

I don't have a strong opinion, though. Any suggestions for what we might call the rule if we combined the two into one one rule?

@InSyncWithFoo
Copy link
Contributor

Survey:

I think INVALID_EXCEPTION is good enough. Each diagnostic can then customize the message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Contributions especially welcome red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants