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

Extra empty line at the end of a block because that empty line is a comment #670

Closed
zimmski opened this issue Apr 7, 2022 · 7 comments · Fixed by #708
Closed

Extra empty line at the end of a block because that empty line is a comment #670

zimmski opened this issue Apr 7, 2022 · 7 comments · Fixed by #708

Comments

@zimmski
Copy link
Contributor

zimmski commented Apr 7, 2022

(i am currently in the process of adding "revive" to our linter stack and i am going through the false-positives for us)

Describe the bug

{
   some code

   // comment
}

leads to "extra empty line at the end of a block" even though it shouldn't.

Expected behavior

Such comments are meant for exactly being at that location. Marking the comment as a probem, is a false-positive for our source code.

@zimmski
Copy link
Contributor Author

zimmski commented Apr 7, 2022

Same is true for

{
// comment
}

@git-hulk
Copy link
Contributor

git-hulk commented Jul 5, 2022

@chavacava I would like to handle this issue.

@git-hulk
Copy link
Contributor

git-hulk commented Jul 6, 2022

Same is true for

{
// comment
}

It's true since revive will check nothing when the block list is empty, so any empty line in the empty block will be ok(I think it can also regard as a bug).

@git-hulk
Copy link
Contributor

git-hulk commented Jul 6, 2022

For this case:

{
   some code

   // comment
}

The owner(key in w.cmap) of the comment line is FuncDef instead of the last node, so is between comment check is false. The workaround can be that records the current block's FuncDef and lookup the comment group with FuncDef node as well.

How do you think about this solution? @chavacava

Workaround from the user side can be:

{
   some code
   // comment
}

then the owner of the comment line is last node instead of FuncDef in Go ast.

@chavacava
Copy link
Collaborator

Fixing the bug might require to stop using the cmap as it is currently used.
The way comments are attached to the AST nodes does not help us.

We could just extract from cmap the list of positions (line, col) of all comments in the whole source file. Then, using that list, for each statement block we can check if there is an empty line (i.e. without an statement or a comment) just after/before the start/end of the open/close curly brace.

For example:

{
   // this is an example
  println("Hello")

   // comment
}

From the above code we can get the list of positions of comments: [(2,3),(5,3)]
Then we check the block as follows:

  1. Check the beginning of the block: Do the line just after the start of the block (line 2) is the position of the first statement OR the position of a comment? Yes (line 2 is in the list of comments positions) then OK.
  2. Check the end of the block: Do the line just before the end of the block (line 5) is the position of the last statement of the block OR the position of a comment? Yes (line 5 is in the list of comments positions) then OK..

chavacava added a commit that referenced this issue Jul 8, 2022
@chavacava
Copy link
Collaborator

@git-hulk please take a look into this commit as a guide on how the bug can be fixed

@git-hulk
Copy link
Contributor

git-hulk commented Jul 11, 2022

Cool, thanks for @chavacava guide. I have a look at your patch and it works good now.

