Skip to content

Commit f5a5788

Browse files
committed
[compiler] source location validator (facebook#35109)
@josephsavona this was briefly discussed in an old thread, lmk your thoughts on the approach. I have some fixes ready as well but wanted to get this test case in first... there's some things I don't _love_ about this approach, but end of the day it's just a tool for the test suite rather than something for end user folks so even if it does a 70% good enough job that's fine. ### refresher on the problem when we generate coverage reports with jest (istanbul), our coverage ends up completely out of whack due to the AST missing a ton of (let's call them "important") source locations after the compiler pipeline has run. At the moment to get around this, we've been doing something a bit unorthodox and also running our test suite with istanbul running before the compiler -- which results in its own set of issues (for eg, things being memoized differently, or the compiler completely bailing out on the instrumented code, etc). before getting in fixes, I wanted to set up a test case to start chipping away on as you had recommended. ### how it works The validator basically: 1. Traverses the original AST and collects the source locations for some "important" node types - (excludes useMemo/useCallback calls, as those are stripped out by the compiler) 3. Traverses the generated AST and looks for nodes with matching source locations. 4. Generates errors for source locations missing nodes in the generated AST ### caveats/drawbacks There are some things that don't work super well with this approach. A more natural test fit I think would be just having some explicit assertions made against an AST in a test file, as you can just bake all of the assumptions/nuance in there that are difficult to handle in a generic manner. However, this is maybe "good enough" for now. 1. Have to be careful what you put into the test fixture. If you put in some code that the compiler just removes (for eg, a variable assignment that is unused), you're creating a failure case that's impossible to fix. I added a skip for useMemo/useCallback. 2. "Important" locations must exactly match for validation to pass. - Might get tricky making sure things are mapped correctly when a node type is completely changed, for eg, when a block statement arrow function body gets turned into an implicit return via the body just being an expression/identifier. - This can/could result in scenarios where more changes are needed to shuttle the locations through due to HIR not having a 1:1 mapping all the babel nuances, even if some combination of other data might be good enough even if not 10000% accurate. This might be the _right_ thing anyways so we don't end up with edge cases having incorrect source locations. DiffTrain build for [3a495ae](facebook@3a495ae)
1 parent c8afb3a commit f5a5788

35 files changed

+200
-86
lines changed

compiled/eslint-plugin-react-hooks/index.js

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32166,6 +32166,7 @@ const EnvironmentConfigSchema = v4.z.object({
3216632166
validateMemoizedEffectDependencies: v4.z.boolean().default(false),
3216732167
validateNoCapitalizedCalls: v4.z.nullable(v4.z.array(v4.z.string())).default(null),
3216832168
validateBlocklistedImports: v4.z.nullable(v4.z.array(v4.z.string())).default(null),
32169+
validateSourceLocations: v4.z.boolean().default(false),
3216932170
validateNoImpureFunctionsInRender: v4.z.boolean().default(false),
3217032171
validateNoFreezingKnownMutableFunctions: v4.z.boolean().default(false),
3217132172
enableAssumeHooksFollowRulesOfReact: v4.z.boolean().default(true),
@@ -50318,6 +50319,116 @@ function isUnmemoized(operand, scopes) {
5031850319
return operand.scope != null && !scopes.has(operand.scope.id);
5031950320
}
5032050321

50322+
const IMPORTANT_INSTRUMENTED_TYPES = new Set([
50323+
'ArrowFunctionExpression',
50324+
'AssignmentPattern',
50325+
'ObjectMethod',
50326+
'ExpressionStatement',
50327+
'BreakStatement',
50328+
'ContinueStatement',
50329+
'ReturnStatement',
50330+
'ThrowStatement',
50331+
'TryStatement',
50332+
'VariableDeclarator',
50333+
'IfStatement',
50334+
'ForStatement',
50335+
'ForInStatement',
50336+
'ForOfStatement',
50337+
'WhileStatement',
50338+
'DoWhileStatement',
50339+
'SwitchStatement',
50340+
'SwitchCase',
50341+
'WithStatement',
50342+
'FunctionDeclaration',
50343+
'FunctionExpression',
50344+
'LabeledStatement',
50345+
'ConditionalExpression',
50346+
'LogicalExpression',
50347+
]);
50348+
function isManualMemoization(node) {
50349+
if (libExports$1.isCallExpression(node)) {
50350+
const callee = node.callee;
50351+
if (libExports$1.isIdentifier(callee)) {
50352+
return callee.name === 'useMemo' || callee.name === 'useCallback';
50353+
}
50354+
if (libExports$1.isMemberExpression(callee) &&
50355+
libExports$1.isIdentifier(callee.property) &&
50356+
libExports$1.isIdentifier(callee.object)) {
50357+
return (callee.object.name === 'React' &&
50358+
(callee.property.name === 'useMemo' ||
50359+
callee.property.name === 'useCallback'));
50360+
}
50361+
}
50362+
return false;
50363+
}
50364+
function locationKey(loc) {
50365+
return `${loc.start.line}:${loc.start.column}-${loc.end.line}:${loc.end.column}`;
50366+
}
50367+
function validateSourceLocations(func, generatedAst) {
50368+
const errors = new CompilerError();
50369+
const importantOriginalLocations = new Map();
50370+
func.traverse({
50371+
enter(path) {
50372+
const node = path.node;
50373+
if (!IMPORTANT_INSTRUMENTED_TYPES.has(node.type)) {
50374+
return;
50375+
}
50376+
if (isManualMemoization(node)) {
50377+
return;
50378+
}
50379+
if (node.loc) {
50380+
const key = locationKey(node.loc);
50381+
importantOriginalLocations.set(key, {
50382+
loc: node.loc,
50383+
nodeType: node.type,
50384+
});
50385+
}
50386+
},
50387+
});
50388+
const generatedLocations = new Set();
50389+
function collectGeneratedLocations(node) {
50390+
if (node.loc) {
50391+
generatedLocations.add(locationKey(node.loc));
50392+
}
50393+
const keys = libExports$1.VISITOR_KEYS[node.type];
50394+
if (!keys) {
50395+
return;
50396+
}
50397+
for (const key of keys) {
50398+
const value = node[key];
50399+
if (Array.isArray(value)) {
50400+
for (const item of value) {
50401+
if (libExports$1.isNode(item)) {
50402+
collectGeneratedLocations(item);
50403+
}
50404+
}
50405+
}
50406+
else if (libExports$1.isNode(value)) {
50407+
collectGeneratedLocations(value);
50408+
}
50409+
}
50410+
}
50411+
collectGeneratedLocations(generatedAst.body);
50412+
for (const outlined of generatedAst.outlined) {
50413+
collectGeneratedLocations(outlined.fn.body);
50414+
}
50415+
for (const [key, { loc, nodeType }] of importantOriginalLocations) {
50416+
if (!generatedLocations.has(key)) {
50417+
errors.pushDiagnostic(CompilerDiagnostic.create({
50418+
category: ErrorCategory.Todo,
50419+
reason: 'Important source location missing in generated code',
50420+
description: `Source location for ${nodeType} is missing in the generated output. This can cause coverage instrumentation ` +
50421+
`to fail to track this code properly, resulting in inaccurate coverage reports.`,
50422+
}).withDetails({
50423+
kind: 'error',
50424+
loc,
50425+
message: null,
50426+
}));
50427+
}
50428+
}
50429+
return errors.asResult();
50430+
}
50431+
5032150432
function validateUseMemo(fn) {
5032250433
const errors = new CompilerError();
5032350434
const voidMemoErrors = new CompilerError();
@@ -53201,6 +53312,9 @@ function runWithEnvironment(func, env) {
5320153312
for (const outlined of ast.outlined) {
5320253313
log({ kind: 'ast', name: 'Codegen (outlined)', value: outlined.fn });
5320353314
}
53315+
if (env.config.validateSourceLocations) {
53316+
validateSourceLocations(func, ast).unwrap();
53317+
}
5320453318
if (env.config.throwUnknownException__testonly) {
5320553319
throw new Error('unexpected error');
5320653320
}

compiled/facebook-www/REVISION

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
bbe3f4d322846083c57009dbd6121900f120b389
1+
3a495ae72264c46b4a4355904c6b4958b0a2f9b2
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
bbe3f4d322846083c57009dbd6121900f120b389
1+
3a495ae72264c46b4a4355904c6b4958b0a2f9b2

compiled/facebook-www/React-dev.classic.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1499,7 +1499,7 @@ __DEV__ &&
14991499
exports.useTransition = function () {
15001500
return resolveDispatcher().useTransition();
15011501
};
1502-
exports.version = "19.3.0-www-classic-bbe3f4d3-20251112";
1502+
exports.version = "19.3.0-www-classic-3a495ae7-20251112";
15031503
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
15041504
"function" ===
15051505
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

compiled/facebook-www/React-dev.modern.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1499,7 +1499,7 @@ __DEV__ &&
14991499
exports.useTransition = function () {
15001500
return resolveDispatcher().useTransition();
15011501
};
1502-
exports.version = "19.3.0-www-modern-bbe3f4d3-20251112";
1502+
exports.version = "19.3.0-www-modern-3a495ae7-20251112";
15031503
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
15041504
"function" ===
15051505
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

compiled/facebook-www/React-prod.classic.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -606,4 +606,4 @@ exports.useSyncExternalStore = function (
606606
exports.useTransition = function () {
607607
return ReactSharedInternals.H.useTransition();
608608
};
609-
exports.version = "19.3.0-www-classic-bbe3f4d3-20251112";
609+
exports.version = "19.3.0-www-classic-3a495ae7-20251112";

compiled/facebook-www/React-prod.modern.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -606,4 +606,4 @@ exports.useSyncExternalStore = function (
606606
exports.useTransition = function () {
607607
return ReactSharedInternals.H.useTransition();
608608
};
609-
exports.version = "19.3.0-www-modern-bbe3f4d3-20251112";
609+
exports.version = "19.3.0-www-modern-3a495ae7-20251112";

compiled/facebook-www/React-profiling.classic.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -610,7 +610,7 @@ exports.useSyncExternalStore = function (
610610
exports.useTransition = function () {
611611
return ReactSharedInternals.H.useTransition();
612612
};
613-
exports.version = "19.3.0-www-classic-bbe3f4d3-20251112";
613+
exports.version = "19.3.0-www-classic-3a495ae7-20251112";
614614
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
615615
"function" ===
616616
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

compiled/facebook-www/React-profiling.modern.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -610,7 +610,7 @@ exports.useSyncExternalStore = function (
610610
exports.useTransition = function () {
611611
return ReactSharedInternals.H.useTransition();
612612
};
613-
exports.version = "19.3.0-www-modern-bbe3f4d3-20251112";
613+
exports.version = "19.3.0-www-modern-3a495ae7-20251112";
614614
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
615615
"function" ===
616616
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

compiled/facebook-www/ReactART-dev.classic.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20466,10 +20466,10 @@ __DEV__ &&
2046620466
(function () {
2046720467
var internals = {
2046820468
bundleType: 1,
20469-
version: "19.3.0-www-classic-bbe3f4d3-20251112",
20469+
version: "19.3.0-www-classic-3a495ae7-20251112",
2047020470
rendererPackageName: "react-art",
2047120471
currentDispatcherRef: ReactSharedInternals,
20472-
reconcilerVersion: "19.3.0-www-classic-bbe3f4d3-20251112"
20472+
reconcilerVersion: "19.3.0-www-classic-3a495ae7-20251112"
2047320473
};
2047420474
internals.overrideHookState = overrideHookState;
2047520475
internals.overrideHookStateDeletePath = overrideHookStateDeletePath;
@@ -20504,7 +20504,7 @@ __DEV__ &&
2050420504
exports.Shape = Shape;
2050520505
exports.Surface = Surface;
2050620506
exports.Text = Text;
20507-
exports.version = "19.3.0-www-classic-bbe3f4d3-20251112";
20507+
exports.version = "19.3.0-www-classic-3a495ae7-20251112";
2050820508
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
2050920509
"function" ===
2051020510
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

0 commit comments

Comments
 (0)