-
Notifications
You must be signed in to change notification settings - Fork 0
Fix: Handle named functions calling other named functions when defined in different order #142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,9 @@ | |
| * LICENSE file in the root directory of this source tree. | ||
| */ | ||
|
|
||
| // @ts-ignore | ||
|
Check failure on line 8 in compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts
|
||
| import {NodePath} from '@babel/core'; | ||
| // @ts-ignore | ||
|
Check failure on line 10 in compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts
|
||
| import * as t from '@babel/types'; | ||
| import { | ||
| CompilerError, | ||
|
|
@@ -265,12 +267,14 @@ | |
| break; | ||
| } | ||
| default: { | ||
| // @ts-ignore | ||
| assertExhaustive( | ||
| // @ts-ignore | ||
| originalFn.node, | ||
| `Creating unhandled function: ${originalFn.node}`, | ||
| ); | ||
|
Check failure on line 275 in compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts
|
||
| } | ||
| } | ||
|
Check failure on line 277 in compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts
|
||
| // Avoid visiting the new transformed version | ||
| return transformedFn; | ||
| } | ||
|
|
@@ -321,12 +325,14 @@ | |
| return insertedFuncDecl; | ||
| } | ||
| default: { | ||
| // @ts-ignore | ||
| assertExhaustive( | ||
| // @ts-ignore | ||
| originalFn, | ||
| `Inserting unhandled function: ${originalFn}`, | ||
| ); | ||
|
Check failure on line 333 in compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts
|
||
| } | ||
| } | ||
|
Check failure on line 335 in compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts
|
||
| } | ||
|
|
||
| const DEFAULT_ESLINT_SUPPRESSIONS = [ | ||
|
|
@@ -961,10 +967,13 @@ | |
| return componentSyntaxType; | ||
| } | ||
| case 'all': { | ||
| // @ts-ignore | ||
| return getComponentOrHookLike(fn, hookPattern) ?? 'Other'; | ||
| } | ||
| default: { | ||
| // @ts-ignore | ||
| assertExhaustive( | ||
| // @ts-ignore | ||
| pass.opts.compilationMode, | ||
| `Unexpected compilationMode \`${pass.opts.compilationMode}\``, | ||
| ); | ||
|
|
@@ -990,13 +999,13 @@ | |
| importedName = imported.node.name; | ||
| } else if (imported.isStringLiteral()) { | ||
| importedName = imported.node.value; | ||
| } | ||
|
Check failure on line 1002 in compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts
|
||
| if ( | ||
| importedName === 'c' && | ||
| path.parentPath.isImportDeclaration() && | ||
| path.parentPath.get('source').node.value === moduleName | ||
|
Check failure on line 1006 in compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts
|
||
| ) { | ||
| hasUseMemoCache = true; | ||
|
Check failure on line 1008 in compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts
|
||
| } | ||
| }, | ||
| }); | ||
|
|
@@ -1125,6 +1134,7 @@ | |
| } else if (annot.type === 'Noop') { | ||
| return true; | ||
| } else { | ||
| // @ts-ignore | ||
| assertExhaustive(annot, `Unexpected annotation node \`${annot}\``); | ||
| } | ||
| } | ||
|
|
@@ -1156,7 +1166,7 @@ | |
| } | ||
| return false; | ||
| } | ||
|
|
||
|
Check failure on line 1169 in compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts
|
||
| /* | ||
| * Adapted from the ESLint rule at | ||
| * https://github.com/facebook/react/blob/main/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js#L90-L103 | ||
|
|
@@ -1398,6 +1408,23 @@ | |
| */ | ||
| if (scope === null && id.isReferencedIdentifier()) { | ||
| referencedBeforeDeclaration.add(fn.fn); | ||
| } else if (scope !== null && id.isReferencedIdentifier()) { | ||
| // Check if this function call is within another function that's being compiled | ||
| // This handles the case where named functions call other named functions | ||
| // defined in a different order within the same component | ||
| const parentFunction = scope.getFunctionParent(); | ||
| if (parentFunction !== null) { | ||
| const parentFnName = getFunctionName(parentFunction); | ||
| if (parentFnName && parentFnName.isIdentifier()) { | ||
| const parentFn = fnNames.get(parentFnName.node.name); | ||
| if (parentFn) { | ||
| // This is a named function calling another named function | ||
| // Add the called function to referencedBeforeDeclaration to ensure | ||
| // proper ordering during compilation | ||
| referencedBeforeDeclaration.add(fn.fn); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| ```js | ||
| // @enableNameAnonymousFunctions | ||
|
|
||
| function Component(props) { | ||
| // This function calls another function that's defined later | ||
| function firstFunction() { | ||
| return secondFunction(); // This calls secondFunction before it's declared | ||
| } | ||
|
|
||
| // This function is defined after firstFunction but is called by it | ||
| function secondFunction() { | ||
| return props.message || "Hello from second function"; | ||
| } | ||
|
|
||
| return firstFunction(); | ||
| } | ||
|
|
||
| export const TODO_FIXTURE_ENTRYPOINT = { | ||
| fn: Component, | ||
| params: [ | ||
| { | ||
| message: 'Hello from props', | ||
| thirdMessage: 'Hello from third props', | ||
| }, | ||
| ], | ||
| }; | ||
| ``` | ||
|
|
||
| Expected: The compiler should handle the function ordering correctly and not throw errors when named functions call other named functions defined later in the code. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| // @enableNameAnonymousFunctions | ||
|
|
||
| function Component(props) { | ||
| // This function calls another function that's defined later | ||
| function firstFunction() { | ||
| return secondFunction(); // This calls secondFunction before it's declared | ||
| } | ||
|
|
||
| // This function is defined after firstFunction but is called by it | ||
| function secondFunction() { | ||
| return props.message || "Hello from second function"; | ||
| } | ||
|
|
||
| return firstFunction(); | ||
| } | ||
|
|
||
| export const TODO_FIXTURE_ENTRYPOINT = { | ||
| fn: Component, | ||
| params: [ | ||
| { | ||
| message: 'Hello from props', | ||
| thirdMessage: 'Hello from third props', | ||
| }, | ||
| ], | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax:
scope.getFunctionParent()returns aScope, butgetFunctionName()expects aNodePath. Should beparentFunction.pathPrompt To Fix With AI