git-hulk pushed a commit to bitleak/revive that referenced this issue Jul 12, 2022
@chavacava chavacava mentioned this issue Jul 14, 2022
chavacava added a commit that referenced this issue Jul 14, 2022
subham-deepsource pushed a commit to DeepSourceCorp/revive that referenced this issue Aug 5, 2022
Signed-off-by: subham sarkar <[email protected]>
subham-deepsource pushed a commit to DeepSourceCorp/revive that referenced this issue Aug 5, 2022
Signed-off-by: subham sarkar <[email protected]>
subham-deepsource pushed a commit to DeepSourceCorp/revive that referenced this issue Aug 5, 2022
Signed-off-by: subham sarkar <[email protected]>
subham-deepsource added a commit to DeepSourceCorp/revive that referenced this issue Aug 5, 2022
* Separating lib from cli (mgechev#655)

* Separating lib from cli

* Renamed NewRevive to New

* Added GetLintFailures helper function

* Moved formatter to call to format since that's when it's needed

* makes fields of Revive struct non-public

* minor modifs in tests: remove unnamed constats

* Added lint package management to lint command

* README message for using revive as a library

* README formatting

* Removed unused method

* Slightly improved wording in README

* Handling format errors

* Renaming file to better reflect intent

* Refactoring pattern usage

* README heads

* renames excludePaths into excludePatterns

Co-authored-by: Bernardo Heynemann <[email protected]>
Co-authored-by: chavacava <[email protected]>
Signed-off-by: subham sarkar <[email protected]>

* Update the contributors list

Signed-off-by: subham sarkar <[email protected]>

* Remove debugging output (mgechev#672)

Noticed during migration from our heavily modified "go-lint" to "revive" that there is an additional line printed. I am unsure that the convention for this project is on this, we do not allow adding such a call.

Signed-off-by: subham sarkar <[email protected]>

* Remove built-in types that existing only for the Go documentation (mgechev#675)

Since these types only exist for documenting Go's standard library there
should be no reason to mark them.

Closes mgechev#673

Signed-off-by: subham sarkar <[email protected]>

* Fix/677 (mgechev#678)

Signed-off-by: subham sarkar <[email protected]>

* Lint cleanup (mgechev#679)

Signed-off-by: subham sarkar <[email protected]>

* add rule datarace (mgechev#683)

Signed-off-by: subham sarkar <[email protected]>

* Fixes issue mgechev#619 imports-blacklist support regex (mgechev#684)

* Fixes issue mgechev#619 imports-blacklist support regex

* refactors method name and error message

* restores original test cases

Co-authored-by: chavacava <[email protected]>
Signed-off-by: subham sarkar <[email protected]>

* fix(var-naming): set node to package name for underscore in package name (mgechev#689)

Setting the entire file AST as the node causes golangci-lint to print
the entire file source as the context, and line and column numbers set
to 1. Point to the package name node instead.

Closes mgechev#688

Signed-off-by: subham sarkar <[email protected]>

* fix(deps): update module golang.org/x/tools to v0.1.11 (mgechev#696)

Co-authored-by: Renovate Bot <[email protected]>
Signed-off-by: subham sarkar <[email protected]>

* fix(deps): update github.com/chavacava/garif digest to 908ad76 (mgechev#695)

Co-authored-by: Renovate Bot <[email protected]>
Signed-off-by: subham sarkar <[email protected]>

* fix(receiver-naming): distinguish types with parameters (mgechev#692)

* fix(receiver-naming): distinguish types with parameters

* chore: run tests using supported Go versions matrix

Signed-off-by: subham sarkar <[email protected]>

* Make package comment more confident (mgechev#694)

Signed-off-by: subham sarkar <[email protected]>

* fix(deps): update github.com/chavacava/garif digest to d6fd61e (mgechev#699)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: subham sarkar <[email protected]>

* fix issue mgechev#691 (mgechev#700)

Signed-off-by: subham sarkar <[email protected]>

* fix(deps): update github.com/chavacava/garif digest to 9351721 (mgechev#702)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: subham sarkar <[email protected]>

* Allow to customize user functions in rule `error-strings` (mgechev#703)

* Allow to customize user functions in rule `error-strings`

* Rollback the Available Rules table format in README

* adds memoization of the rule's configuration

Co-authored-by: chavacava <[email protected]>
Signed-off-by: subham sarkar <[email protected]>

* doc: add devlake to README usage (mgechev#704)

Co-authored-by: linyh <[email protected]>
Signed-off-by: subham sarkar <[email protected]>

* Check whether the tag name is duplicate or not (mgechev#706)

* Check whether the tag name is duplicate or not

* - minor refactoring
- continues checking tag even if name is repeated

* adds test cases for duplicated tag names

* adds test case with two tag types (json & yaml)

* Fix allow the same tag name in different tag key

* fix checks on protobuf tag names

Co-authored-by: chavacava <[email protected]>
Signed-off-by: subham sarkar <[email protected]>

* fix mgechev#670 (mgechev#708)

Signed-off-by: subham sarkar <[email protected]>

* Fix module name

Signed-off-by: subham sarkar <[email protected]>

Co-authored-by: Bernardo Heynemann <[email protected]>
Co-authored-by: Bernardo Heynemann <[email protected]>
Co-authored-by: chavacava <[email protected]>
Co-authored-by: mgechev <[email protected]>
Co-authored-by: Markus Zimmermann <[email protected]>
Co-authored-by: Markus Zimmermann <[email protected]>
Co-authored-by: Yudai Takada <[email protected]>
Co-authored-by: Ville Skyttä <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Renovate Bot <[email protected]>
Co-authored-by: Ivan Trubach <[email protected]>
Co-authored-by: okhowang <[email protected]>
Co-authored-by: hulk <[email protected]>
Co-authored-by: likyh <[email protected]>
Co-authored-by: linyh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants