Conversation
Greptile SummaryThis PR adds support for Key Changes
Issues Found
Confidence Score: 3/5
Important Files Changed
|
Comment on lines
57
to
64
| if (block.terminal.kind === 'try') { | ||
| activeTryBlocks.push(block.terminal.handler); | ||
| // Add the try block itself to activeTryBlocks | ||
| activeTryBlocks.push(block.terminal.block); | ||
| // Also add handler if present | ||
| if (block.terminal.handler !== null) { | ||
| activeTryBlocks.push(block.terminal.handler); | ||
| } | ||
| } |
There was a problem hiding this comment.
logic: finalizer block not added to activeTryBlocks, so JSX in finally won't be validated
Suggested change
| if (block.terminal.kind === 'try') { | |
| activeTryBlocks.push(block.terminal.handler); | |
| // Add the try block itself to activeTryBlocks | |
| activeTryBlocks.push(block.terminal.block); | |
| // Also add handler if present | |
| if (block.terminal.handler !== null) { | |
| activeTryBlocks.push(block.terminal.handler); | |
| } | |
| } | |
| if (block.terminal.kind === 'try') { | |
| // Add the try block itself to activeTryBlocks | |
| activeTryBlocks.push(block.terminal.block); | |
| // Also add handler if present | |
| if (block.terminal.handler !== null) { | |
| activeTryBlocks.push(block.terminal.handler); | |
| } | |
| // Also add finalizer if present | |
| if (block.terminal.finalizer !== null) { | |
| activeTryBlocks.push(block.terminal.finalizer); | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoJSXInTryStatement.ts
Line: 57:64
Comment:
**logic:** finalizer block not added to `activeTryBlocks`, so JSX in finally won't be validated
```suggestion
if (block.terminal.kind === 'try') {
// Add the try block itself to activeTryBlocks
activeTryBlocks.push(block.terminal.block);
// Also add handler if present
if (block.terminal.handler !== null) {
activeTryBlocks.push(block.terminal.handler);
}
// Also add finalizer if present
if (block.terminal.finalizer !== null) {
activeTryBlocks.push(block.terminal.finalizer);
}
}
```
How can I resolve this? If you propose a fix, please make it concise.Much nicer workflow for working through errors in the compiler: * Run `yarn snap -w`, oops there are are errors * Hit 'p' to select a fixture => the suggestions populate with recent failures, sorted alphabetically. No need to copy/paste the name of the fixture you want to focus on! * tab/shift-tab to pick one, hit enter to select that one * ...Focus on fixing that test... * 'p' to re-enter the picker. Snap tracks the last state of each fixture and continues to show all tests that failed on their last run, so you can easily move on to the next one. The currently selected test is highlighted, making it easy to move to the next one. * 'a' at any time to run all tests * 'd' at any time to toggle debug output on/off (while focusing on a single test)
…opment Autogenerated summaries of each of the compiler passes which allow agents to get the key ideas of a compiler pass, including key input/output invariants, without having to reprocess the file each time. In the subsequent diff this seemed to help.
Optional chaining and other value blocks within try/catch blocks were throwing an Invariant error ("Unexpected terminal in optional") instead of the expected Todo error. This caused hard failures instead of graceful bailouts.
The issue occurred because DropManualMemoization and ValidateExhaustiveDependencies ran before BuildReactiveFunction, and encountered `maybe-throw` terminals in their switch statements without a handler, falling through to the invariant case.
Fixed by adding explicit `maybe-throw` cases in both files that throw the proper Todo error with the message "Support value blocks (conditional, logical, optional chaining, etc) within a try/catch statement".
Also renamed the existing bug test to reflect it's now correctly handled:
- error.bug-invariant-unexpected-terminal-in-optional → error.todo-optional-chaining-within-try-catch
Closes #35570
Fixes a longstanding issue where we didn't support code like `useFoo(value?.bar(), value?.bar()) ?? {}` - we would attempt to construct a ReactiveFunction, recursively processing the blocks, but the inner optional `value?.bar()` wouldn't match with what the outer optional was expecting to find. It's a one-line fix!
Note: memoization in the examples is not ideal, but i've confirmed that it is not strictly related to the optional issue.
# Summary note: This implements the idea discussed in reactwg/react#389 (comment) This is a large PR that significantly changes our impurity and ref validation to address multiple issues. The goal is to reduce false positives and make the errors we do report more actionable. ## Validating Against Impure Values In Render Currently we create `Impure` effects for impure functions like `Date.now()` or `Math.random()`, and then throw if the effect is reachable during render. However, impurity is a property of the resulting value: if the value isn't accessed during render then it's okay: maybe you're console-logging the time while debugging (fine), or storing the impure value into a ref and only accessing it in an effect or event handler (totally ok). This PR updates to validate that impure values are not transitively consumed during render, building on the new effects system: rather than look at instruction types, we use effects like `Capture a -> b`, `Impure a`, and `Render b` to determine how impure values are introduced and where they flow through the program. We're intentionally conservative, and do _not_ propagate impurity through MaybeCapture effects, which is used for functions we don't have signatures for. This means that things like `identity(performance.now())` drop the impurity. This feels like a good compromise since it means we have very high confidence in the errors that we report and can always add increased strictness later as our confidence increases. An example error: ``` Error: Cannot access impure value during render Calling an impure function can produce unstable results that update unpredictably when the component happens to re-render. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent). error.invalid-impure-value-in-render-helper.ts:5:17 3 | const now = () => Date.now(); 4 | const render = () => { > 5 | return <div>{now()}</div>; | ^^^^^ Cannot access impure value during render 6 | }; 7 | return <div>{render()}</div>; 8 | } error.invalid-impure-value-in-render-helper.ts:3:20 1 | // @validateNoImpureFunctionsInRender 2 | function Component() { > 3 | const now = () => Date.now(); | ^^^^^^^^^^ `Date.now` is an impure function. 4 | const render = () => { 5 | return <div>{now()}</div>; 6 | }; ``` Impure values are allowed to flow into refs, meaning that we now allow `useRef(Date.now())` or `useRef(localFunctionThatReturnsMathDotRandom())` which would have errored previously. The next PR reuses this improved impurity tracking to validate ref access in render as well. ## Refs Now Treated As Impure Values in Render Reading a ref now produces an `Impure` effect, and reading refs in render is validated using the above validation against impure values in render. The error category and message is customized for refs, we're just reusing the validation implementation. This means you get consistent results for `performance.now()` as for `ref.current`. A nice consistency win. ## Simplified writing-ref validation Now that _reading_ a ref in render is validated using the impure values infra, I also dramatically simplified ValidateNoRefAccessInRender to focus solely on validation against _writing_ refs during render. It was harder to use the new effects infra for this since we intentionally do not record ref mutations as a `Mutate` effect. So for now, the pass switches on InstructionValue variants. We continue to support the `if (ref.current == null) { ref.current = <init> }` pattern and reasonable variants of it. We're conservative about what we consider to be a write of a ref - `foo(ref)` now assumes you're not mutating rather than the inverse. # Takeaways * Impure-values-in-render logic is more conservative about what it considers an error (ie to users it appears more persmissive). We follow clearly identifiable (and if we wanted traceable/explainable) paths from impure sources through to where they are rendered. We allow many more cases than before, notably `x = foo(ref)` optimistically assumes you don't read the ref. Concretely, this follows from not tracking impure values across MaybeCapture effects. * No-writing-refs-in-render is also more conservative about what it counts as a write, appearing to users as allowing more cases. We look for very direct evidence of writing to refs, ie `ref.current = <value>` or `ref.current.property = <value>`, and allow potential writes through eg `writeToRef(ref, value)`.
First pass at adding support for `finally` clauses. I took a simple approach where we reify the JS semantics around return shadowing in the HIR and output. Ie, we create a temporary variable that will be the return value, and change returns in the try/catch to assign to that value and then break to the finalizer. In the finalizer returns work as normal, but we put a final extra "if temporary set, return it" statement. It would be more ideal to fully restore the return shadowing, but given that finally clauses are relatively rare this seems like a reasonable compromise. I need to do more analysis to make sure the approach is correct.
Comment on lines
57
to
+63
| if (block.terminal.kind === 'try') { | ||
| activeTryBlocks.push(block.terminal.handler); | ||
| // Add the try block itself to activeTryBlocks | ||
| activeTryBlocks.push(block.terminal.block); | ||
| // Also add handler if present | ||
| if (block.terminal.handler !== null) { | ||
| activeTryBlocks.push(block.terminal.handler); | ||
| } |
There was a problem hiding this comment.
logic: finalizer block not added to activeTryBlocks, JSX in finally won't be validated
Suggested change
| if (block.terminal.kind === 'try') { | |
| activeTryBlocks.push(block.terminal.handler); | |
| // Add the try block itself to activeTryBlocks | |
| activeTryBlocks.push(block.terminal.block); | |
| // Also add handler if present | |
| if (block.terminal.handler !== null) { | |
| activeTryBlocks.push(block.terminal.handler); | |
| } | |
| if (block.terminal.kind === 'try') { | |
| // Add the try block itself to activeTryBlocks | |
| activeTryBlocks.push(block.terminal.block); | |
| // Also add handler if present | |
| if (block.terminal.handler !== null) { | |
| activeTryBlocks.push(block.terminal.handler); | |
| } | |
| // Also add finalizer if present | |
| if (block.terminal.finalizer !== null) { | |
| activeTryBlocks.push(block.terminal.finalizer); | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoJSXInTryStatement.ts
Line: 57:63
Comment:
**logic:** finalizer block not added to `activeTryBlocks`, JSX in finally won't be validated
```suggestion
if (block.terminal.kind === 'try') {
// Add the try block itself to activeTryBlocks
activeTryBlocks.push(block.terminal.block);
// Also add handler if present
if (block.terminal.handler !== null) {
activeTryBlocks.push(block.terminal.handler);
}
// Also add finalizer if present
if (block.terminal.finalizer !== null) {
activeTryBlocks.push(block.terminal.finalizer);
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mirror of facebook/react#35607
Original author: josephsavona
First pass at adding support for
finallyclauses. I took a simple approach where we reify the JS semantics around return shadowing in the HIR and output. Ie, we create a temporary variable that will be the return value, and change returns in the try/catch to assign to that value and then break to the finalizer. In the finalizer returns work as normal, but we put a final extra "if temporary set, return it" statement. It would be more ideal to fully restore the return shadowing, but given that finally clauses are relatively rare this seems like a reasonable compromise.I need to do more analysis to make sure the approach is correct.
Stack created with Sapling. Best reviewed with ReviewStack.