[red-knot] Do not panic if named expressions show up in assignment position

[rtpg]

Unfortunately the test I added here still fails (I believe due to scoping issues, still trying to wrap my head around how the scoping is supposed to work), but I believe my changes to the builder are correct: it is not incorrect for there to be current_assignments to juggle, given the existence of named expressions.

[carljm]

Thank you!! I had totally failed to consider the possibility of a namedexpr in a LHS subscript expression.

[carljm]

This is invalid syntax. Red-knot isn't resilient to arbitrary invalid syntax yet (though we should become so). We should make a change so that for now red-knot just always fails fast if there are parser errors, rather than hitting panics that are hard to debug and cause confusion; this isn't the first time we've had this scenario.

For this to be valid syntax, the namedexpr has to be parenthesized to fix the binding precedence. If we do this, the test passes:

Suggested change

	x[0 if y := 2 else 1] = 1
	x[0 if (y := 2) else 1] = 1

[carljm]

The new mdtest framework will already help with this, since it won't hide syntax errors like the current tests do. Currently it will just fail fast on any test with a syntax error in it; once we start working on syntax error resilience we will add features to it to explicitly assert on syntax errors, but tests with accidental syntax errors will still fail with a clear diagnostic.

[rtpg]

Ah, great catch, will fix the test. Glad to see these changes are able to be handled.

My current reading is that the semantic index expects everything to be covered so even in the case of syntax errors we would be expected to try and see each symbol anyways... but that might be me buying into the error's premise a bit too much.

Like should the semantic index cover the entire program? If so, it feels like the underlying error is still a problem. If not, then it feels like there's some concept here of not being covered (and some of the philosophy of #13701, of having lookups be Option/Result-y and falling back, could apply).

[carljm]

The panic here with the syntax error isn't exactly due to lack of coverage. It's that we currently assume (reasonably!) that if a name is being defined, there will be a Name node with Store context; that's normally how the AST works. With the malformed syntax, we end up with a Named expression node whose target doesn't contain any Name node with Store context. So we never create any Definition for the Named. But then type inference assumes that every Named node should be associated with a Definition, it tries to look up the Definition and there isn't one.

I agree with you in general that becoming syntax-error-resilient will probably mean making fewer such invariant assumptions, and being more willing to fall back to Unknown when something just doesn't make sense. The loss with doing this unconditionally is that it becomes easier for bugs to go unnoticed and propagate silently. Ideally perhaps we could relax our invariants only when we actually observe invalid nodes in the AST, rather than doing so unconditionally, to preserve a bit more fail-fast when we have bugs. I'm not sure if this will be feasible or not. The truth is that we have a lot to do and syntax-error-resilience hasn't made it to the top of the list yet!

I think this particular panic would be fixed by having infer_named_expr in type inference check if the target of the Named node is a Name with Store context (which it always should be, if syntax is valid -- walrus expressions aren't allowed to have complex left-hand side), and if not, bail out and don't try to look up a Definition at all. But I would kind of like to consider our strategy for syntax-error-resilience a bit more holistically.

cc @dhruvmanila who is intending to take on the syntax-error-resilience at some point.

I hadn't seen #13701 yet. (Note, I generally won't be alerted to the existence of draft PRs unless explicitly pointed to them; they don't show up in reviewer notifications until marked ready for review.) I'll take a look at it soon (may not be til Monday.)

[rtpg]

I am a bit mystified by the benchmark/ecosystem failures but I think the code changes here are ready for another review.

[MichaReiser]

The benchmark fail at

ruff/crates/red_knot_python_semantic/src/semantic_index/builder.rs

Line 420 in 47a3625

assert_eq!(self.current_assignments.len(), 0);

Indicating that there are assignments that weren't popped correctly or that the assertion needs updating with your changes.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[carljm]

Looks great, thank you! I will go ahead and move the tests and then merge.