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-modern-dom-apis: Only fix when expression is not used #503

Merged

Conversation

brettz9
Copy link
Contributor

@brettz9 brettz9 commented Jan 24, 2020

Providing a fix related to this comment.

fix(prefer-modern-dom-apis): Ignore other cases where node return value is used

- Where parent is an ArrayExpression, IfStatement, MemberExpression, Property, ReturnStatement

Fixes a test which made a fix which would break because of an inability of the replacement API to return a value.

Also provides minor optimization to avoid declaring fixer with value when not needed.
Also applies (and for prefer-node-* rules, switches from) checking ExpressionStatement in favor of specific parents (BinaryExpression, CallExpression, ConditionalExpression, LogicalExpression, UnaryExpression), since in theory, the last part of parentNode.insertBefore(alfa, beta).insertBefore(charlie, delta); could be checked if the rule were changed to allow such parent expressions rather than requiring a parent member to be a name only (i.e., fixing parentNode in parent.insertBefore() but ignoring some.expression().insertBefore()) since technically such a replacement could be made. But allowing it to be made would allow additional refactoring and potentially reporting long errors, e.g., "Prefer some.very.very.long.expression().before() over some.very.very.long.expression().insertBefore", and I'm not sure it is worth the trouble to support this strange pattern. But I did think it at least more logically clear to check the parents.

fix(prefer-modern-dom-apis): Switch to whitelist for determining where node return values are expected safe not to be used.

For prefer-modern-dom-apis, the change will avoid fixers in more cases than just VariableDeclarator and AssignmentExpression parent types (as previously); will now avoid whenever parent is not an ExpressionStatement since many other parents, e.g., ArrayExpression, IfStatement, 'MemberExpression, Property, ReturnStatement, etc. can use a return value.

For prefer-node-* rules, rather than blacklisting an albeit more extensive set of parents, will also switch to a whitelist, potentially avoiding fixers on other contexts not previously avoided, e.g., AwaitExpression.

The previous avoidance by prefer-node-* of fixers with ExpressionStatement grandparents is not currently required.

Also:

  1. provides minor optimization to avoid declaring fixer with value when not needed.
  2. fixes a test which made a fix which would break because of an inability of the replacement API to return a value.
  3. refactors to use isValueNotUsable over isValueUsed

@fisker
Copy link
Collaborator

fisker commented Jan 26, 2020

@brettz9

Thanks for fixing this rule.

I think we still missing many cases on that parent type check, eg:

TemplateLiteral

`${a.insertBefore(b, c)}` // maybe no one do this way, but still possible

NewExpression

new Dom(a.insertBefore(b, c))

UnaryExpression

+a.insertBefore(b, c)

Not sure can we do opposite way? only allow certain types, or can we check all possible node types, and add more.

@fisker
Copy link
Collaborator

fisker commented Jan 26, 2020

Is this all possible types? https://esprima.readthedocs.io/en/latest/syntax-tree-format.html

@brettz9 brettz9 force-pushed the prefer-modern-dom-apis-returning-value branch from 6c8dab0 to 0e4b513 Compare January 27, 2020 06:53
@brettz9
Copy link
Contributor Author

brettz9 commented Jan 27, 2020

