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

Rubocop warnings with zero length do not show up in editor #466

Open
trkoch opened this Issue Apr 5, 2019 · 8 comments

Comments

Projects
None yet
2 participants
@trkoch
Copy link

commented Apr 5, 2019

Your environment

  • vscode-ruby version: 0.22.3
  • Ruby version: 2.6 (also tested with 2.5)
  • Ruby version manager (if any): rbenv
  • VS Code version: 1.33.0 (also tested with previous version)
  • Operating System: macOS
  • Using language server? Yes (cannot reproduce without language server)

Expected behavior

Extension reports all warnings detected by rubocop as problems and decorates significant characters.

Actual behavior

I'm having the issue that some warnings reported by rubocop (when run manually) do not show up in the editor (0 problems, no decorations). I noticed this applies to errors where start_column/last_column in JSON output of rubocop are identical.

Hints

range: {
start: {
line: offense.location.line - 1,
character: offenseCharacter,
},
end: {
line: offense.location.line - 1,
character: offenseCharacter + offense.location.length - 1,
},

Are you sure it's required to subtract from offense.location.length? Removing the subtraction appears to fix this error and also a glitch where the last character of an error is not decorated.

@wingrunr21

This comment has been minimized.

Copy link
Collaborator

commented Apr 8, 2019

I'm sure it is required as VSCode uses a 0 offset while RuboCop uses a 1 offset.

Do you have an example file I can use?

@trkoch

This comment has been minimized.

Copy link
Author

commented Apr 14, 2019

Is this true for both lines and columns?

Example

$ cat some_controller.rb

# frozen_string_literal: true

class SomeController < ApplicationController
  def show
    @record = Record.find(params[:id])
  # end
end

$ rubocop some_controller.rb -f json

{
  "metadata": {
    "rubocop_version": "0.65.0",
    "ruby_engine": "ruby",
    "ruby_version": "2.6.2",
    "ruby_patchlevel": "47",
    "ruby_platform": "x86_64-darwin18"
  },
  "files": [
    {
      "path": "some_controller.rb",
      "offenses": [
        {
          "severity": "error",
          "message": "Lint/Syntax: unexpected token $end\n(Using Ruby 2.6 parser; configure using `TargetRubyVersion` parameter, under `AllCops`)",
          "cop_name": "Lint/Syntax",
          "corrected": false,
          "location": {
            "start_line": 8,
            "start_column": 1,
            "last_line": 8,
            "last_column": 0,
            "length": 0,
            "line": 8,
            "column": 1
          }
        }
      ]
    }
  ],
  "summary": {
    "offense_count": 1,
    "target_file_count": 1,
    "inspected_file_count": 1
  }
}
diff --git a/server/src/linters/RuboCop.ts b/server/src/linters/RuboCop.ts
index 2bc01c4..df27c20 100644
--- a/server/src/linters/RuboCop.ts
+++ b/server/src/linters/RuboCop.ts
@@ -71,6 +71,7 @@ export default class RuboCop extends BaseLinter {
                        const offenses: IRuboCopResults = JSON.parse(data);

                        for (const file of offenses.files) {
+                               file.offenses.forEach(o => console.log(this.rubocopOffenseToDiagnostic(o)));
                                const diagnostics = file.offenses.map(o => this.rubocopOffenseToDiagnostic(o));
                                results = results.concat(diagnostics);
                        }

Current behavior

Ruby Language Server (Output)

Lint: executing rubocop -s /Users/tristan/Desktop/project/some_controller.rb -f json --force-exclusion...
{ range:
   { start: { line: 7, character: 0 },
     end: { line: 7, character: -1 } },
  severity: 1,
  message:
   'Lint/Syntax: unexpected token $end\n(Using Ruby 2.6 parser; configure using `TargetRubyVersion` parameter, under `AllCops`)',
  source: 'Lint/Syntax',
  code: 'RuboCop' }

Note -1 in range.end.character.

Proposed changes

diff --git a/server/src/linters/RuboCop.ts b/server/src/linters/RuboCop.ts
index 2bc01c4..df27c20 100644
--- a/server/src/linters/RuboCop.ts
+++ b/server/src/linters/RuboCop.ts
@@ -91,7 +92,7 @@ export default class RuboCop extends BaseLinter {
                                },
                                end: {
                                        line: offense.location.line - 1,
-                                       character: offenseCharacter + offense.location.length - 1,
+                                       character: offenseCharacter + offense.location.length,
                                },
                        },
                        severity: this.DIAGNOSTIC_SEVERITIES[offense.severity],

Ruby Language Server (Output)

Lint: executing rubocop -s /Users/tristan/Desktop/project/some_controller.rb -f json --force-exclusion...
{ range:
   { start: { line: 7, character: 0 },
     end: { line: 7, character: 0 } },
  severity: 1,
  message:
   'Lint/Syntax: unexpected token $end\n(Using Ruby 2.6 parser; configure using `TargetRubyVersion` parameter, under `AllCops`)',
  source: 'Lint/Syntax',
  code: 'RuboCop' }

Note 0 in range.end.character.

1 Problem

image

@wingrunr21

This comment has been minimized.

Copy link
Collaborator

commented Apr 14, 2019

yes it is true. You aren't taking into account that RuboCop is outputting an end position before the start position for this particular error. That's why it appears to "work" when you remove the subtraction.

@trkoch

This comment has been minimized.

Copy link
Author

commented Apr 14, 2019

Building on the previous example, another example…

This time VSCode correctly indicates a warning, but fails to correctly highlight its boundaries.

Current behavior

image

Proposed changes

image

@wingrunr21

This comment has been minimized.

Copy link
Collaborator

commented Apr 14, 2019

def foo
end

Does that give you an EmptyMethod warning with the whole def line underlined? I don't think this is as straightforward as you think it is

@trkoch

This comment has been minimized.

Copy link
Author

commented Apr 14, 2019

Yes. Interestingly with both current behavior and proposed changes.

{
  "metadata": {
    "rubocop_version": "0.65.0",
    "ruby_engine": "ruby",
    "ruby_version": "2.6.2",
    "ruby_patchlevel": "47",
    "ruby_platform": "x86_64-darwin18"
  },
  "files": [
    {
      "path": "some_controller.rb",
      "offenses": [
        {
          "severity": "warning",
          "message": "Lint/UselessAssignment: Useless assignment to variable - `unused`.",
          "cop_name": "Lint/UselessAssignment",
          "corrected": false,
          "location": {
            "start_line": 9,
            "start_column": 5,
            "last_line": 9,
            "last_column": 10,
            "length": 6,
            "line": 9,
            "column": 5
          }
        },
        {
          "severity": "convention",
          "message": "Style/EmptyMethod: Put empty method definitions on a single line.",
          "cop_name": "Style/EmptyMethod",
          "corrected": false,
          "location": {
            "start_line": 12,
            "start_column": 3,
            "last_line": 13,
            "last_column": 5,
            "length": 13,
            "line": 12,
            "column": 3
          }
        }
      ]
    }
  ],
  "summary": {
    "offense_count": 2,
    "target_file_count": 1,
    "inspected_file_count": 1
  }
}

Current behavior

image

Proposed changes

image

I agree it's probably not as straightforward as I think. Thanks for taking a closer look!

@trkoch

This comment has been minimized.

Copy link
Author

commented Apr 14, 2019

Attempting to avoid any misconceptions, main issue being here that VSCode does not detect some errors at all (apparently those with range.end.character = -1).

@wingrunr21

This comment has been minimized.

Copy link
Collaborator

commented Apr 14, 2019

I think there's two things that need fixed here:

  1. Things with length 0 need to have their start/ends set to the error row but a column of either start or end. For the Parser error, I'm not sure what's going to make more sense to display that on the line. It may be that we need to actually set that as the whole line
  2. Sometimes the RuboCop error does not encompass the whole warning (I just reproduced this as well). That seems to indicate the translation between what RuboCop outputs and the VSCode file mapping is inconsistent.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.