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

Jump to bottom

Fix: prefer-const autofix (fixes #10582) #10987

Merged
merged 1 commit into from Nov 9, 2018
Merged

Fix: prefer-const autofix (fixes #10582) #10987

merged 1 commit into from Nov 9, 2018

Conversation

sstern6
Copy link
Contributor

@sstern6 sstern6 commented Oct 16, 2018

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)
Removed the guard checking for multiline var declarations to ensure autofix is applied.

Issue: #10582

Is there anything you'd like reviewers to focus on?
N/A

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Oct 16, 2018
platinumazure
platinumazure reviewed Oct 16, 2018
View reviewed changes
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Not sure about this approach- see inline comments...

tests/lib/rules/prefer-const.js Outdated
@@ -309,7 +309,7 @@ ruleTester.run("prefer-const", rule, {
},
{
code: "let {a = 0, b} = obj, c = a; b = a;",
output: null,
output: "const {a = 0, b} = obj, c = a; b = a;",
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this result will have a runtime error as b is reassigned after being declared const. Are you sure this is the right approach?

Copy link
Member

Choose a reason for hiding this comment

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

Can we also to make sure to add a test that has non-destructuring assignments as well (such as the examples listed here or here)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good @kaicataldo

tests/lib/rules/prefer-const.js Outdated
@@ -318,7 +318,7 @@ ruleTester.run("prefer-const", rule, {
},
{
code: "let {a = 0, b} = obj, c = a; b = a;",
output: null,
output: "const {a = 0, b} = obj, c = a; b = a;" ,
Copy link
Member

Choose a reason for hiding this comment

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

Previous comment applies here as well.

@sstern6
Copy link
Contributor Author

sstern6 commented Oct 16, 2018

Thanks @platinumazure will review the approach, good catch. The rule should guard against creating runtime errors like above?

@platinumazure
Copy link
Member

The rule should guard against creating runtime errors like above?

Yes- autofix should never result in incorrect code.

@sstern6
Copy link
Contributor Author

sstern6 commented Oct 16, 2018

Thanks @platinumazure will rework this PR, never worked with the auto-fixing aspect of a rule and this completely slipped my mind.

@kaicataldo kaicataldo added bug ESLint is working incorrectly rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Oct 19, 2018
@sstern6
Copy link
Contributor Author

sstern6 commented Oct 21, 2018

@platinumazure @kaicataldo PR and tests back up, let me know if you have any questions or comments.

Thanks

platinumazure
platinumazure reviewed Oct 21, 2018
View reviewed changes
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

I don't have time to do a full review now-- very sorry-- but I wanted to leave one small note while it's on my mind.

I hope to revisit this over the next few days. Thanks for your patience!

lib/rules/prefer-const.js Outdated
@@ -357,6 +357,9 @@ module.exports = {
const shouldMatchAnyDestructuredVariable = options.destructuring !== "all";
const ignoreReadBeforeAssign = options.ignoreReadBeforeAssign === true;
const variables = [];
let reportCount = 0;
const firstIdentifier = sourceCode.ast.tokens.find(node => node.type === "Identifier");
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused about this line. It looks like this line will grab the first identifier token in the program, without consideration for scope.

If you add a few tests that have some nested scopes, you might see some issues. For example:

const foo = 1;   // First identifier token is on this line

function someFunc() {
    let a = 0,
        b = 1;

    bar(a, b);
    // Both a and b should be made const in this case, but are they detected properly?
}

Copy link
Contributor Author

@sstern6 sstern6 Oct 21, 2018 •
edited

Choose a reason for hiding this comment

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

@platinumazure thanks for the feedback, good point, didnt think about this but yes this is detected properly. The line you commented on is just an initializer and then compares current with previous name. I will add tests for the various scoping options as well.

Update: Removed the logic for the first Identifier and just replaced the initialized name with an empty string.

@sstern6
Copy link
Contributor Author

sstern6 commented Oct 25, 2018

@platinumazure just following up on this, let me know if you have any questions or comments that I can address. Thank you!

@sstern6
Copy link
Contributor Author

sstern6 commented Nov 6, 2018 •
edited

Bump, if anyone has time to take a look at this.

platinumazure
platinumazure approved these changes Nov 6, 2018
View reviewed changes
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Nice test cases!

Very sorry for the delay in reviewing this.

@platinumazure
Copy link
Member

The new test cases don't seem to be multiline-- I think the focus of this issue has changed if I'm not mistaken? I think the goal now is to autofix when we know all parts of the declaration (all declarators in a VariableDeclaration, all properties in an ObjectPattern) can be made const, and whether something is multiline or not doesn't matter. Let me know if I'm mistaken... Thanks for your patience!

@sstern6
Copy link
Contributor Author

sstern6 commented Nov 6, 2018

@platinumazure youre right, it shouldnt matter if its multiline or not, Ill reformat the tests to be multiline.

Sound good?

For example, the code found https://github.com/eslint/eslint/pull/10987/files#diff-7bdeecf4f450247ac04335fce20a1e85R459.

I can change the code from let x = 'x', y = 'y';

to:

let x = 'x',
  y = 'y';

How does this sound? Did I answer your question?

@sstern6 sstern6 changed the title Fix: prefer-const autofix multiline assignment (fixes #10582) Fix: prefer-const autofix (fixes #10582) Nov 6, 2018
@platinumazure
Copy link
Member

platinumazure commented Nov 6, 2018 •
edited

As we discussed in Gitter-- the tests are fine, it's just that the commit message/PR title aren't quite accurate anymore. If others on the team are okay with this, we could just handle it as we merge.

I'm going to leave this open another couple of days for others to review, just in case.

@kaicataldo Did you want to take another look at this? Have your comments/questions been addressed?

btmills
btmills approved these changes Nov 9, 2018
View reviewed changes
Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for being so thorough on the tests, @sstern6!

@nzakas nzakas merged commit 54687a8 into eslint:master Nov 9, 2018
@nzakas
Copy link
Member

nzakas commented Nov 9, 2018

Thanks @sstern6!

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators May 9, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label May 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kaicataldo kaicataldo kaicataldo left review comments

@nzakas nzakas nzakas approved these changes

@platinumazure platinumazure platinumazure approved these changes

@btmills btmills btmills approved these changes

Assignees
No one assigned
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sstern6 @platinumazure @nzakas @btmills @kaicataldo