Skip to content

[ty] Fix standalone expression type retrieval in presence of cycles#17849

Merged
sharkdp merged 1 commit into
mainfrom
david/fix-17792
May 5, 2025
Merged

[ty] Fix standalone expression type retrieval in presence of cycles#17849
sharkdp merged 1 commit into
mainfrom
david/fix-17792

Conversation

@sharkdp

@sharkdp sharkdp commented May 5, 2025

Copy link
Copy Markdown
Contributor

Summary

When entering an infer_expression_types cycle from TypeInferenceBuilder::infer_standalone_expression, we might get back a TypeInference::cycle_fallback(…) that doesn't actually contain any new types, but instead it contains a cycle_fallback_type which is set to Some(Type::Never). When calling self.extend(…), we therefore don't really pull in a type for the expression we're interested in. This caused us to panic if we tried to call self.expression_type(…) after self.extend(…).

The proposed fix here is to retrieve that type from the nested TypeInferenceBuilder directly, which will correctly fall back to cycle_fallback_type.

Details

I minimized the second example from #17792 a bit further and used this example for debugging:

from __future__ import annotations

class C: ...

def f(arg: C):
    pass

x, _ = f(1)

assert x

This is self-referential because when we check the assignment statement x, _ = f(1), we need to look up the signature of f. Since evaluation of annotations is deferred, we look up the public type of C for the arg parameter. The public use of C is visibility-constraint by "x" via the assert statement. While evaluating this constraint, we need to look up the type of x, which in turn leads us back to the x, _ = f(1) definition.

The reason why this only showed up in the relatively peculiar case with unpack assignments is the code here:

fn infer_target<F>(&mut self, target: &ast::Expr, value: &ast::Expr, infer_value_expr: F)
where
F: Fn(&mut TypeInferenceBuilder<'db>, &ast::Expr) -> Type<'db>,
{
let assigned_ty = match target {
ast::Expr::Name(_) => None,
_ => Some(infer_value_expr(self, value)),
};
self.infer_target_impl(target, assigned_ty);
}

For a non-unpack assignment like x = f(1), we would not try to infer the right-hand side eagerly. Instead, we would enter a infer_definition_types cycle that handles the situation correctly. For unpack assignments, however, we try to infer the type of value (f(1)) and therefore enter the cycle via standalone_expression_type => infer_expression_type.

closes #17792

Test Plan

@sharkdp sharkdp added the ty Multi-file analysis & type inference label May 5, 2025
@github-actions

github-actions Bot commented May 5, 2025

Copy link
Copy Markdown
Contributor

mypy_primer results

No ecosystem changes detected ✅

@sharkdp sharkdp force-pushed the david/fix-17792 branch from 3f16a5e to 393b847 Compare May 5, 2025 11:18
@sharkdp sharkdp changed the title [ty] Fix expression retrieval in presence of cycles [ty] Fix standalone expression type retrieval in presence of cycles May 5, 2025
@sharkdp sharkdp marked this pull request as ready for review May 5, 2025 11:36

@MichaReiser MichaReiser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice find and thanks for adding an inline comment!

@sharkdp sharkdp merged commit 1945bfd into main May 5, 2025
35 checks passed
@sharkdp sharkdp deleted the david/fix-17792 branch May 5, 2025 11:52
sharkdp added a commit that referenced this pull request May 5, 2025
## Summary

Adds `scipy` as a new project to `good.txt` as a follow-up to #17849.
Glyphack pushed a commit to Glyphack/ruff that referenced this pull request May 6, 2025
…stral-sh#17849)

## Summary

When entering an `infer_expression_types` cycle from
`TypeInferenceBuilder::infer_standalone_expression`, we might get back a
`TypeInference::cycle_fallback(…)` that doesn't actually contain any new
types, but instead it contains a `cycle_fallback_type` which is set to
`Some(Type::Never)`. When calling `self.extend(…)`, we therefore don't
really pull in a type for the expression we're interested in. This
caused us to panic if we tried to call `self.expression_type(…)` after
`self.extend(…)`.

The proposed fix here is to retrieve that type from the nested
`TypeInferenceBuilder` directly, which will correctly fall back to
`cycle_fallback_type`.

## Details

I minimized the second example from astral-sh#17792 a bit further and used this
example for debugging:

```py
from __future__ import annotations

class C: ...

def f(arg: C):
    pass

x, _ = f(1)

assert x
```

This is self-referential because when we check the assignment statement
`x, _ = f(1)`, we need to look up the signature of `f`. Since evaluation
of annotations is deferred, we look up the public type of `C` for the
`arg` parameter. The public use of `C` is visibility-constraint by "`x`"
via the `assert` statement. While evaluating this constraint, we need to
look up the type of `x`, which in turn leads us back to the `x, _ =
f(1)` definition.

The reason why this only showed up in the relatively peculiar case with
unpack assignments is the code here:


https://github.com/astral-sh/ruff/blob/78b4c3ccf1d6cb10613671ccec09cafba0d1de72/crates/ty_python_semantic/src/types/infer.rs#L2709-L2718

For a non-unpack assignment like `x = f(1)`, we would not try to infer
the right-hand side eagerly. Instead, we would enter a
`infer_definition_types` cycle that handles the situation correctly. For
unpack assignments, however, we try to infer the type of `value`
(`f(1)`) and therefore enter the cycle via `standalone_expression_type
=> infer_expression_type`.

closes astral-sh#17792 

## Test Plan

* New regression test
* Made sure that we can now run successfully on scipy => see astral-sh#17850
Glyphack pushed a commit to Glyphack/ruff that referenced this pull request May 6, 2025
## Summary

Adds `scipy` as a new project to `good.txt` as a follow-up to astral-sh#17849.
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

Successfully merging this pull request may close these issues.

[red-knot] Panic: "expression should belong to this TypeInference region"

2 participants