Skip to content
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

[compiler] Run compiler pipeline on 'use no forget' #30720

Merged
merged 8 commits into from
Aug 16, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,12 @@ export type LoggerEvent =
fnLoc: t.SourceLocation | null;
detail: Omit<Omit<CompilerErrorDetailOptions, 'severity'>, 'suggestions'>;
}
| {
kind: 'CompileSkip';
fnLoc: t.SourceLocation | null;
reason: string;
loc: t.SourceLocation | null;
}
| {
kind: 'CompileSuccess';
fnLoc: t.SourceLocation | null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,34 +43,23 @@ export type CompilerPass = {
comments: Array<t.CommentBlock | t.CommentLine>;
code: string | null;
};
const OPT_IN_DIRECTIVES = new Set(['use forget', 'use memo']);
export const OPT_OUT_DIRECTIVES = new Set(['use no forget', 'use no memo']);

function findDirectiveEnablingMemoization(
directives: Array<t.Directive>,
): t.Directive | null {
for (const directive of directives) {
const directiveValue = directive.value.value;
if (directiveValue === 'use forget' || directiveValue === 'use memo') {
return directive;
}
}
return null;
): Array<t.Directive> {
return directives.filter(directive =>
OPT_IN_DIRECTIVES.has(directive.value.value),
);
}

function findDirectiveDisablingMemoization(
directives: Array<t.Directive>,
options: PluginOptions,
): t.Directive | null {
for (const directive of directives) {
const directiveValue = directive.value.value;
if (
(directiveValue === 'use no forget' ||
directiveValue === 'use no memo') &&
!options.ignoreUseNoForget
) {
return directive;
}
}
return null;
): Array<t.Directive> {
return directives.filter(directive =>
OPT_OUT_DIRECTIVES.has(directive.value.value),
);
}

function isCriticalError(err: unknown): boolean {
Expand Down Expand Up @@ -102,7 +91,7 @@ export type CompileResult = {
compiledFn: CodegenFunction;
};

function handleError(
function logError(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split this up so we can decide to only log in some cases instead of throwing (eg a file contains at least one function that was compiled although there were opt outs + errors in others)

err: unknown,
pass: CompilerPass,
fnLoc: t.SourceLocation | null,
Expand Down Expand Up @@ -131,6 +120,13 @@ function handleError(
});
}
}
}
function handleError(
err: unknown,
pass: CompilerPass,
fnLoc: t.SourceLocation | null,
): void {
logError(err, pass, fnLoc);
if (
pass.opts.panicThreshold === 'all_errors' ||
(pass.opts.panicThreshold === 'critical_errors' && isCriticalError(err)) ||
Expand Down Expand Up @@ -430,11 +426,65 @@ export function compileProgram(
prunedMemoValues: compiledFn.prunedMemoValues,
});
} catch (err) {
/**
* If an opt out directive is present, log only instead of throwing and don't mark as
* containing a critical error.
*/
if (fn.node.body.type === 'BlockStatement') {
const optOutDirectives = findDirectiveDisablingMemoization(
fn.node.body.directives,
);
if (optOutDirectives.length > 0) {
logError(err, pass, fn.node.loc ?? null);
return null;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you need similar logic to this above on https://github.com/facebook/react/pull/30720/files#diff-def62b77d903a117d9a493b69a884f79317eb8c8beac025622265c99f1c7189fR403 (ugh, why doesn't GH let you comment on lines that weren't changed by a PR)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yeah, thanks for catching this!

hasCriticalError ||= isCriticalError(err);
handleError(err, pass, fn.node.loc ?? null);
return null;
}

if (fn.node.body.type === 'BlockStatement') {
const optInDirectives = findDirectiveEnablingMemoization(
fn.node.body.directives,
);
const optOutDirectives = findDirectiveDisablingMemoization(
fn.node.body.directives,
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could probably just calculate these once at the top of processFn, then reference the result here and above in the error case?


/**
* Always compile functions with opt in directives.
*/
if (optInDirectives.length > 0) {
return compiledFn;
} else if (pass.opts.compilationMode === 'annotation') {
/**
* No opt-in directive in annotation mode, so don't insert the compiled function.
*/
return null;
}

/**
* Otherwise if 'use no forget/memo' is present, we still run the code through the compiler
* for validation but we don't mutate the babel AST. This allows us to flag if there is an
* unused 'use no forget/memo' directive.
*/
if (
pass.opts.ignoreUseNoForget === false &&
optOutDirectives.length > 0
) {
for (const directive of optOutDirectives) {
pass.opts.logger?.logEvent(pass.filename, {
kind: 'CompileSkip',
fnLoc: fn.node.body.loc ?? null,
reason: `Skipped due to '${directive.value.value}' directive.`,
loc: directive.loc ?? null,
});
}
return null;
}
}

if (!pass.opts.noEmit && !hasCriticalError) {
return compiledFn;
}
Expand Down Expand Up @@ -481,6 +531,16 @@ export function compileProgram(
});
}

/**
* Do not modify source if there is a module scope level opt out directive.
*/
const moduleScopeOptOutDirectives = findDirectiveDisablingMemoization(
program.node.directives,
);
if (moduleScopeOptOutDirectives.length > 0) {
return;
}

if (pass.opts.gating != null) {
const error = checkFunctionReferencedBeforeDeclarationAtTopLevel(
program,
Expand Down Expand Up @@ -596,24 +656,6 @@ function shouldSkipCompilation(
}
}

// Top level "use no forget", skip this file entirely
const useNoForget = findDirectiveDisablingMemoization(
program.node.directives,
pass.opts,
);
if (useNoForget != null) {
pass.opts.logger?.logEvent(pass.filename, {
kind: 'CompileError',
fnLoc: null,
detail: {
severity: ErrorSeverity.Todo,
reason: 'Skipped due to "use no forget" directive.',
loc: useNoForget.loc ?? null,
suggestions: null,
},
});
return true;
}
Comment on lines -599 to -616
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously this would skip visiting the function at all, so no validations could be checked.

const moduleName = pass.opts.runtimeModule ?? 'react/compiler-runtime';
if (hasMemoCacheFunctionImport(program, moduleName)) {
return true;
Expand All @@ -631,28 +673,8 @@ function getReactFunctionType(
): ReactFunctionType | null {
const hookPattern = environment.hookPattern;
if (fn.node.body.type === 'BlockStatement') {
// Opt-outs disable compilation regardless of mode
const useNoForget = findDirectiveDisablingMemoization(
fn.node.body.directives,
pass.opts,
);
if (useNoForget != null) {
pass.opts.logger?.logEvent(pass.filename, {
kind: 'CompileError',
fnLoc: fn.node.body.loc ?? null,
detail: {
severity: ErrorSeverity.Todo,
reason: 'Skipped due to "use no forget" directive.',
loc: useNoForget.loc ?? null,
suggestions: null,
},
});
return null;
}
// Otherwise opt-ins enable compilation regardless of mode
if (findDirectiveEnablingMemoization(fn.node.body.directives) != null) {
if (findDirectiveEnablingMemoization(fn.node.body.directives).length > 0)
return getComponentOrHookLike(fn, hookPattern) ?? 'Other';
}
Comment on lines -634 to -655
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic really shouldn't have been in this function which is responsible for categorization of the function (eg "Component" or "Hook". This has been moved into processFn instead.

}

// Component and hook declarations are known components/hooks
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@

## Input

```javascript
import {useRef} from 'react';

const useControllableState = options => {};
function NoopComponent() {}

function Component() {
'use no forget';
const ref = useRef(null);
// eslint-disable-next-line react-hooks/rules-of-hooks
ref.current = 'bad';
return <button ref={ref} />;
}

export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [],
};

```


## Error

```
7 | 'use no forget';
8 | const ref = useRef(null);
> 9 | // eslint-disable-next-line react-hooks/rules-of-hooks
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: React Compiler has skipped optimizing this component because one or more React ESLint rules were disabled. React Compiler only works when your components follow all the rules of React, disabling them may result in unexpected or incorrect behavior. eslint-disable-next-line react-hooks/rules-of-hooks (9:9)
10 | ref.current = 'bad';
11 | return <button ref={ref} />;
12 | }
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, shouldn't this not be reported since the component is opted out?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed



Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@

## Input

```javascript
import {useRef} from 'react';

function Component() {
'use no forget';
const ref = useRef(null);
// eslint-disable-next-line react-hooks/rules-of-hooks
ref.current = 'bad';
return <button ref={ref} />;
}

export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [],
};

```


## Error

```
4 | 'use no forget';
5 | const ref = useRef(null);
> 6 | // eslint-disable-next-line react-hooks/rules-of-hooks
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: React Compiler has skipped optimizing this component because one or more React ESLint rules were disabled. React Compiler only works when your components follow all the rules of React, disabling them may result in unexpected or incorrect behavior. eslint-disable-next-line react-hooks/rules-of-hooks (6:6)
7 | ref.current = 'bad';
8 | return <button ref={ref} />;
9 | }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

```


This file was deleted.

This file was deleted.

Loading