Maybe fix parent setting race

[jakebailey]

The only think I can think for #1224 is that we're hitting the reparsed case where we want to overwrite the parent, but have already done so somewhere else (as in, during references collection originally), so are writing the same value back and create a race. That's what #970 fixed for other cases, but #1031 might have added that back for the reparsed cases?

It's not 100% clear to me anymore why the special case exists to always reassign reparsed parents, though. But, removing it changes baselines in ways that look wrong, so it must be doing something and I just don't remember why it matters anymore.

Or, maybe this PR won't work, because the issue is explicitly that reparsed case not meshing well?

Fixes #1224

(maybe?)

[Copilot]

Pull Request Overview

This PR updates the logic for reassigning a node’s parent in the binder to avoid redundant resets in reparsed cases.

• Refines the if condition to only overwrite the parent when the node has no parent or has a different reparsed parent.
• Aims to prevent a race where the parent gets set back to the same value unnecessarily.

Comments suppressed due to low confidence (1)

internal/binder/binder.go:587

• Consider adding a unit test for the case where node.Parent == b.parent and the NodeFlagsReparsed flag is set, to verify that the parent is not reassigned and to prevent future regressions.

	if node.Parent == nil || node.Parent != b.parent && node.Parent.Flags&ast.NodeFlagsReparsed != 0 {

[andrewbranch]

I don’t think this fixes the root cause. I had this exact code in one of my fixes at one point and then realized that it should be unnecessary if other invariants hold:

1. Any node that gets its .Parent queried as part of module resolution should have it set to its final value during parsing
2. A real node may only have a reparsed node as a parent temporarily. By the end of binding, real nodes should have real parents.

It would be fine to adjust (2) to say that a real node may never have a reparsed parent, but I found it was easier to allow a temporary assignment in cases where we traverse down a reparsed node into real children before seeing the real parent of those real children—I don’t remember the details of when this happens or why it was easier (also Nathan has made some adjustments to how binding interacts with reparsed nodes since then).

I eventually developed a viable strategy for debugging this, which I don’t remember off the top of my head but can probably reconstruct. I’ll take a look at this.

[jakebailey]

Yeah, I don't have a great mental model about this, clearly.

I think I was thinking that it could only be the former, until I realized that the file loader must necessarily only ever touch nodes that were previously provided by the references code. So, I dunno.