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

prefer-string-starts-ends-with: add suggestions for safely handling non-strings #1277

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 13 additions & 1 deletion docs/rules/prefer-string-starts-ends-with.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

Prefer [`String#startsWith()`](https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/String/startsWith) and [`String#endsWith()`](https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/String/endsWith) over using a regex with `/^foo/` or `/foo$/`.

This rule is fixable.
This rule is fixable, unless the matching object is known not a string.

## Fail

Expand All @@ -24,6 +24,18 @@ const foo = baz.startsWith('bar');
const foo = baz.endsWith('bar');
```

```js
const foo = baz?.startsWith('bar');
```

```js
const foo = (baz ?? '').startsWith('bar');
```

```js
const foo = String(baz).startsWith('bar');
```

```js
const foo = /^bar/i.test(baz);
```
141 changes: 106 additions & 35 deletions rules/prefer-string-starts-ends-with.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,28 @@ const getDocumentationUrl = require('./utils/get-documentation-url');
const methodSelector = require('./utils/method-selector');
const quoteString = require('./utils/quote-string');
const shouldAddParenthesesToMemberExpressionObject = require('./utils/should-add-parentheses-to-member-expression-object');
const shouldAddParenthesesToLogicalExpressionChild = require('./utils/should-add-parentheses-to-logical-expression-child');
const {getParenthesizedText, getParenthesizedRange} = require('./utils/parentheses');

const MESSAGE_STARTS_WITH = 'prefer-starts-with';
const MESSAGE_ENDS_WITH = 'prefer-ends-with';
const FIX_TYPE_STRING_CASTING = 'useStringCasting';
const FIX_TYPE_OPTIONAL_CHAINING = 'useOptionalChaining';
const FIX_TYPE_NULLISH_COALESCING = 'useNullishCoalescing';
const messages = {
[MESSAGE_STARTS_WITH]: 'Prefer `String#startsWith()` over a regex with `^`.',
[MESSAGE_ENDS_WITH]: 'Prefer `String#endsWith()` over a regex with `$`.'
[MESSAGE_ENDS_WITH]: 'Prefer `String#endsWith()` over a regex with `$`.',
[FIX_TYPE_STRING_CASTING]: 'Convert to string `String(…).{{method}}()`.',
[FIX_TYPE_OPTIONAL_CHAINING]: 'Use optional chaining `…?.{{method}}()`.',
[FIX_TYPE_NULLISH_COALESCING]: 'Use nullish coalescing `(… ?? \'\').{{method}}()`.'
};

const doesNotContain = (string, characters) => characters.every(character => !string.includes(character));

const isSimpleString = string => doesNotContain(
string,
['^', '$', '+', '[', '{', '(', '\\', '.', '?', '*', '|']
);
const addParentheses = text => `(${text})`;

