Skip to content

Commit

Permalink
Fix a bug in autofixer and autofix additional cases with `firstObject…
Browse files Browse the repository at this point in the history 
… and `lastObject` in `no-get` rule (#1841)
  • Loading branch information
@ArtixZ
ArtixZ authored Apr 22, 2023
1 parent 0b314ce commit 7e82eb1
Show file tree
Hide file tree
Showing 2 changed files with 235 additions and 9 deletions.
41 changes: 33 additions & 8 deletions lib/rules/no-get.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,13 @@ function fixGet({
isInLeftSideOfAssignmentExpression,
objectText,
}) {
if (path.includes('.') && !useOptionalChaining && !isInLeftSideOfAssignmentExpression) {
const getResultIsChained = node.parent.type === 'MemberExpression' && node.parent.object === node;

// If the result of get is chained, we can safely autofix nests paths without using optional chaining.
// In the left side of an assignment, we can safely autofix nested paths without using optional chaining.
const shouldIgnoreOptionalChaining = getResultIsChained || isInLeftSideOfAssignmentExpression;

if (path.includes('.') && !useOptionalChaining && !shouldIgnoreOptionalChaining) {
// Not safe to autofix nested properties because some properties in the path might be null or undefined.
return null;
}
Expand All @@ -59,19 +65,38 @@ function fixGet({
return null;
}

const getResultIsChained = node.parent.type === 'MemberExpression' && node.parent.object === node;
if (path.match(/lastObject/g)?.length > 1) {
// Do not autofix when multiple `lastObject` are chained.
return null;
}

// If the result of get is chained, we can safely autofix nests paths without using optional chaining.
// In the left side of an assignment, we can safely autofix nested paths without using optional chaining.
let replacementPath =
getResultIsChained || isInLeftSideOfAssignmentExpression ? path : path.replace(/\./g, '?.');
let replacementPath = shouldIgnoreOptionalChaining ? path : path.replace(/\./g, '?.');

// Replace any array element access (foo.1 => foo[1] or foo?.[1]).
replacementPath = replacementPath
.replace(/\.(\d+)/g, isInLeftSideOfAssignmentExpression ? '[$1]' : '.[$1]') // Usages in middle of path.
.replace(/^(\d+)\??\./, isInLeftSideOfAssignmentExpression ? '[$1].' : '[$1]?.') // Usage at beginning of path.
.replace(/\.(\d+)/g, shouldIgnoreOptionalChaining ? '[$1]' : '.[$1]') // Usages in middle of path.
.replace(/^(\d+)\??\./, shouldIgnoreOptionalChaining ? '[$1].' : '[$1]?.') // Usage at beginning of path.
.replace(/^(\d+)$/, '[$1]'); // Usage as entire string.

// Replace any array element access using `firstObject` and `lastObject` (foo.firstObject => foo[0] or foo?.[0]).
replacementPath = replacementPath
.replace(/\.firstObject/g, shouldIgnoreOptionalChaining ? '[0]' : '.[0]') // When `firstObject` is used in the middle of the path. e.g. foo.firstObject
.replace(/^firstObject\??\./, shouldIgnoreOptionalChaining ? '[0].' : '[0]?.') // When `firstObject` is used at the beginning of the path. e.g. firstObject.bar
.replace(/^firstObject$/, '[0]') // When `firstObject` is used as the entire path.
.replace(
/\??\.lastObject/, // When `lastObject` is used in the middle of the path. e.g. foo.lastObject
(_, offset) =>
`${shouldIgnoreOptionalChaining ? '' : '?.'}[${objectText}.${replacementPath.slice(
0,
offset
)}.length - 1]`
)
.replace(
/^lastObject\??\./, // When `lastObject` is used at the beginning of the path. e.g. lastObject.bar
`[${objectText}.length - 1]${shouldIgnoreOptionalChaining ? '.' : '?.'}`
)
.replace(/^lastObject$/, `[${objectText}.length - 1]`); // When `lastObject` is used as the entire path.

// Add parenthesis around the object text in case of something like this: get(foo || {}, 'bar')
const objectTextSafe = isValidJSPath(objectText) ? objectText : `(${objectText})`;

Expand Down
203 changes: 202 additions & 1 deletion tests/lib/rules/no-get.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,8 +226,9 @@ ruleTester.run('no-get', rule, {
},
{
// useOptionalChaining = false
// We can safely autofix nested paths when the result of get() is chained,
code: "foo1.foo2.get('bar.bar').baz;",
output: null,
output: 'foo1.foo2.bar.bar.baz;',
options: [{ catchUnsafeObjects: true, useOptionalChaining: false }],
errors: [{ message: ERROR_MESSAGE_GET, type: 'CallExpression' }],
},
Expand Down Expand Up @@ -618,6 +619,18 @@ ruleTester.run('no-get', rule, {
},
],
},
{
// We can safely autofix nested paths when the result of get() is chained,
code: "this.get('foo.0.bar')[123];",
output: 'this.foo[0].bar[123];',
options: [{ useOptionalChaining: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
type: 'CallExpression',
},
],
},
{
// Handle array element access (left side of an assignment, entire string).
code: "this.get('0')[123] = 'hello world';",
Expand All @@ -643,6 +656,194 @@ ruleTester.run('no-get', rule, {
],
},

{
code: `
import { get } from '@ember/object';
import { somethingElse } from '@ember/object';
import { random } from 'random';
get(this, 'foo.firstObject.bar');
`,
output: `
import { get } from '@ember/object';
import { somethingElse } from '@ember/object';
import { random } from 'random';
this.foo?.[0]?.bar;
`,
options: [{ useOptionalChaining: true }],
errors: [{ message: ERROR_MESSAGE_GET, type: 'CallExpression' }],
},
{
code: `
import { get as g } from '@ember/object';
import { somethingElse } from '@ember/object';
import { random } from 'random';
g(obj.baz.qux, 'foo.firstObject.bar');
`,
output: `
import { get as g } from '@ember/object';
import { somethingElse } from '@ember/object';
import { random } from 'random';
obj.baz.qux.foo?.[0]?.bar;
`,
options: [{ useOptionalChaining: true }],
errors: [{ message: ERROR_MESSAGE_GET, type: 'CallExpression' }],
},
{
// `firstObject` used in the middle of a path.
// And the result of get() is chained (getResultIsChained=true).
code: "this.get('foo.firstObject.bar')[123];",
output: 'this.foo[0].bar[123];',
options: [{ useOptionalChaining: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
type: 'CallExpression',
},
],
},
{
// `firstObject` used in the middle of a path.
// And the resolved path of `get` is NOT chained (getResultIsChained=false).
code: "this.get('foo.firstObject.bar');",
output: 'this.foo?.[0]?.bar;',
options: [{ useOptionalChaining: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
type: 'CallExpression',
},
],
},
{
// `firstObject` used at the beginning of a path.
// And the resolved path of `get` is NOT chained (getResultIsChained=false).
code: "this.get('firstObject.bar');",
output: 'this[0]?.bar;',
options: [{ useOptionalChaining: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
type: 'CallExpression',
},
],
},
{
// `firstObject` used as the entire path.
// And the result of get() is chained (getResultIsChained=true).
code: "this.get('firstObject')[123];",
output: 'this[0][123];',
options: [{ useOptionalChaining: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
type: 'CallExpression',
},
],
},
{
// `firstObject` used in the middle of a path.
// And `get` is used in a left side of an assignment (isInLeftSideOfAssignmentExpression=true).
code: "this.get('foo.firstObject.bar')[123] = 'hello world';",
output: "this.foo[0].bar[123] = 'hello world';",
options: [{ useOptionalChaining: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
type: 'CallExpression',
},
],
},
{
// `lastObject` used in the middle of a path.
// And the result of get() is chained (getResultIsChained=true).
code: "this.get('foo.lastObject.bar')[123];",
output: 'this.foo[this.foo.length - 1].bar[123];',
options: [{ useOptionalChaining: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
type: 'CallExpression',
},
],
},
{
// `lastObject` used at the beginning of a path.
// And the result of get() is chained (getResultIsChained=true).
code: "this.get('lastObject.bar')[123];",
output: 'this[this.length - 1].bar[123];',
options: [{ useOptionalChaining: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
type: 'CallExpression',
},
],
},
{
// `lastObject` used as the entire path.
// And the result of get() is chained (getResultIsChained=true).
code: "this.get('lastObject')[123];",
output: 'this[this.length - 1][123];',
options: [{ useOptionalChaining: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
type: 'CallExpression',
},
],
},
{
// `lastObject` used in the middle of a path.
// And the resolved path of `get` is NOT chained (getResultIsChained=false).
code: "this.get('foo.lastObject.bar');",
output: 'this.foo?.[this.foo.length - 1]?.bar;',
options: [{ useOptionalChaining: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
type: 'CallExpression',
},
],
},
{
// `lastObject` used at the beginning of a path.
// And the resolved path of `get` is NOT chained (getResultIsChained=false).
code: "this.get('lastObject.bar');",
output: 'this[this.length - 1]?.bar;',
options: [{ useOptionalChaining: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
type: 'CallExpression',
},
],
},
{
// `lastObject` used in the middle of a path.
// When multiple `lastObject` are chained, it won't auto-fix.
code: "this.get('foo.lastObject.bar.lastObject')[123];",
output: null,
options: [{ useOptionalChaining: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
type: 'CallExpression',
},
],
},
{
// `lastObject` used at the beginning of a path.
// And the result of get() is chained (getResultIsChained=true).
code: "this.get('lastObject.bar.lastObject')[123];",
output: null,
options: [{ useOptionalChaining: true }],
errors: [
{
message: ERROR_MESSAGE_GET,
type: 'CallExpression',
},
],
},
{
// Reports violation after (classic) proxy class.
code: `
Expand Down

0 comments on commit 7e82eb1

Please sign in to comment.