Skip to content

Commit

Permalink
[flow-dev-tools] Do not remove suppressions that include "eslint-disa…
Browse files Browse the repository at this point in the history
…ble"

Summary:
While not common it is possible that a single comment is used to suppress both a Flow error and an eslint issue. E.g.
```
// $FlowFixMe eslint-disable-next-line no-fallthrough
```
This diff prevents `flow update-suppressions` from removing a Flow suppression comment if in included in the text associated with it is the string `eslint-disable`.

Changelog: [internal]

Reviewed By: SamChou19815

Differential Revision: D55656739

fbshipit-source-id: 9768276f4df1895377347883dfef9a976f172815
  • Loading branch information
panagosg7 authored and facebook-github-bot committed Apr 3, 2024
1 parent fd06398 commit 2856f03
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 0 deletions.
50 changes: 50 additions & 0 deletions packages/flow-dev-tools/src/__tests__/all-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,56 @@ function test(name, fn) {
collectedTests.push({name, fn});
}

test('updateSuppressionsInText does not remove eslint-disable comment', async () => {
const testInput = `
// $FlowFixMe eslint-disable-next-line no-fallthrough
const x = 4;
`.trimLeft();

const loc = makeLoc(testInput, 1, 1, 1, 54);

const testOutput = `
// $FlowFixMe eslint-disable-next-line no-fallthrough
const x = 4;
`.trimLeft();

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

test('updateSuppressionsInText does not remove eslint-disable multiline comment', async () => {
const testInput = `
/* $FlowFixMe
* eslint-disable no-fallthrough
*/
const x = 4;
`.trimLeft();

const loc = makeLoc(testInput, 1, 1, 3, 4);

const testOutput = `
/* $FlowFixMe
* eslint-disable no-fallthrough
*/
const x = 4;
`.trimLeft();

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

test('exec', async () => {
expect(await exec('echo foo')).toBe('foo\n');
expect(await exec('cat', {stdin: 'bar'})).toBe('bar');
Expand Down
6 changes: 6 additions & 0 deletions packages/flow-dev-tools/src/comment/commentMutator.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ function isLintSuppression(commentAST: {value: string}): boolean {
return flowlintRegex.test(commentAST.value);
}

const eslintRegex = /eslint-disable/m;
function isEsLintSuppression(value: string): boolean {
return eslintRegex.test(value);
}

const newlineRegex = /[\u2029\u2028\r\n]/;
const edible = /[\t ]/;
/* This is the most confusing part of this command. A simple version of this
Expand Down Expand Up @@ -428,6 +433,7 @@ function formatComment(

module.exports = {
isLintSuppression,
isEsLintSuppression,
findStartOfLine,
insertCommentToText,
addCommentToText,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const {readFile, writeFile} = require('fs').promises;
const {
removeUnusedErrorSuppressionFromText,
isLintSuppression,
isEsLintSuppression,
addCommentToText,
findStartOfLine,
} = require('../comment/commentMutator');
Expand Down Expand Up @@ -219,6 +220,12 @@ function updateErrorSuppression(
let innerOffset = commentStartOffset + 2; // `/*` and `//` are both 2 chars
let text = contents.slice(innerOffset, endOffset).toString('utf8');

// do not remove the comment if it's a eslint suppression
// TODO update the logging provided in the end of the command
if (isEsLintSuppression(text)) {
return contents;
}

const existingRoots = getSites(text); // may include unknown roots
const roots = new Set([...existingRoots, ...knownRoots]);
unusedRoots.forEach(root => roots.delete(root));
Expand Down

0 comments on commit 2856f03

Please sign in to comment.