[ty] Don't suggest things that aren't subclasses of `BaseException` after `raise` #21571

BurntSushi · 2025-11-21T19:36:27Z

This only applies to items that have a type associated with them. That
is, things that are already in scope. For items that don't have a type
associated with them (i.e., suggestions from auto-import), we still
suggest them since we can't know if they're appropriate or not. It's not
quite clear on how best to improve here for the auto-import case. (Short
of, say, asking for the type of each such symbol. But the performance
implications of that aren't known yet.)

Note that because of auto-import, we were still suggesting
NotImplemented even though astral-sh/ty#1262 specifically cites it as
the motivating example that we shouldn't suggest. This was occuring
because auto-import was including symbols from the builtins module,
even though those are actually already in scope. So this PR also gets
rid of those suggestions from auto-import.

Overall, this means that, at least, raise NotImpl won't suggest
NotImplemented.

Fixes astral-sh/ty#1262

BurntSushi · 2025-11-21T19:37:38Z

Demo:

2025-11-21T14.36.00-05.00.mp4

astral-sh-bot · 2025-11-21T19:39:09Z

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

astral-sh-bot · 2025-11-21T19:41:01Z

`mypy_primer` results

No ecosystem changes detected ✅

No memory usage changes detected ✅

Gankra

On paper it makes sense?

Gankra · 2025-11-21T20:21:14Z

crates/ty_ide/src/completion.rs

+    /// The maximum number of tokens we're willing to
+    /// look-behind to find a `raise` keyword.
+    const LIMIT: usize = 10;


Wow I need to review more of your PRs, y'all are wildin' in here. I assume you can't more-normally walk up the AST because autocomplete has to operate in malformed ASTs more?

I think it's more that I've been operating under the presumption that "if we can do things directly from the tokens, then that's probably better." In part to avoid the problem you bring up (although I've found our AST to be pretty good at dealing with malformed input) but also because it's very cheap to just look at a few tokens. It's possible that doing an AST traversal is also cheap enough. I honestly don't have a great sense of it yet, but for completions we do a lot of "check if we're in context foo, or bar, or baz, or quux..." so the faster each of those checks are (which I assume will continue to grow over time), the better. Although I did just remove a bunch of those checks for imports and consolidated it into one single check.

TL;DR - We could probably look at the AST for cases like this, but I'm starting simple.

Walking tokens is certainly faster, but walking the AST also isn't crazy expensive (all Ruff lint rules just do that). That's why I'd pick whatever is easier.

The bigger challenge with our AST is that you can't walk upwards (or sideways). At least not without building another intermediate representation that stores upward pointers for each AST node

One downside of using tokens is that there are cases where the parsed AST and token stream can disagree. E.g. the parser sometimes synthesizes name expressions if a required expression is missing (e.g. a. should synthesize a name expression for the attribute). However, we don't synthesize a name token in that case).

However, our lexing is, to some extent, parser-directed during error recovery. For example, a "normal" lexer would parse the whitespace before pass as such whitespace because there's an unclosed (. This is not the case for our token stream because the parser will inform the lexer that it will start error recovery after ( because pass isn't a valid argument name and that the lexer should try to lex the current token, assuming it's in a statement context, in which case the whitespace is lexed as an indent.

def test( pass

https://play.ruff.rs/f1d64305-c9ff-47cc-8edf-3a7216871932

However, there are a few cases where the parsed AST and the token stream can disagree, e.g. the token stream doesn't contain a newline before pass even though this is parsed as def test():\npass

def test(pass

https://play.ruff.rs/d9e759b6-d676-4871-b421-abbd58514fc9

Thanks for that explanation! I didn't know some of that. I think sticking with tokens for now is fine. We can always switch to AST in response to user feedback.

MichaReiser · 2025-11-22T14:22:10Z

crates/ty_ide/src/completion.rs

+    // mode here is that we may wind up suggesting things that
+    // shouldn't be raised. The benefit is that when this heuristic
+    // does work, we won't suggest things that shouldn't be raised.
+    for token in tokens.iter().rev().take(LIMIT) {


I think you want to, at least, skip all trivia tokens and parentheses ((, ))) to properly support

raise ( a # comment .Error )

AlexWaygood

Thank you! This looks fantastic.

Though... the more I think about this, the more edge cases I think of. What if a user has something like this?

import jsonschema.exceptions

if condition:
    raise jsons<CURSOR>

The user wants to type raise jsonschema.exceptions.SchemaError, but we refuse to suggest jsonschema here, and even after they've typed jsonschema.<CURSOR>, we refuse to suggest jsonschema.exceptions, since jsonschema and jsonschema.exceptions both have module-literal types, and module-literal types aren't assignable to type[BaseException] | BaseException.

I can think of three ways round this:

Make things more complicated: say that we'll also include module-literal types after raise keywords
Make things simpler: rather than trying to do a full analysis of which types make sense after a raise keyword in the autocompletion engine, just special-case a few symbols (like NotImplemented) that we know don't make sense after a raise keyword, and make sure that they're removed.
Somewhere in the middle? Rather than removing types that aren't assignable to type[BaseException] | BaseException from the list of suggestions, just make sure that they're all ranked below symbols that are assignable to type[BaseException] | BaseException. And maybe also special-case NotImplemented so that it's removed entirely?

I think I'm leaning towards (3). (1) just feels like it has too many edge cases attached to it, because there are lots of ways of creating nested namespaces in Python (not just modules and submodules!). Something like this is unusual in Python, but I can imagine it would be pretty surprising if a user tried it and then found they didn't have the autocompletions they expected (they want to type raise Namespace.Exception1):

class Namespace:
    class Exception1(ValueError): ...
    class Exception2(TypeError): ...
    class Exception3(RuntimeError): ...

raise Name<CURSOR>

(3) also seems like it'll have a lot less complexity associated with it than (1)

crates/ty_ide/src/completion.rs

BurntSushi · 2025-11-22T15:31:22Z

@AlexWaygood Great points. I like (3) too. I think that also fits better with symbols from auto-import not having a type associated with it. So it recasts the problem more as a ranking problem than a "don't show symbols" problem.

BurntSushi · 2025-11-24T16:47:26Z

@AlexWaygood OK, this should be updated now to do your (3) idea. Review is appreciated, particularly on how I did the check for NotImplemented. :-)

AlexWaygood

This is awesome!! It makes me ridiculously happy that NotImplemented is no longer suggested after raise, and that all the exceptions are suggested first here:

😃

AlexWaygood · 2025-11-24T16:57:15Z

crates/ty_ide/src/completion.rs

+        // As a special case, and because it's a common footgun, we
+        // specifically disallow `NotImplemented` in this context.
+        // `NotImplementedError` should be used instead. So if we can
+        // definitively detect `NotImplemented`, then we can safely
+        // omit it from suggestions.
+        let not_implemented_type = UnionType::from_elements(
+            db,
+            [
+                KnownClass::NotImplementedType.to_subclass_of(db),
+                KnownClass::NotImplementedType.to_instance(db),
+            ],
+        );
+        completions.retain(|item| {
+            let Some(ty) = item.ty else { return true };
+            !ty.is_assignable_to(db, not_implemented_type)
+        });


I think here you can simplify this a bit -- the big reason that folks mix up NotImplemented and NotImplementedError all the time is that they not only have very similar names, they're also both builtins. type(NotImplemented) doesn't really have the same issue, because it's not exposed as a builtin (you have to import the NotImplementedType class from the types module)

Suggested change

// As a special case, and because it's a common footgun, we

// specifically disallow `NotImplemented` in this context.

// `NotImplementedError` should be used instead. So if we can

// definitively detect `NotImplemented`, then we can safely

// omit it from suggestions.

let not_implemented_type = UnionType::from_elements(

db,

[

KnownClass::NotImplementedType.to_subclass_of(db),

KnownClass::NotImplementedType.to_instance(db),

],

);

completions.retain(|item| {

let Some(ty) = item.ty else { return true };

!ty.is_assignable_to(db, not_implemented_type)

});

// As a special case, and because it's a common footgun, we

// specifically disallow `NotImplemented` in this context.

// `NotImplementedError` should be used instead. So if we can

// definitively detect `NotImplemented`, then we can safely

// omit it from suggestions.

completions.retain(|item| {

let Some(ty) = item.ty else { return true };

!ty.is_notimplemented(db)

});

this will of course require making yet another ty_python_semantic API pub rather than pub(crate) 😆

ruff/crates/ty_python_semantic/src/types.rs

Lines 895 to 897 in a57e291

pub(crate) fn is_notimplemented(&self, db: &'db dyn Db) -> bool {

self.is_instance_of(db, KnownClass::NotImplementedType)

}

Yeah I guess I was thinking about

class Tricksy(NotImplementedType): pass

But I guess it's probably not worth that and better to focus on the specific footgun?

My approach also covered

tricksy = NotImplemented raise tricksy

But probably also not worth handling that?

Especially since both of my examples will get down-ranked anyway.

yeah, I think I'd just focus on the known footgun -- apart from anything else, NotImplementedType cannot be subclassed, so the user will get an error about Tricksy at runtime and ty will also complain about the invalid class definition: https://play.ty.dev/03244fb4-7141-43e4-baf8-240d405ac1d3

My approach also covered

tricksy = NotImplemented raise tricksy

that's also covered by my suggested approach! tricksy has the same type as the NotImplemented singleton here

Interesting. For whatever reason, the type that completions sees for tricksy is Never:

[crates/ty_ide/src/completion.rs:413:17] item = Completion { name: Name("tricksy"), insert: None, ty: Some( Never, ), kind: None, module_name: None, import: None, builtin: false, is_type_check_only: false, is_definitively_raisable: true, documentation: None, }

Example:

2025-11-24T12.21.46-05.00.mp4

I think it has something to do with it being used in a raise context?

(This is for my own edification at this point. I updated the PR to use your approach. :))

Interesting. For whatever reason, the type that completions sees for tricksy is Never:

hmm... that seems incorrect; there must be a bug somewhere. (Where it is I don't know -- probably in ty_python_semantic rather than ty_ide, though it might well be in ide_support.rs rather than one of the "core type-inference" parts of that crate.)

I think it has something to do with it being used in a raise context?

yes, that seems likely

…fter `raise` This only applies to items that have a type associated with them. That is, things that are already in scope. For items that don't have a type associated with them (i.e., suggestions from auto-import), we still suggest them since we can't know if they're appropriate or not. It's not quite clear on how best to improve here for the auto-import case. (Short of, say, asking for the type of each such symbol. But the performance implications of that aren't known yet.) Note that because of auto-import, we were still suggesting `NotImplemented` even though astral-sh/ty#1262 specifically cites it as the motivating example that we *shouldn't* suggest. This was occuring because auto-import was including symbols from the `builtins` module, even though those are actually already in scope. So this PR also gets rid of those suggestions from auto-import. Overall, this means that, at least, `raise NotImpl` won't suggest `NotImplemented`. Fixes astral-sh/ty#1262

BurntSushi requested review from AlexWaygood, MichaReiser, carljm, dcreager and sharkdp as code owners November 21, 2025 19:36

BurntSushi changed the title ~~ag/filter non exceptions after raise~~ [ty] Don't suggest things that aren't subclasses of BaseException after raise Nov 21, 2025

BurntSushi force-pushed the ag/filter-non-exceptions-after-raise branch from 5d1ca35 to ccf25e2 Compare November 21, 2025 19:37

BurntSushi removed request for carljm, dcreager and sharkdp November 21, 2025 19:37

BurntSushi added server Related to the LSP server ty Multi-file analysis & type inference labels Nov 21, 2025

BurntSushi force-pushed the ag/filter-non-exceptions-after-raise branch from ccf25e2 to d78ee47 Compare November 21, 2025 19:50

Gankra approved these changes Nov 21, 2025

View reviewed changes

MichaReiser reviewed Nov 22, 2025

View reviewed changes

AlexWaygood reviewed Nov 22, 2025

View reviewed changes

crates/ty_ide/src/completion.rs Outdated Show resolved Hide resolved

crates/ty_ide/src/completion.rs Show resolved Hide resolved

BurntSushi force-pushed the ag/filter-non-exceptions-after-raise branch from d78ee47 to 51f1ce4 Compare November 24, 2025 16:47

BurntSushi force-pushed the ag/filter-non-exceptions-after-raise branch from 51f1ce4 to 149860e Compare November 24, 2025 16:48

AlexWaygood approved these changes Nov 24, 2025

View reviewed changes

BurntSushi force-pushed the ag/filter-non-exceptions-after-raise branch from 149860e to 9856a12 Compare November 24, 2025 17:21

BurntSushi merged commit 68343e7 into main Nov 24, 2025
41 checks passed

BurntSushi deleted the ag/filter-non-exceptions-after-raise branch November 24, 2025 17:55

	pub(crate) fn is_notimplemented(&self, db: &'db dyn Db) -> bool {
	self.is_instance_of(db, KnownClass::NotImplementedType)
	}

[ty] Don't suggest things that aren't subclasses of BaseException after raise #21571

[ty] Don't suggest things that aren't subclasses of BaseException after raise #21571

Conversation

BurntSushi commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BurntSushi commented Nov 21, 2025

Uh oh!

astral-sh-bot bot commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Diagnostic diff on typing conformance tests

Uh oh!

astral-sh-bot bot commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mypy_primer results

Uh oh!

Gankra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexWaygood left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BurntSushi commented Nov 22, 2025

Uh oh!

BurntSushi commented Nov 24, 2025

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[ty] Don't suggest things that aren't subclasses of `BaseException` after `raise` #21571

[ty] Don't suggest things that aren't subclasses of `BaseException` after `raise` #21571

BurntSushi commented Nov 21, 2025 •

edited

Loading

astral-sh-bot bot commented Nov 21, 2025 •

edited

Loading

astral-sh-bot bot commented Nov 21, 2025 •

edited

Loading

`mypy_primer` results

MichaReiser Nov 22, 2025 •

edited

Loading

AlexWaygood left a comment •

edited

Loading

AlexWaygood Nov 24, 2025 •

edited

Loading

AlexWaygood Nov 24, 2025 •

edited

Loading