next.js

f76e1dc7 - Turbopack: Replace `current_value` set/restore mutation pattern with a safer `with_pat_value` helper (#81696)

Turbopack: Replace `current_value` set/restore mutation pattern with a safer `with_pat_value` helper (#81696) The previous pattern of ``` self.current_value = Some(...); // visit children self.current_value = None; ``` felt very dangerous to me, because it's too easy to not restore `current_value` (or accidentally clear it instead of restoring it), which could lead to really subtle bugs. This wraps it with a helper method (`with_pat_value`) that stores the previous value and restores it after exit. It uses an inner module with private struct fields to prevent accidental direct mutation of the `pat_value`. ### Renaming `current_value` to `pat_value` I decided to rename it to `pat_value`, as that's a little more specific to how it's used than `current_value`. It's still not a perfect name though because simple assignments are arguably not patterns. ### Other places that could use this pattern There are other fields that use this set/restore pattern (e.g. `early_return_stack`), so if accepted, I'll make more PRs to migrate those to this pattern as well. ### Why not use a guard object? A guard object would need access to `&mut Analyzer` in order to reset the state upon drop. The guard object needs to be held across visiting children, which also requires `&mut Analyzer`. So we'd need two simultaneous mutable references. This could be done with a `Rc<RefCell<...>>`, but that's not worth it. ### Performance considerations The analyzer is a hot codepath. This may clear `pat_value` in a few more places than needed, e.g. inside of loops where we know the `pat_value` will immediately be overwritten anyways by the next iteration of the loop. But this shouldn't be a significant difference, and might get compiled away in some cases. I'm running the full benchmark suite just in case.