Skip to content

[red-knot] add narrowing support in match class patterns #14740

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

Closed
carljm opened this issue Dec 2, 2024 · 3 comments
Closed

[red-knot] add narrowing support in match class patterns #14740

carljm opened this issue Dec 2, 2024 · 3 comments
Assignees
Labels
ty Multi-file analysis & type inference

Comments

@carljm
Copy link
Contributor

carljm commented Dec 2, 2024

The goal of this issue is to make this mdtest pass:

def get_bool() -> bool: ...
def get_object() -> object: ...
class A: ...
class B: ...

x = get_object()

reveal_type(x)  # revealed: object

match x:
    case A():
        reveal_type(x)  # revealed: A
    case B():
        reveal_type(x)  # revealed: B

reveal_type(x)  # revealed: object

This should just require filling in the TODO for MatchClass at

We don't need to worry about arguments yet; the goal here is just to narrow based on the implicit isinstance check performed by the class pattern.

See https://docs.python.org/3/reference/compound_stmts.html#class-patterns for the documentation of the language feature.

@carljm carljm added the ty Multi-file analysis & type inference label Dec 2, 2024
@carljm carljm self-assigned this Dec 2, 2024
@InSyncWithFoo
Copy link
Contributor

I looked into this and got a major block:

match pattern.pattern(self.db).node() {
    // ...
    ast::Pattern::MatchClass(PatternMatchClass { cls, .. }) => {
        let inference = infer_scope_types(self.db, self.scope());  // This panics.
    }
    // ...
}

The last logged message is Box<dyn Any>.

The problem appears to be reproducible with just:

```py
x = object()

match x:
    case int():
        x  # Or any name
```

This comment seems relevant:

// SAFETY: we should always have a symbol for every Name node.

@carljm
Copy link
Contributor Author

carljm commented Dec 9, 2024

The inscrutable Box<dyn Any> error message indicates a Salsa query cycle. In this case it would be due to calling infer_scope_types in type narrowing, when in turn type narrowing may well have been called by infer_scope_types on that same scope.

For expressions that function as narrowing predicates, we will usually need to add them as standalone expressions when building the semantic index, so we can query their type independently. Though since match patterns don't use Expr but rather ComparableExpr, this will probably require adding some new infrastructure for standalone ComparableExpr.

(Note this task is assigned to me, it's not up-for-grabs at the moment.)

@sharkdp
Copy link
Contributor

sharkdp commented Dec 11, 2024

Just a quick note that I touched some of the relevant code in #14759. So maybe contact me before starting with this task (in case #14759 has not been merged in the meantime).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ty Multi-file analysis & type inference
Projects
None yet
Development

No branches or pull requests

4 participants