const regexTestSelector = [
methodSelector({name: 'test', length: 1}),
Expand Down Expand Up @@ -64,40 +72,102 @@ const create = context => {
return;
}

context.report({
const [target] = node.arguments;
const method = result.messageId === MESSAGE_STARTS_WITH ? 'startsWith' : 'endsWith';

let isString = target.type === 'TemplateLiteral' ||
(
target.type === 'CallExpression' &&
target.callee.type === 'Identifier' &&
target.callee.name === 'String'
);
let isNonString = false;
if (!isString) {
const staticValue = getStaticValue(target, context.getScope());

if (staticValue) {
isString = typeof staticValue.value === 'string';
isNonString = !isString;
}
}

const problem = {
node,
messageId: result.messageId,
fix: fixer => {
const [target] = node.arguments;
const staticValue = getStaticValue(target, context.getScope());
if (staticValue && typeof staticValue.value !== 'string') {
// Ignore known, non-string value which won't have a startsWith/endsWith function.
return;
}

const method = result.messageId === MESSAGE_STARTS_WITH ? 'startsWith' : 'endsWith';
let targetString = sourceCode.getText(target);

if (
// If regex is parenthesized, we can use it, so we don't need add again
!isParenthesized(regexNode, sourceCode) &&
(isParenthesized(target, sourceCode) || shouldAddParenthesesToMemberExpressionObject(target, sourceCode))
) {
targetString = `(${targetString})`;
}

// The regex literal always starts with `/` or `(`, so we don't need check ASI

return [
// Replace regex with string
fixer.replaceText(regexNode, targetString),
// `.test` => `.startsWith` / `.endsWith`
fixer.replaceText(node.callee.property, method),
// Replace argument with result.string
fixer.replaceText(target, quoteString(result.string))
];
messageId: result.messageId
};

function * fix(fixer, fixType) {
let targetText = getParenthesizedText(target, sourceCode);
const isRegexParenthesized = isParenthesized(regexNode, sourceCode);
const isTargetParenthesized = isParenthesized(target, sourceCode);

switch (fixType) {
// Goal: `(target ?? '').startsWith(pattern)`
case FIX_TYPE_NULLISH_COALESCING:
if (
!isTargetParenthesized &&
shouldAddParenthesesToLogicalExpressionChild(target, {operator: '??', property: 'left'})
) {
targetText = addParentheses(targetText);
}

targetText += ' ?? \'\'';

// `LogicalExpression` need add parentheses to call `.startsWith()`,
// but if regex is parenthesized, we can reuse it
if (!isRegexParenthesized) {
targetText = addParentheses(targetText);
}

break;

// Goal: `String(target).startsWith(pattern)`
case FIX_TYPE_STRING_CASTING:
// `target` was a call argument, don't need check parentheses
targetText = `String(${targetText})`;
Copy link
Sponsor Contributor Author

@bmish bmish May 18, 2021

Choose a reason for hiding this comment

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

This adds extra parenthesis that aren't needed sometimes. I think I had code to handle this before.

in: (/^b/).test((a))
out: ((a)).startsWith('b')

Not a big deal though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that, but better "add missing parens" instead of "remove extra parens".

// `CallExpression` don't need add parentheses to call `.startsWith()`
break;

// Goal: `target.startsWith(pattern)` or `target?.startsWith(pattern)`
case FIX_TYPE_OPTIONAL_CHAINING:
// Optional chaining: `target.startsWith` => `target?.startsWith`
yield fixer.replaceText(sourceCode.getTokenBefore(node.callee.property), '?.');
// Fallthrough
default:
if (
!isRegexParenthesized &&
!isTargetParenthesized &&
shouldAddParenthesesToMemberExpressionObject(target, sourceCode)
) {
targetText = addParentheses(targetText);
}
}
});

// The regex literal always starts with `/` or `(`, so we don't need check ASI

// Replace regex with string
yield fixer.replaceText(regexNode, targetText);

// `.test` => `.startsWith` / `.endsWith`
yield fixer.replaceText(node.callee.property, method);

// Replace argument with result.string
yield fixer.replaceTextRange(getParenthesizedRange(target, sourceCode), quoteString(result.string));
}

if (isString || !isNonString) {
problem.fix = fix;
}

if (!isString) {
problem.suggest = [
FIX_TYPE_STRING_CASTING,
FIX_TYPE_OPTIONAL_CHAINING,
FIX_TYPE_NULLISH_COALESCING
].map(type => ({messageId: type, data: {method}, fix: fixer => fix(fixer, type)}));
}

context.report(problem);
}
};
};
Expand All @@ -108,7 +178,8 @@ module.exports = {
type: 'suggestion',
docs: {
description: 'Prefer `String#startsWith()` & `String#endsWith()` over `RegExp#test()`.',
url: getDocumentationUrl(__filename)
url: getDocumentationUrl(__filename),
suggest: true
},
fixable: 'code',
schema: [],
Expand Down
38 changes: 38 additions & 0 deletions rules/utils/should-add-parentheses-to-logical-expression-child.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
'use strict';

/**
Check if parentheses should be added to a `node` when it's used as child of `LogicalExpression`.
@param {Node} node - The AST node to check.
@param {{operator: string, property: string}} options - Options
@returns {boolean}
*/
function shouldAddParenthesesToLogicalExpressionChild(node, {operator, property}) {
/* istanbul ignore next: When operator or property is different, need check `LogicalExpression` operator precedence, not implemented */
if (operator !== '??' || property !== 'left') {
throw new Error('Not supported.');
}

// Not really needed, but more readable
if (
node.type === 'AwaitExpression' ||
node.type === 'BinaryExpression'
) {
return true;
}

// Lower precedence than `LogicalExpression`
// see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence#Table
if (
node.type === 'ConditionalExpression' ||
node.type === 'AssignmentExpression' ||
node.type === 'AssignmentExpression' ||
node.type === 'YieldExpression' ||
node.type === 'SequenceExpression'
) {
return true;
}

return false;
}

module.exports = shouldAddParenthesesToLogicalExpressionChild;