Ah, good to know re: other parent types. (The PR is already covering UnaryExpression, btw, but not TemplateLiteral or NewExpression; I've added tests for these and added them to the blacklisted parents if it is decided to continue that route.)

With ESLint's parser being Espree, I wouldn't be sure that would cover everything, though it does seem to be a project goal bring fundamental compatibility with Esprima. However, beyond this, other parsers are being used with eslint, including for TypeScript, so I wouldn't be surprised if there are even more.

As far as inverting the check, the one parent type in use in the tests which isn't caught here is ExpressionStatement. And that would seem to make sense as the one AST I can think of which would be possible and wouldn't have side effects. But I could be missing something. Let me know if you want to go that route.

@fisker
Copy link
Collaborator

fisker commented Jan 28, 2020

We still not cover all possible cases, like await, yield etc. Really hard to cover them all, maybe we can try do whitelist check.

/cc @sindresorhus WDYT?

@sindresorhus
Copy link
Owner

We still not cover all possible cases, like await, yield etc.

What do you mean exactly? await a.insertBefore(b, c)?

@brettz9
Copy link
Contributor Author

brettz9 commented Feb 1, 2020

Yeah, if I may answer, that is correct. As unlikely as such cases may be, there are other types of places in JS we are not currently addressing such as await where the return value may be significant (insertBefore gave a return value, whereas before does not, so we don't want to apply a fixer that substitutes one for the other when a return value is being used).

I think fisker and I may agree that only allowing fixers when the parent is a ExpressionStatement is the better choice, and to my knowledge, there are no other parent types where it would always be safe. The previous approach was to blacklist fixing for parent types where the return value was known to be usable (e.g., LogicalExpression), but there are quite a few types to cover in JS.

IMO, without going through each type (and that would require looking at TypeScript AST as well, to support users applying such a parser), in either approach, we could be having false negatives or false positives, so I think it would be better to go with the safer approach of only allowing the fixer for ExpressionStatement. Besides being the simpler solution, it avoids being potentially over-aggressive in adding a "fix" where it would be harmful--and we can always add to the whitelist if we find cases where it may be safely fixable.

@sindresorhus
Copy link
Owner

so I think it would be better to go with the safer approach of only allowing the fixer for ExpressionStatement.

👍

…ere node return values are expected safe not to be used.

For `prefer-modern-dom-apis`, the change will avoid fixers in more cases than just `VariableDeclarator` and `AssignmentExpression` parent types (as previously); will now avoid whenever parent is not an `ExpressionStatement` since many other parents, e.g., `ArrayExpression`, `IfStatement`, `'MemberExpression`,  `Property`, `ReturnStatement`, etc. can use a return value.

For `prefer-node-*` rules, rather than blacklisting an albeit more extensive set of parents, will also switch to a whitelist, potentially avoiding fixers on other contexts not previously avoided, e.g., `AwaitExpression`.

The previous avoidance by `prefer-node-*` of fixers with `ExpressionStatement` grandparents is not currently required.

Also:
1. provides minor optimization to avoid declaring fixer with value when not needed.
2. fixes a test which made a fix which would break because of an inability of the replacement API to return a value.
@brettz9 brettz9 force-pushed the prefer-modern-dom-apis-returning-value branch from 0e4b513 to ff4a6ca Compare February 1, 2020 06:37
@brettz9
Copy link
Contributor Author

brettz9 commented Feb 1, 2020

I've pushed a fix that only allows a fixer for ExpressionStatement parents.

@fisker
Copy link
Collaborator

fisker commented Feb 1, 2020

@brettz9

Should we change isValueUsed to negative isValueNotUsed, and only fix cases we are sure that value is not used?

@brettz9
Copy link
Contributor Author

brettz9 commented Feb 1, 2020

I think the result should be the same, but we can invert the naming (though maybe isValueNotUsable would be better since we don't know that the value will really be "used", e.g., if a function calling a function with a ReturnStatement doesn't use the value returned).

@fisker
Copy link
Collaborator

fisker commented Feb 1, 2020

Great, let's do this.

@brettz9
Copy link
Contributor Author

brettz9 commented Feb 1, 2020

I've updated

Copy link
Collaborator

@fisker fisker left a comment

Choose a reason for hiding this comment

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

Good job, thanks.

rules/utils/is-value-not-usable.js Outdated Show resolved Hide resolved
Co-Authored-By: fisker Cheung <lionkay@gmail.com>
@brettz9 brettz9 force-pushed the prefer-modern-dom-apis-returning-value branch from 87d8431 to a5d5d53 Compare February 1, 2020 10:03
@fisker fisker changed the title fix(prefer-modern-dom-apis): Ignore other cases where node return value is used prefer-modern-dom-apis: Only fix when expression is not used Feb 1, 2020
@fisker fisker merged commit 096fead into sindresorhus:master Feb 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants