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.

Sign up for GitHub

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

Jump to bottom

Do not autofix self-referential computed properties in require-computed-macros rule #1121

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Do not autofix self-referential computed properties in require-computed-macros rule #1121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
605ca00
fix: require-computed-macros should not autofix self-referential comp…
runspired Mar 29, 2021
402a25d
don't specify a fix if we won't be able to fix
runspired Mar 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
168 changes: 112 additions & 56 deletions lib/rules/require-computed-macros.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,17 @@ function getThisExpressions(nodeLogicalExpression) {
return arrayOfThisExpressions.reverse();
}

function detectCircularKey(node, macroArgs) {
if (node.parent && node.parent.key) {
Copy link
Member

Choose a reason for hiding this comment

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

There should always be a parent, but we do need to check what node type the parent is before we check its key. That way, we won't end up using the key of some unexpected kind of node.

Copy link
Author

Choose a reason for hiding this comment

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

oddly I believe I encountered a no parent situation in our own test suite. Will double check that.

Copy link
Member

Choose a reason for hiding this comment

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

ESLint adds the parent when it runs the rule test cases. But if you write separate unit tests for this function by itself, then it won't have a parent. So feel free to add the parent guard but you only need if you write separate unit tests for the function itself.

// `class { foo = computed() }` and `EmberObject({ foo: computed() })` usage
return macroArgs.includes(node.parent.key.name);
} else if (types.isMethodDefinition(node) && node.key.name) {
// class { @computed() get foo() }
return macroArgs.includes(node.key.name);
}
return false;
}

function makeFix(nodeToReplace, macro, macroArgs) {
const isDecoratorUsage = types.isMethodDefinition(nodeToReplace);
const prefix = isDecoratorUsage ? '@' : ''; // Decorator usage has @ symbol prefixing computed()
Expand Down Expand Up @@ -161,71 +172,116 @@ module.exports = {

create(context) {
function reportSingleArg(nodeComputedProperty, nodeWithThisExpression, macro) {
context.report({
node: nodeComputedProperty,
message: makeErrorMessage(macro),
fix(fixer) {
const text = propertyGetterUtils.nodeToDependentKey(nodeWithThisExpression, context);
return fixer.replaceText(
nodeComputedProperty,
makeFix(nodeComputedProperty, macro, `'${text}'`)
);
},
});
const text = propertyGetterUtils.nodeToDependentKey(nodeWithThisExpression, context);
const isNotFixable = detectCircularKey(nodeComputedProperty, [text]);

if (isNotFixable) {
context.report({
Copy link
Member

Choose a reason for hiding this comment

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

Instead of duplicating the context.report call, we should actually just return null inside the fixer function if we encounter a non-fixable situation. That goes for all these reporting functions and it will simplify all this code a lot.

fix(fixer) {
  if (!isFixable(...)) { return null; }
  ...  
}

Copy link
Author

Choose a reason for hiding this comment

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

@bmish I did that initially but it is linted against, I also suspect it's not good practice since it might cause eslint to report the violation as fixable when it is not.

Copy link
Member

@bmish bmish Apr 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It's not linted against, we actually have some examples of autofixers with return null in our codebase. And it's not bad practice, in fact it's common practice to have autofixers which ignore certain situations, and this is how we do it. I can assure you it will result in much simpler code :)

Copy link
Author

Choose a reason for hiding this comment

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

this was the lint rule it triggered https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/fixer-return.md

Copy link
Member

Choose a reason for hiding this comment

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

Using return null won't trigger that rule. return null indicates that you don't want to autofix in a particular situation.

node: nodeComputedProperty,
message: makeErrorMessage(macro),
});
} else {
context.report({
node: nodeComputedProperty,
message: makeErrorMessage(macro),
fix(fixer) {
const text = propertyGetterUtils.nodeToDependentKey(nodeWithThisExpression, context);
const fix = makeFix(nodeComputedProperty, macro, `'${text}'`);

return fixer.replaceText(nodeComputedProperty, fix);
},
});
}
}

function reportBinaryExpression(nodeComputedProperty, nodeBinaryExpression, macro) {
context.report({
node: nodeComputedProperty,
message: makeErrorMessage(macro),
fix(fixer) {
const sourceCode = context.getSourceCode();
const textLeft = propertyGetterUtils.nodeToDependentKey(
nodeBinaryExpression.left,
context
);
const textRight = sourceCode.getText(nodeBinaryExpression.right);
return fixer.replaceText(
nodeComputedProperty,
makeFix(nodeComputedProperty, macro, `'${textLeft}', ${textRight}`)
);
},
});
const sourceCode = context.getSourceCode();
const argLeft = propertyGetterUtils.nodeToDependentKey(nodeBinaryExpression.left, context);
const argRight = sourceCode.getText(nodeBinaryExpression.right);
const isNotFixable = detectCircularKey(nodeComputedProperty, [argLeft, argRight]);

if (isNotFixable) {
context.report({
node: nodeComputedProperty,
message: makeErrorMessage(macro),
});
} else {
context.report({
node: nodeComputedProperty,
message: makeErrorMessage(macro),
fix(fixer) {
const sourceCode = context.getSourceCode();
const textLeft = propertyGetterUtils.nodeToDependentKey(
nodeBinaryExpression.left,
context
);
const textRight = sourceCode.getText(nodeBinaryExpression.right);
const fix = makeFix(nodeComputedProperty, macro, `'${textLeft}', ${textRight}`);

return fixer.replaceText(nodeComputedProperty, fix);
},
});
}
}

function reportLogicalExpression(nodeComputedProperty, nodeLogicalExpression, macro) {
context.report({
node: nodeComputedProperty,
message: makeErrorMessage(macro),
fix(fixer) {
const text = getThisExpressions(nodeLogicalExpression)
.map((node) => propertyGetterUtils.nodeToDependentKey(node, context))
.join("', '");
return fixer.replaceText(
nodeComputedProperty,
makeFix(nodeComputedProperty, macro, `'${text}'`)
);
},
});
const compArgs = getThisExpressions(nodeLogicalExpression).map((node) =>
propertyGetterUtils.nodeToDependentKey(node, context)
);
const isNotFixable = detectCircularKey(nodeComputedProperty, compArgs);

if (isNotFixable) {
context.report({
node: nodeComputedProperty,
message: makeErrorMessage(macro),
});
} else {
context.report({
node: nodeComputedProperty,
message: makeErrorMessage(macro),
fix(fixer) {
const text = getThisExpressions(nodeLogicalExpression)
.map((node) => propertyGetterUtils.nodeToDependentKey(node, context))
.join("', '");
const fix = makeFix(nodeComputedProperty, macro, `'${text}'`);

return fixer.replaceText(nodeComputedProperty, fix);
},
});
}
}

function reportFunctionCall(nodeComputedProperty, nodeCallExpression, macro) {
context.report({
node: nodeComputedProperty,
message: makeErrorMessage(macro),
fix(fixer) {
const sourceCode = context.getSourceCode();
const arg1 = propertyGetterUtils.nodeToDependentKey(
nodeCallExpression.callee.object,
context
);
const restOfArgs = nodeCallExpression.arguments.map((arg) => sourceCode.getText(arg));
return fixer.replaceText(
nodeComputedProperty,
makeFix(nodeComputedProperty, macro, `'${arg1}', ${restOfArgs.join(', ')}`)
);
},
});
const sourceCode = context.getSourceCode();
const arg1 = propertyGetterUtils.nodeToDependentKey(
nodeCallExpression.callee.object,
context
);
const restOfArgs = nodeCallExpression.arguments.map((arg) => sourceCode.getText(arg));
const isNotFixable = detectCircularKey(nodeComputedProperty, [arg1, ...restOfArgs]);

if (isNotFixable) {
context.report({
node: nodeComputedProperty,
message: makeErrorMessage(macro),
});
} else {
context.report({
node: nodeComputedProperty,
message: makeErrorMessage(macro),
fix(fixer) {
const sourceCode = context.getSourceCode();
const arg1 = propertyGetterUtils.nodeToDependentKey(
nodeCallExpression.callee.object,
context
);
const restOfArgs = nodeCallExpression.arguments.map((arg) => sourceCode.getText(arg));
const fix = makeFix(nodeComputedProperty, macro, `'${arg1}', ${restOfArgs.join(', ')}`);

return fixer.replaceText(nodeComputedProperty, fix);
},
});
}
}

let importedEmberName;
Expand Down
51 changes: 51 additions & 0 deletions tests/lib/rules/require-computed-macros.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,5 +287,56 @@ ruleTester.run('require-computed-macros', rule, {
output: "class Test { @computed.mapBy('children', 'age') someProp }",
errors: [{ message: ERROR_MESSAGE_MAP_BY, type: 'MethodDefinition' }],
},

// Self Referential
Copy link
Member

Choose a reason for hiding this comment

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

I think we're adding a few redundant test cases as well as missing some test cases.

We need the following test cases basic test cases which test a simple single-arg return expression that will be converted into the reads macro:

  • class Test { @computed() get foo() { return this.foo; } }
  • class Test { foo = computed(function() { return this.foo; }) } // non-decorator style
  • import EmberObject from '@ember/object'; EmberObject.extend({ foo: computed(function() { return this.foo; }) } // classic class

And we need to test other types of expressions that will be converted into other macros:

  • class Test { @computed() get a() { return this.a && this.b; } } // logical
    *class Test { @computed() get a() { return this.a > this.b; } } // binary
    *import EmberObject from '@ember/object'; EmberObject.extend({ computed(function() { return this.chores.filterBy('done', true); }) // function

We don't need to test anything related to the dependent key arguments (computed('this-dependent-key-does-not-matter', function () {})) since those end up ignored anyway.

Copy link
Author

Choose a reason for hiding this comment

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

The redundancy is intentional, it covers both forms of classic and both forms of class based declarations both with and without args being supplied to the computed as dependent keys (since I encountered errors from conversion from both in the wild).

Can add tests for a few of the others.

Copy link
Member

@bmish bmish Apr 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good. A comment next to each test case explaining what variation it is testing would help.

{
code: 'class Test { @computed() get foo() { return this.foo; } }',
options: [{ includeNativeGetters: true }],
output: 'class Test { @computed() get foo() { return this.foo; } }',
Copy link
Member

Choose a reason for hiding this comment

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

We should use output: null to indicate there's no autofix, it's more clear and concise.

Copy link
Author

Choose a reason for hiding this comment

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

wouldn't that imply the rule was deleting code?

Copy link
Member

Choose a reason for hiding this comment

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

No. It means there's no autofix.

And there's actually a lint rule to enforce this but unfortunately it's not working on this code because the array of test cases has .map(addComputedImport) on it.

https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/prefer-output-null.md

errors: [{ message: ERROR_MESSAGE_READS, type: 'MethodDefinition' }],
},
{
code: "class Test { @computed('foo') get foo() { return this.foo; } }",
options: [{ includeNativeGetters: true }],
output: "class Test { @computed('foo') get foo() { return this.foo; } }",
errors: [{ message: ERROR_MESSAGE_READS, type: 'MethodDefinition' }],
},
{
code: "class Test { @computed('foo', 'bar') get foo() { return this.foo; } }",
options: [{ includeNativeGetters: true }],
output: "class Test { @computed('foo', 'bar') get foo() { return this.foo; } }",
errors: [{ message: ERROR_MESSAGE_READS, type: 'MethodDefinition' }],
},
{
code: "class Test { foo = computed('foo', function() { return this.foo; }) }",
output: "class Test { foo = computed('foo', function() { return this.foo; }) }",
errors: [{ message: ERROR_MESSAGE_READS, type: 'CallExpression' }],
},
{
code: "class Test { foo = computed('foo', 'bar', function() { return this.foo; }) }",
output: "class Test { foo = computed('foo', 'bar', function() { return this.foo; }) }",
errors: [{ message: ERROR_MESSAGE_READS, type: 'CallExpression' }],
},
{
code:
"import EmberObject from '@ember/object'; const Test = EmberObject.extend({ foo: computed('foo', function() { return this.foo; }) })",
output:
"import EmberObject from '@ember/object'; const Test = EmberObject.extend({ foo: computed('foo', function() { return this.foo; }) })",
errors: [{ message: ERROR_MESSAGE_READS, type: 'CallExpression' }],
},
{
code:
"import EmberObject from '@ember/object'; const Test = EmberObject.extend({ foo: computed('foo', 'bar', function() { return this.foo; }) })",
output:
"import EmberObject from '@ember/object'; const Test = EmberObject.extend({ foo: computed('foo', 'bar', function() { return this.foo; }) })",
errors: [{ message: ERROR_MESSAGE_READS, type: 'CallExpression' }],
},
{
code:
"import EmberObject from '@ember/object'; const Test = EmberObject.extend({ foo: computed(function() { return this.foo; }) })",
output:
"import EmberObject from '@ember/object'; const Test = EmberObject.extend({ foo: computed(function() { return this.foo; }) })",
errors: [{ message: ERROR_MESSAGE_READS, type: 'CallExpression' }],
},
].map(addComputedImport),
});