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

Fix range bug with no-extra-boolean-cast #11324

Assignees
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

Comments

@aboyton
Copy link

aboyton commented Jan 28, 2019

Tell us about your environment

  • ESLint Version: 5.12.1
  • Node Version: 8.11.4
  • npm Version: 5.6.0

What parser (default, Babel-ESLint, etc.) are you using? default

Please show your full configuration:

Configuration
{
  "extends": ["eslint:recommended"],
  "rules": {
    "no-extra-boolean-cast": "error"
  }
}

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

var bar = 1;
var foo = !!bar ? 'true' : 'false';
npm run lint -- --format=json

What did you expect to happen?
The information about the location of the fix appears to be off. Running --fix applies the fix correctly but the response from the JSON output is off (which means that other external tools cannot apply the fix).

I would expect the location information to be something like:

{
  "line": 2,
  "column": 11,
  "nodeType": "UnaryExpression",
  "messageId": "unexpectedNegation",
  "endLine": 2,
  "endColumn": 16,
  "fix": { "range": [24, 29], "text": "bar" }
}

Looking at AST Explorer seems to have the right location information.

https://astexplorer.net/#/gist/b5517bff697af4446176699497517e36/8f1a35f7594886e63d855960a1649120071963d4
What actually happened? Please include the actual, raw output from ESLint.

[
  {
    "filePath": "/Volumes/Freelancer/scratch/eslint/test.js",
    "messages": [
      {
        "ruleId": "no-unused-vars",
        "severity": 2,
        "message": "'foo' is assigned a value but never used.",
        "line": 2,
        "column": 5,
        "nodeType": "Identifier",
        "endLine": 2,
        "endColumn": 8
      },
      {
        "ruleId": "no-extra-boolean-cast",
        "severity": 2,
        "message": "Redundant double negation.",
        "line": 2,
        "column": 12,
        "nodeType": "UnaryExpression",
        "messageId": "unexpectedNegation",
        "endLine": 2,
        "endColumn": 16,
        "fix": { "range": [23, 28], "text": "bar" }
      }
    ],
    "errorCount": 2,
    "warningCount": 0,
    "fixableErrorCount": 1,
    "fixableWarningCount": 0,
    "source": "var bar = 1;\nvar foo = !!bar ? 'true' : 'false';\n"
  }
]

Are you willing to submit a pull request to fix this bug? With help potentially. I'm unfamiliar with how rules output the position information.

@aboyton aboyton added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Jan 28, 2019
@not-an-aardvark
Copy link
Member

Does the source file have a byte-order-mark at the start? Fix ranges from ESLint don't include a BOM when calculating positions, so if you're piping results from ESLint into an external tool to apply fixes, you might need to remove the BOM from the source text yourself before passing it to the external tool.

@aboyton
Copy link
Author

aboyton commented Feb 15, 2019

I don't have a byte order mark and other offsets work fine for me.

@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Mar 18, 2019
@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@platinumazure platinumazure added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed auto closed The bot closed this issue triage An ESLint team member will look at this issue soon labels Mar 18, 2019
@platinumazure platinumazure self-assigned this Mar 18, 2019
@platinumazure
Copy link
Member

Reopening as this slipped through the cracks. We at least need to try to reproduce this before closing.

@platinumazure platinumazure reopened this Mar 18, 2019
@mdjermanovic
Copy link
Member

Source:

"var bar = 1;\nvar foo = !!bar ? 'true' : 'false';\n"

Error:

{
    "ruleId": "no-extra-boolean-cast",
    "severity": 2,
    "message": "Redundant double negation.",
    "line": 2,
    "column": 12,
    "nodeType": "UnaryExpression",
    "messageId": "unexpectedNegation",
    "endLine": 2,
    "endColumn": 16,
     "fix": { "range": [23, 28], "text": "bar" }
}

I think that:

  • "column": 12 is a bug, it should be 11. The rule is reporting !bar node instead of !!bar node.
  • "range": [23, 28] is correct, range is 0-based and \n counts as only 1.

For comparison, this is no-implicit-coercion error on the same code:

{
    "ruleId": "no-implicit-coercion",
    "severity": 2,
    "message": "use `Boolean(bar)` instead.",
    "line": 2,
    "column": 11,
    "nodeType": "UnaryExpression",
    "endLine": 2,
    "endColumn": 16,
    "fix": { "range": [23, 28], "text": "Boolean(bar)" }
}

@platinumazure
Copy link
Member

@mdjermanovic Nice work. I think we can accept this issue based on that investigation.

...This raises an interesting thought. Should the autofix logic allow a fix range outside of the range of the reported node?

@platinumazure platinumazure added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Aug 31, 2019
@mdjermanovic
Copy link
Member

I'll fix this soon.

...This raises an interesting thought. Should the autofix logic allow a fix range outside of the range of the reported node?

Could be very interesting to test this, perhaps it would find similar bugs in other rules.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.