Infer the endLocation of a problem from the reported node

[not-an-aardvark]

Problem

Currently, the context.report API allows rules to specify a start and end location for problems:

context.report({
  loc: {
    start: {
      line: 1,
      column: 3
    },
    end: {
      line: 1,
      column: 5
    }
  },
  message: 'Detected a problem.'
});

This was added to address #3307, and it's very useful because it allows editor integrations to highlight an entire range.

However, as currently implemented this API is tedious to use, because it only works if a report includes aloc key. As a result, most rules don't use this API, because they report a node instead of an explicit location. (When a node is reported, only the start location of the node appears in the resulting message -- the end location does not appear.)

Proposal

When a node is reported without a location, the end location of the node should be used as the end location of the report. (The start location of the node is already used for the start location of the report.)

For example, given the following node:

{
  "type": "Identifier",
  "start": 0,
  "end": 3,
  "name": "foo"
}

Suppose a rule uses

context.report({ node, message: 'report message' });

The resulting problem message currently looks like this:

{
  "ruleId": "some-rule",
  "severity": 2,
  "message": "report message",
  "line": 1,
  "column": 1,
  "nodeType": "Identifier",
  "source": "foo"
}

With this change, the resulting problem message would look like this:

{
  "ruleId": "some-rule",
  "severity": 2,
  "message": "report message",
  "line": 1,
  "column": 1,
+ "endLine": 1,
+ "endColumn": 4,
  "nodeType": "Identifier",
  "source": "foo"
}

[ilyavolodin]

I was under impression that we are already doing this. But if we are not, then I definitely support this.

[mysticatea]

I avoided it: #6640

Because some rules in core and plugin ecosystem have sometimes used big node (Program, FunctionDeclaration, Property, .*Statement, ...). I'm afraid that it fills editors with red.

[platinumazure]

Maybe we could implement it and then try to fix rules using large nodes as reports come in?

[not-an-aardvark]

Personally I think the issue with filling a screen is an editor integration problem, not an ESLint problem. I'm fine with waiting until a major release, but if we do that then we might also want to give some warning so that editors can display the best possible UI.

[mysticatea]

For example:

return {
    Program(node) {
        context.report({ node, message: 'report message' });
    }
}

In this case, currently the report is at the head of the source code, but the report spans whole the source code after this change. It's a breaking change that plugin owners need to modify their plugins.

[mysticatea]

I don't oppose this if it happens on the next major release.

[not-an-aardvark]

Since it looks like we're probably going ahead with this change, we should probably notify editor integrations about this in case they want to change their behavior ahead of time. I pulled the list from the integrations page:

• linter-eslint
• vscode-eslint
• SublimeLinter-eslint
• eslint-plugin (IntelliJ)
• TextMate 2 bundle
• tern-eslint
• Orion/ESLint (not sure where the issue tracker is)
• Flycheck (has an eslint setting)
• Vim

I haven't used most of these integrations, so I'm not sure how many of them are maintained. Also, there are probably some of integrations that wouldn't be affected by this change anyway.

[mysticatea]

At least, vscode-eslint and atom-linter are using endLine and endColumn to show reports.

I think the announcement is needed for plugin's authors rather than editor integrations because this is a breaking change of RuleContext API and the form of reports are not changed. This change does not affect to editors, but affects to plugins.

[kaicataldo]

To add to the list of Vim editor integrations, neomake is also quite popular alongside Syntastic.

[not-an-aardvark]

I'll add this to the TSC agenda since it's a change to core that could break things.

TSC Summary: As currently implemented, the endLine and endColumn report API is tedious to use, because it only works if a report includes a loc key. As a result, most rules don't use this API, because they report a node instead of an explicit location. We could update core to infer the endLine and endColumn of a reported node, but this would be a breaking change because it could cause a poor user experience in some existing editor integrations (e.g. if hundreds of lines are highlighted because a large node is reported).

TSC Question: Should we make this change?

[not-an-aardvark]

In today's TSC meeting, the TSC accepted this issue.