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

Update: autofix bug in lines-between-class-members (fixes #12391) #12632

Merged
merged 7 commits into from Dec 20, 2019
Merged

Update: autofix bug in lines-between-class-members (fixes #12391) #12632

merged 7 commits into from Dec 20, 2019

Conversation

yeonjuan
Copy link
Member

@yeonjuan yeonjuan commented Dec 2, 2019

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)

This PR will fix three bugs in lines-between-class-members.

  1. Disallow lines-between-class-members deletes comments and docblocks between methods #12391

  2. doesn't work well with semicolons.

    • the actual master version.
    /* eslint lines-between-class-members: ["error", "always"] */
    
    class A {
      foo() {}
      /* comment */;
      ;  // No error, but here is no blank line.
      bar() {}
    }
    • this PR
    /* eslint lines-between-class-members: ["error", "always"] */
    
    class A {
      foo() {}
      /* comment */; // Lint Error
      ; 
      bar() {}
    }
  3. incorrect fixing trailing comments

    /* eslint lines-between-class-members: ["error", "always"] */
    
    class foo{ 
      bar(){} //comment
      baz(){}
    }
    • the actual master version
    /* eslint lines-between-class-members: ["error", "always"] */
    
     class foo{ 
       bar(){}
      //comment
       baz(){}
     }
    • this PR
    /* eslint lines-between-class-members: ["error", "always"] */
    
     class foo{ 
       bar(){} //comment
    
       baz(){}
     }

Is there anything you'd like reviewers to focus on?
#12391

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Dec 2, 2019
@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion autofix This change is related to ESLint's autofixing capabilities bug ESLint is working incorrectly rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Dec 4, 2019
@yeonjuan yeonjuan changed the title Fix: comments deletion bug in lines-between-class-members(fixes #12391) Fix: autofix bug in lines-between-class-members(fixes #12391) Dec 5, 2019
@yeonjuan yeonjuan changed the title Fix: autofix bug in lines-between-class-members(fixes #12391) Fix: autofix bug in lines-between-class-members (fixes #12391) Dec 5, 2019
Copy link
Member

@kaicataldo kaicataldo 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 working on this! I'd like at least one more pair of eyes on this before we merge.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

This is a very nice code but I think it can still remove comments in some cases, in particular when there are blank lines both before and after the comment?

/* eslint lines-between-class-members:["error", "never"] */

class MyClass {
    myMethod1 (param) {
    }
    
    /** 
     * this comment will be removed?
     */
    
    myMethod2 (param) {
    }
}

Wondering if we should anyway merge this because it does fix the original issue (and these should be the most common scenarios), or maybe implement something similar to how padding-line-between-statements handles comments.

Another solution might be to ignore class members that have comments between them and defer to the lines-around-comment rule. It seems that these two rules can have some unavoidable conflicts anyway.

@yeonjuan
Copy link
Member Author

yeonjuan commented Dec 8, 2019

@mdjermanovic
oh..Yes. this is how padding-line-between-statements handles comments - demo(no fix).
I changed it. thanks! :)

@mdjermanovic
Copy link
Member

Thank for the change! Looks like the comments are completely safe now.

There is another thing to note. In some extremely rare cases, this fix can change the behavior of this rule to report more or fewer warnings. The actual master version looks for comments before the next member (using sourceCode.getCommentsBefore, which returns comments after the first preceding non-comment token), while this fix searches from both sides.

This will almost always produce the same result at the end, but a class body can also contain useless empty members (semicolons), which don't exist in AST.

An example where the actual version doesn't report warning, but the version from this PR does:

/* eslint lines-between-class-members: ["error", "always"] */

class A {
  foo() {}
  /* comment */;
  bar() {}
}

This rule in general at the moment doesn't work well with these semicolons. I think it's okay to merge this PR as is (just mark with Update because it can produce more warnings) and maybe later analyze and fix handling of semicolons in a separate PR.

@yeonjuan yeonjuan changed the title Fix: autofix bug in lines-between-class-members (fixes #12391) Update: autofix bug in lines-between-class-members (fixes #12391) Dec 9, 2019
@yeonjuan
Copy link
Member Author

yeonjuan commented Dec 9, 2019

@mdjermanovic
Thanks for the elaborate review. I changed the pr message(fix -> update).

@kaicataldo
Copy link
Member

TIL! Nice catch, @mdjermanovic 👍

I'm personally not sure we should defer the fix until later. Would it be possible to check for semicolon tokens in addition to comments in this PR?

@yeonjuan
Copy link
Member Author

@kaicataldo @mdjermanovic

May I ask some questions?
Before I move on, I would like to double-check whether I understand correctly. :)

The actual version doesn't report an error in case1. But I assume that this is a bug. ...right?

  • case1
/* eslint lines-between-class-members: ["error", "always"] */

class A {
  foo() {}
  /* comment */; // No error, but here is no blank line.
  bar() {}
}

The PR version report an error in case1, but no error in case2.

  • case2
/* eslint lines-between-class-members: ["error", "always"] */

class A {
  foo() {}
  /* comment */;
  ;  // No error, but here is no blank line.
  bar() {}
}

So I need to add check for semicolon tokens. Do I get this right?

@mdjermanovic
Copy link
Member

The actual version doesn't report an error in case1. But I assume that this is a bug. ...right?

In my opinion - yes, this is a bug, It seems that the actual master version doesn't handle semicolons well. In some cases, this rule can even remove them:

/* eslint lines-between-class-members: ["error", "never"] */

class A {
  foo() {}
  ;
  bar() {}
}

This is fixed to:

/* eslint lines-between-class-members: ["error", "never"] */

class A {
  foo() {}
bar() {}
}

This doesn't look like something this rule should do, no-extra-semi removes these semicolons.

So I need to add check for semicolon tokens. Do I get this right?

While these semicolons are technically class members (I think), this rule should probably treat them as it treats comments - don't require/disallow empty lines around semicolons, but a line with a semicolon shouldn't be seen as an empty line.

Maybe getLastConsecutiveTokenAfter/getFirstConsecutiveTokenBefore shouldn't explicitly check for comments and semicolons, but use first/last token of the following/previous class members as a stop token instead.

@yeonjuan
Copy link
Member Author

yeonjuan commented Dec 10, 2019

@mdjermanovic @kaicataldo
there is one more bug(in my opinion) in the actual version. - demo

/* eslint lines-between-class-members: ["error", "always"] */

class foo{ 
  bar(){} //comment
  baz(){}
}

This is fixed to:

/* eslint lines-between-class-members: ["error", "always"] */

class foo{ 
  bar(){}

  //comment
  baz(){}
}

It's because the actual version inserts newline only after the last member node without considering trailing comments or semicolon.
I pushed the commit(7ec30b2) includes fixing this problem also. thanks.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

Thanks for all the changes!

This PR now fixes 3 different bugs: #12391, semicolons, and "always" autofix.

I left two notes for some minor edge cases.

lib/rules/lines-between-class-members.js Outdated Show resolved Hide resolved
lib/rules/lines-between-class-members.js Outdated Show resolved Hide resolved
@yeonjuan
Copy link
Member Author

@mdjermanovic
thanks for the reviews. I'm all done :)

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

Looks good! Just to remove some probably extra chars in the tests, to avoid confusion about what was the intention of these tests.

tests/lib/rules/lines-between-class-members.js Outdated Show resolved Hide resolved
tests/lib/rules/lines-between-class-members.js Outdated Show resolved Hide resolved
tests/lib/rules/lines-between-class-members.js Outdated Show resolved Hide resolved
@yeonjuan
Copy link
Member Author

@mdjermanovic
Oh.. sorry for my mistake. I fixed typo.

Copy link
Member

@mdjermanovic mdjermanovic 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!

@kaicataldo kaicataldo merged commit 5c25a26 into eslint:master Dec 20, 2019
@kaicataldo
Copy link
Member

Thanks for contributing!

@yeonjuan yeonjuan deleted the line-between-class-member branch January 3, 2020 14:51
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jun 19, 2020
@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 Jun 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 autofix This change is related to ESLint's autofixing capabilities bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants