Skip to content

Commit

Permalink
[flow][dev-tools] Hermes-parser based getAST for error suppression
Browse files Browse the repository at this point in the history 
Summary:
This diff removes our dependence on `flow ast` command for error suppression. Previously, we rely on it to get the AST of the source code, so we can choose what kind of suppressions to add (e.g. we special case jsx elements since it's not always valid to add line comment above it).

`flow ast` doesn't read from flowconfig and the parser configuration can become out of sync with the rest of the setup. In this diff, I switched us to use hermes-parser instead. We can remove all of our dependency on flow-bin as a result.

There is an important change: we can just use normal js string indexing to get characters, rather than going through the Buffer, since hermes-parser, like most of the other js tools (including the js_of_ocaml compiled flow.js, but not flow-bin ...), encode the string as utf-16 (the js string encoding), so we don't need to worry about these longer-than-1-byte characters. flow-bin handles it differently, probably as a result of negligence, but js_of_ocaml happens to correct it, because there is no straightforward way to get a byte out of a string in the JS.

Changelog: [internal]

Reviewed By: pieterv

Differential Revision: D56796632

fbshipit-source-id: fbe1f923d3b37614e7fd5274a9401a7c4749cc77
  • Loading branch information
@SamChou19815 @facebook-github-bot
SamChou19815 authored and facebook-github-bot committed May 1, 2024
1 parent 8948694 commit f727f42
Show file tree
Hide file tree
Showing 6 changed files with 85 additions and 177 deletions.
113 changes: 35 additions & 78 deletions packages/flow-dev-tools/src/__tests__/all-tests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ const x = 4;
[],
['foo', 'bar'],
);
await expectUpdatedComments(testInput, testOutput, errorsByLine, flowBinPath);
await expectUpdatedComments(testInput, testOutput, errorsByLine);
});

test('updateSuppressionsInText does not remove eslint-disable multiline comment', async () => {
Expand All @@ -106,7 +106,7 @@ const x = 4;
[],
['foo', 'bar'],
);
await expectUpdatedComments(testInput, testOutput, errorsByLine, flowBinPath);
await expectUpdatedComments(testInput, testOutput, errorsByLine);
});

test('exec', async () => {
Expand All @@ -131,8 +131,6 @@ test('splitIntoChunks', () => {
expect(splitIntoChunks('✓✓✓✓✓', 1)).toEqual(['✓', '✓', '✓', '✓', '✓']);
});

const flowBinPath = path.resolve(process.env.FLOW_BIN);

test('updateSuppressionsInText removes unused comments', async () => {
const testInput = `
// $FlowFixMe
Expand All @@ -152,7 +150,7 @@ const x = 4;
[],
['foo', 'bar'],
);
await expectUpdatedComments(testInput, testOutput, errorsByLine, flowBinPath);
await expectUpdatedComments(testInput, testOutput, errorsByLine);
});

test('updateSuppressionsInText removes unused comments with sites', async () => {
Expand All @@ -174,7 +172,7 @@ const x = 4;
[],
['foo', 'bar'],
);
await expectUpdatedComments(testInput, testOutput, errorsByLine, flowBinPath);
await expectUpdatedComments(testInput, testOutput, errorsByLine);
});

test('updateSuppressionsInText can add site', async () => {
Expand All @@ -191,7 +189,7 @@ const x = 4;
`.trimLeft();

const errorsByLine = addErrorByLine(new Map(), testInput, loc, [], ['foo']);
await expectUpdatedComments(testInput, testOutput, errorsByLine, flowBinPath);
await expectUpdatedComments(testInput, testOutput, errorsByLine);
});

test('updateSuppressionsInText keeps unknown sites', async () => {
Expand All @@ -214,7 +212,7 @@ const x = 4;
[],
['foo', 'bar'],
);
await expectUpdatedComments(testInput, testOutput, errorsByLine, flowBinPath);
await expectUpdatedComments(testInput, testOutput, errorsByLine);
});

test('updateSuppressionsInText keeps unknown sites (2)', async () => {
Expand All @@ -237,7 +235,7 @@ const x = 4;
[],
['foo', 'bar'],
);
await expectUpdatedComments(testInput, testOutput, errorsByLine, flowBinPath);
await expectUpdatedComments(testInput, testOutput, errorsByLine);
});

test('updateSuppressionsInText adds site and keeps unknown sites', async () => {
Expand All @@ -254,7 +252,7 @@ const x = 4;
`.trimLeft();

const errorsByLine = addErrorByLine(new Map(), testInput, loc, [], ['foo']);
await expectUpdatedComments(testInput, testOutput, errorsByLine, flowBinPath);
await expectUpdatedComments(testInput, testOutput, errorsByLine);
});

test('updateSuppressionsInText handles JSX children', async () => {
Expand All @@ -276,13 +274,7 @@ test('updateSuppressionsInText handles JSX children', async () => {
`.trimLeft();

const errorsByLine = addErrorByLine(new Map(), testInput, loc, ['code'], []);
await expectUpdatedComments(
testInput,
testOutput,
errorsByLine,
flowBinPath,
[],
);
await expectUpdatedComments(testInput, testOutput, errorsByLine, []);
});

test('updateSuppressionsInText handles JSX children with multiline open tag', async () => {
Expand All @@ -306,13 +298,7 @@ test('updateSuppressionsInText handles JSX children with multiline open tag', as
`.trimLeft();

const errorsByLine = addErrorByLine(new Map(), testInput, loc, ['code'], []);
await expectUpdatedComments(
testInput,
testOutput,
errorsByLine,
flowBinPath,
[],
);
await expectUpdatedComments(testInput, testOutput, errorsByLine, []);
});

function addErrorByLine(errorsByLine, contents, loc, errorCodes, unusedRoots) {
Expand Down Expand Up @@ -352,9 +338,11 @@ function posToOffset(contents, line, col) {
// Using 1-indexed line and column for this
let currentLine = 1;
let currentCol = 1;
const buf = Buffer.from(contents, 'utf8');
while (offset < buf.length && !(currentLine === line && currentCol === col)) {
const char = buf.toString('utf8', offset, offset + 1);
while (
offset < contents.length &&
!(currentLine === line && currentCol === col)
) {
const char = contents[offset];
if (char === '\n') {
currentLine++;
currentCol = 1;
Expand All @@ -371,25 +359,20 @@ async function expectUpdatedComments(
input,
expectedOutput,
errorsByLine,
flowBinPath,
sites = ['foo', 'bar'],
) {
const actualOutput = await updateSuppressionsInText(
Buffer.from(input),
input,
new Set(sites),
1,
errorsByLine,
'',
flowBinPath,
);
expect(actualOutput.toString()).toEqual(expectedOutput);
expect(actualOutput).toEqual(expectedOutput);
}

test('addCommentsToCode > basic', async () => {
expect(await addCommentsToCode('foobar', null, '', [], flowBinPath)).toEqual([
'',
0,
]);
expect(await addCommentsToCode('foobar', null, '', [])).toEqual(['', 0]);
});

test('addCommentsToCode > advanced', async () => {
Expand All @@ -400,7 +383,6 @@ test('addCommentsToCode > advanced', async () => {
testInput,
/* Intentionally made these out of order to test that they are still inserted properly */
[1, 6, 5, 3].map(line => makeSuppression(line, testInput)),
flowBinPath,
),
).toEqual([testOutput, 8]);
});
Expand Down Expand Up @@ -453,7 +435,6 @@ function bar<a>(): b {}
error_codes: ['code4'],
},
],
flowBinPath,
),
).toEqual([
`// $FlowFixMe[code1]
Expand All @@ -471,23 +452,17 @@ function bar<a>(): b {}

test('addCommentsToCode > does not touch AST when no errors match the given code', async () => {
expect(
await addCommentsToCode(
'',
'code2',
`function foo() {}`,
[
{
loc: {
start: {line: 1, column: 13, offset: 13},
end: {line: 1, column: 14, offset: 14},
},
isError: true,
lints: new Set(),
error_codes: ['code1'],
await addCommentsToCode('', 'code2', `function foo() {}`, [
{
loc: {
start: {line: 1, column: 13, offset: 13},
end: {line: 1, column: 14, offset: 14},
},
],
flowBinPath,
),
isError: true,
lints: new Set(),
error_codes: ['code1'],
},
]),
).toEqual([`function foo() {}`, 0]);
});

Expand Down Expand Up @@ -519,7 +494,6 @@ function bar<a>(): b {}
error_codes: ['code2'],
},
],
flowBinPath,
),
).toEqual([
`function foo() {}
Expand Down Expand Up @@ -554,7 +528,6 @@ class A {
error_codes: ['code1'],
},
],
flowBinPath,
),
).toEqual([
`
Expand Down Expand Up @@ -590,7 +563,6 @@ class Foo {
error_codes: ['code1'],
},
],
flowBinPath,
),
).toEqual([
`
Expand Down Expand Up @@ -667,7 +639,6 @@ test('addCommentsToCode > JSX', async () => {
error_codes: ['code7'],
},
],
flowBinPath,
),
).toEqual([
`function A(): React.Node {
Expand Down Expand Up @@ -769,8 +740,6 @@ function locForLine(line, text) {
}

test('removeUnusedErrorSuppressionsFromText', async () => {
const flowBinPath = path.resolve(process.env.FLOW_BIN);

const testInput = ` // single line comment with some whitespace
const bar = 4;
/* multiline
Expand Down Expand Up @@ -805,12 +774,10 @@ const foo = 4;
[7, 4, 8, 20],
[],
].map(args => makeLoc(testInput, ...args));
await expectCommentsAreRemoved(testInput, testOutput, errorLocs, flowBinPath);
await expectCommentsAreRemoved(testInput, testOutput, errorLocs);
});

test('removeExtraSpaceWhenRemovingUnusedFlowLint', async () => {
const flowBinPath = path.resolve(process.env.FLOW_BIN);

const testInput = `// flowlint foo bar
//flowlint foo bar
// flowlint foo bar
Expand Down Expand Up @@ -844,12 +811,10 @@ test('removeExtraSpaceWhenRemovingUnusedFlowLint', async () => {
[8, 22, 8, 25],
[9, 27, 9, 30],
].map(args => makeLoc(testInput, ...args));
await expectCommentsAreRemoved(testInput, testOutput, errorLocs, flowBinPath);
await expectCommentsAreRemoved(testInput, testOutput, errorLocs);
});

test('deleteUnusedFlowLintComments', async () => {
const flowBinPath = path.resolve(process.env.FLOW_BIN);

const testInput = `// flowlint foo bar
//flowlint foo bar
// flowlint-next-line foo bar
Expand Down Expand Up @@ -917,12 +882,10 @@ let x =
[19, 15, 19, 18],
[21, 6, 21, 9],
].map(args => makeLoc(testInput, ...args));
await expectCommentsAreRemoved(testInput, testOutput, errorLocs, flowBinPath);
await expectCommentsAreRemoved(testInput, testOutput, errorLocs);
});

test('unicode', async () => {
const flowBinPath = path.resolve(process.env.FLOW_BIN);

const testInput = `const unicode = "\u{714E}\u{8336}";
const smiley = "\uD83D\uDE00";
Expand Down Expand Up @@ -992,21 +955,15 @@ let x =
[21, 17, 21, 20],
[22, 15, 22, 18],
].map(args => makeLoc(testInput, ...args));
await expectCommentsAreRemoved(testInput, testOutput, errorLocs, flowBinPath);
await expectCommentsAreRemoved(testInput, testOutput, errorLocs);
});

async function expectCommentsAreRemoved(
input,
expectedOutput,
errorLocs,
flowBinPath,
) {
async function expectCommentsAreRemoved(input, expectedOutput, errorLocs) {
const actualOutput = await removeUnusedErrorSuppressionsFromText(
Buffer.from(input),
input,
errorLocs,
flowBinPath,
);
expect(actualOutput.toString()).toEqual(expectedOutput);
expect(actualOutput).toEqual(expectedOutput);
}

(async () => {
Expand Down
12 changes: 2 additions & 10 deletions packages/flow-dev-tools/src/comment/add-commentsRunner.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ async function addCommentsToSource(
args.error_code,
codeString,
locs,
args.bin,
);
await writeFile(source, code);
return commentCount;
Expand All @@ -160,27 +159,20 @@ function addCommentsToCodeInternal(
path: Array<PathNode>,
) {
const [inside, ast] = getContext(loc, path);
return addCommentToText(
Buffer.from(code),
loc,
inside,
comments,
ast,
).toString();
return addCommentToText(code, loc, inside, comments, ast).toString();
}

async function addCommentsToCode(
comment: ?string,
error_code: ?string,
code: string,
locs: Array<Suppression>,
flowBinPath: string,
): Promise<
[string, number],
> /* [resulting code, number of comments inserted] */ {
locs.sort((l1, l2) => l2.loc.start.line - l1.loc.start.line);

const ast = await getAst(code, flowBinPath);
const ast = await getAst(code);

let commentCount = 0;
for (let {loc, isError, lints, error_codes} of locs) {
Expand Down
Loading

0 comments on commit f727f42

Please sign in to comment.