Skip to content

Commit

Permalink
fix(eslint-plugin): Cast a wider net in api-no-slashes.js (#8649)
Browse files Browse the repository at this point in the history 
* fix(eslint-plugin): Cast a wider net in api-no-slashes.js

* refactor(eslint-plugin): Refactored api-no-slashes to improve readability

... I hope

* fix(eslint-plugin): in api-no-slashes, always return null sourceCode when a null node is passed

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
  • Loading branch information
@christopherthielen @mergify
christopherthielen and mergify[bot] committed Oct 14, 2020
1 parent ae9292e commit cf5ab83
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 37 deletions.
83 changes: 54 additions & 29 deletions packages/eslint-plugin/rules/api-no-slashes.js
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';
const { getCallingIdentifier, getVariableInScope } = require('../utils/utils');
const _ = require('lodash');
const { get } = require('lodash');

/**
* No slashes in string literals passed to API.one() / API.all()
Expand All @@ -20,33 +20,54 @@ const rule = function (context) {
return;
}

// API.one('foo/bad')
// ^^^^^^^
// API.all('ok').one('ok', 'foo/bad', 'ok')
// ^^^^^^^
const literalArgs = args.filter((arg) => arg.type === 'Literal' && arg.raw.includes('/'));
// ^^^
if ((getCallingIdentifier(node) || {}).name !== 'API') {
// console.log(getCallingIdentifier(callee));
// console.log('calling identifier not API');
return;
}

// var badvar = 'foo/bad'; API.one(badvar);
// ^^^^^^
const variableArgs = args.filter((argument) => {
if (argument.type !== 'Identifier') {
// Get the source code (think .toString()) of the AST node and find a slash
// This isn't 100% accurate, but it's good enough.
function sourceCodeHasSlash(node) {
const text = node ? context.getSourceCode().getText(node) : '';
return !!(text && text.includes('/'));
}

function isArgLiteralWithSlash(arg) {
return arg.type === 'Literal' && sourceCodeHasSlash(arg);
}

const isVariableInitializedWithSlash = (arg) => {
if (arg.type !== 'Identifier') {
return false;
}
const variable = getVariableInScope(context, argument);
const literalValue = _.get(variable, 'defs[0].node.init.raw', '');
return literalValue.includes('/');
});

if (literalArgs.length === 0 && variableArgs.length === 0) {
// console.log('no slashes');
return;
}
// Check if the arg is a variable
const variable = getVariableInScope(context, arg);
// Find the variable's initializer
const initializer = get(variable, 'defs[0].node.init', null);
// Check if that variable's initializer contains a slash
return sourceCodeHasSlash(initializer);
};

// API.all('ok').one('ok', 'foo/bad', 'ok')
// ^^^
if ((getCallingIdentifier(node) || {}).name !== 'API') {
// console.log(getCallingIdentifier(callee));
// console.log('calling identifier not API');
// Literal:
// .one(okArg, 'foo/' + barid)
// .one(okArg, `foo/${barid}`)
const literalsWithSlashes = args.filter((arg) => isArgLiteralWithSlash(arg));

// Initializer:
// var badArg = `foo/${barid}`;
// var badArg2 = 'foo/' + barid`;
// .one(badArg)
// .one(badArg2)
const varsWithSlashes = args.filter((arg) => isVariableInitializedWithSlash(arg));

// Detected an arg with a slash, but can't auto-fix
const argsWithSlashes = args.filter((arg) => !literalsWithSlashes.includes(arg) && sourceCodeHasSlash(arg));

if (literalsWithSlashes.length === 0 && varsWithSlashes.length === 0 && argsWithSlashes.length === 0) {
return;
}

Expand All @@ -61,29 +82,33 @@ const rule = function (context) {
// 'foo/bad'
// with:
// 'foo', 'bad'
const literalArgFixes = literalArgs.map((arg) => {
const literalArgFixes = literalsWithSlashes.map((arg) => {
const varArgs = arg.value
.split('/')
.map((segment) => "'" + segment + "'")
.join(', ');

return fixer.replaceText(arg, varArgs);
});

// within:
// let varDeclaration = 'foo/bad';
// API.one(varDeclaration)
// let myVar = 'foo/bad';
// API.one(myVar)
// replaces argument:
// varDeclaration
// myVar
// with:
// ...varDeclaration.split('/')
const variableArgFixes = variableArgs.map((arg) => {
// ...myVar.split('/')
// i.e.:
// API.one(...myVar.split('/'))
const variableArgFixes = varsWithSlashes.map((arg) => {
// Found a variable with an initializer containing a slash
// Change the argument to be a string-split + array-spread
const spread = `...${arg.name}.split('/')`;
return fixer.replaceText(arg, spread);
});

return literalArgFixes.concat(variableArgFixes);
};

context.report({ fix, node, message });
},
};
Expand Down
42 changes: 36 additions & 6 deletions packages/eslint-plugin/test/api-no-slashes.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,30 +16,60 @@ ruleTester.run('api-no-slashes', rule, {
invalid: [
{
code: `API.one('foo/bad');`,
errors: [errorMessage],
output: `API.one('foo', 'bad');`,
errors: [errorMessage],
},
// Fixes slashes in chained .one().all().one() calls
{
code: `API.one('ok').one('ok').all('ok').one('foo/bad');`,
errors: [errorMessage],
output: `API.one('ok').one('ok').all('ok').one('foo', 'bad');`,
errors: [errorMessage],
},
// Fixes slashes in varargs arguments
{
code: `API.one('ok').one('ok', 'foo/bad');`,
output: `API.one('ok').one('ok', 'foo', 'bad');`,
errors: [errorMessage],
},
// Variables which are initialized to a string literal with a slash
// Variables which are initialized to a string literal with a slash transform to split+spread
{
code: `let foo = "foo/bad"; API.one(foo);`,
errors: [errorMessage],
output: `let foo = "foo/bad"; API.one(...foo.split('/'));`,
errors: [errorMessage],
},
// Mix of everything
// Variables whose initializer contains a slash
{
code: 'let foo = `foo/${variable}`; API.one(foo);',
output: "let foo = `foo/${variable}`; API.one(...foo.split('/'));",
errors: [errorMessage],
},
// Variables whose initializer contains a slash (anywhere)
{
code: 'let foo = "foo/bad" + variable; API.one(foo);',
output: `let foo = "foo/bad" + variable; API.one(...foo.split('/'));`,
errors: [errorMessage],
},
// Variables whose initializer contains a slash (mix of everything, still can be detected)
{
code: 'let foo = variable + `foo/${expr}` + "/bad/" + variable; API.one(foo);',
output: 'let foo = variable + `foo/${expr}` + "/bad/" + variable; API.one(...foo.split(\'/\'));',
errors: [errorMessage],
},
// Multiple errors, mix of styles
{
code: `const foo = "foo/bad"; API.all('api').one(foo).one('ok', 'bar/baz');`,
errors: [errorMessage, errorMessage], // two errors
output: `const foo = "foo/bad"; API.all('api').one(...foo.split('/')).one('ok', 'bar', 'baz');`,
errors: [errorMessage, errorMessage], // two errors
},
// Detectable but unfixable
{
code: 'API.one(`foo/${cantfix}`);',
errors: [errorMessage], // two errors
},
// Detectable but unfixable
{
code: 'API.one("foo/" + cantfix);',
errors: [errorMessage], // two errors
},
],
});
6 changes: 4 additions & 2 deletions packages/eslint-plugin/utils/utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@ function getCallingIdentifier(calleeObject) {

/** given an identifier, finds its Variable in the enclosing scope */
function getVariableInScope(context, identifier) {
const { variables } = context.getScope();
return variables.find((v) => v.name === identifier.name);
if (identifier.type === 'Identifier') {
const { variables } = context.getScope();
return variables.find((v) => v.name === identifier.name);
}
}

function getProgram(node) {
Expand Down

0 comments on commit cf5ab83

Please sign in to comment.