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

Escape characters are handled incorrectly in inline config comments #9366

Closed
dead-claudia opened this issue Sep 28, 2017 · 16 comments · Fixed by #13140
Closed

Escape characters are handled incorrectly in inline config comments #9366

dead-claudia opened this issue Sep 28, 2017 · 16 comments · Fixed by #13140
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 core Relates to ESLint's core APIs and features

Comments

@dead-claudia
Copy link

dead-claudia commented Sep 28, 2017

Tell us about your environment

  • ESLint Version: v4.7.2
  • Node Version: v8.5.0
  • npm Version: Yarn v1.0.2

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

Please show your full configuration:

Configuration
rules:
    id-match: [2, "^(([^$\\W]|\\$[a-f\\d]{2})+|[$_]\\w*|[^\\W\\d]\\w*|[A-Z]([A-Z_]*[A-Z])?)$", {
        properties: true
    }]

What did you do? Please include the actual source code causing the issue.

/* eslint id-match: [2, "^(([^$\\W]|\\$[a-f\\d]{2})+|[$_]\\w*|[^\\W\\d]\\w*|[A-Z]([A-Z_]*[A-Z])?)$", {properties: true}] */

function is$2dvoid(value) {
    return value == null
}

What did you expect to happen? It to pass (as it does without the comment)

What actually happened? Please include the actual, raw output from ESLint.

/path/to/file.js
  3:10  error  Identifier 'is$2dvoid' does not match the pattern '^(([^$\\W]|\\$[a-f\\d]{2})+|[$_]\\w*|[^\\W\\d]\\w*|[A-Z]([A-Z_]*[A-Z])?)$'  id-match

✖ 1 problem (1 error, 0 warnings)
@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Sep 28, 2017
@not-an-aardvark
Copy link
Member

not-an-aardvark commented Sep 28, 2017

I think this is happening because backslash escapes are handled differently in config comments. I noticed this in #9309 but didn't end up having time to investigate.

@not-an-aardvark not-an-aardvark added bug ESLint is working incorrectly core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Sep 28, 2017
@dead-claudia
Copy link
Author

If this helps, here's another repro, in the web interface.

@dead-claudia
Copy link
Author

Oh, and this actually works as intended.

(Note: those two repros use a simpler regexp, but it works to the same effect.)

@aladdin-add
Copy link
Member

aladdin-add commented Sep 29, 2017

hmm..interesting! seems levn will replace "\\'" to "\\\\'". (we are using it to parse comment config)
https://github.com/gkz/levn/blob/a92b9acf928282ba81134b4ae8e6a5f29e1f5e1e/lib/parse-string.js#L103

I guess it's because there's no need to escape in source code.
to fix this, I just replace "\\" to "\" in comment config options. how can we add tests for it?

aladdin-add pushed a commit to aladdin-add/eslint that referenced this issue Sep 30, 2017
(fixes eslint#9366) levn parsing string differently than JSON.parse,
e.g.
JSON.parse(          '{ "foo": "\\n" }') // => { foo: '\n' }
levn.parse('Object', '{ "foo": "\\n" }') // => { foo: '\\n' }
@platinumazure
Copy link
Member

platinumazure commented Sep 30, 2017

I was the one who suggested using levn for inline comment handling, reasoning that it would be nice to have the same syntax in inline comments as on the command line (for options like --rule). I didn't realize that it would be incompatible with JavaScript/JSON string literals (including as used in config files) in some cases. I could see an argument for preferring to align with config files rather than CLI arguments, if we had to choose.

@eslint/eslint-team Should we consider removing levn from inline comment handling, given this issue? Or can we just update the inline comment docs to point to levn? Maybe we could reasonably fix this in levn?

@not-an-aardvark
Copy link
Member

Should we consider removing levn from inline comment handling, given this issue?

To clarify, do you mean that we would keep levn's semantics (aside from this case) without using the library? Or do you mean that we would require inline comments to actually be JSON?

I would be fine with the former (provided it isn't too much work), but I don't think we should do the latter -- it would be a big breaking change, and I like not needing to quote everything in inline comments.

Or can we just update the inline comment docs to point to levn?

This doesn't seem ideal to me -- I think it's unexpected that a comment which appears to be JSON would not get parsed as JSON.

Maybe we could reasonably fix this in levn?

👍, although it seems unmaintained.

@aladdin-add
Copy link
Member

aladdin-add commented Oct 2, 2017

const code = "/* eslint max-len: [2, 100, 2, {ignoreUrls: true, ignorePattern: \"data:image\\/|\\s*require\\s*\\(|^\\s*loader\\.lazy|-\\*-\"}] */\nalert('test');";

so this test should be failing -- not a valid regex in JSON, it passed just because of the levn issue?

@dead-claudia
Copy link
Author

Yes, that is the correct understanding.

@not-an-aardvark not-an-aardvark 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 Nov 3, 2017
@not-an-aardvark not-an-aardvark changed the title id-match mysteriously fails when defined in a comment Escape characters are handled incorrectly in inline config comments Nov 3, 2017
@platinumazure
Copy link
Member

I found another example of this while trying to implement #10197. In the max-len docs, we specify a rule config inline and it crashes ESLint when trying to initialize the rule:

/*eslint max-len: ["error", { "ignorePattern": "^\\s*var\\s.+=\\s*require\\s*\\(" }]*/

I'll try to write a report in levn in the next few days.

@platinumazure platinumazure self-assigned this Oct 13, 2018
@platinumazure
Copy link
Member

I've opened gkz/levn#2 to try to get some discussion going. (That said, there hasn't been any activity on levn in nearly three years, so I might need to take it a step further and write a PR.)

@kaicataldo
Copy link
Member

How do we want to proceed with this? It unfortunately doesn't look like there has been any movement on the levn issue. It seems like moving away from levn might be the prudent thing to do.

@kaicataldo
Copy link
Member

Thoughts on using the JSON5 parser for this?

@kaicataldo
Copy link
Member

I think the only big difference is that string values must be quoted (though it can be single or double quotes).

@not-an-aardvark
Copy link
Member

It seems like that might be a significant breaking change, since it would make /* eslint foo-rule: error */ fail to parse and require /* eslint foo-rule: "error" */ instead.

Would it work to parse the strings as YAML instead? I know YAML is a superset of JSON and allows unquoted strings, although I'm not sure how it handles some edge cases.

@kaicataldo
Copy link
Member

That's correct, parsing as JSON5 would be a breaking change. I made a quick PoC to help evaluate this.

Would it work to parse the strings as YAML instead? I know YAML is a superset of JSON and allows unquoted strings, although I'm not sure how it handles some edge cases.

That's an interesting thought! Seems worth exploring.

@platinumazure platinumazure removed their assignment Apr 4, 2020
@platinumazure
Copy link
Member

The levn project has just issued version 0.4.1 which allows quoted string literals to use the same escaping rules as JavaScript itself, so upgrading to latest levn may resolve this issue.

kaicataldo added a commit that referenced this issue Apr 4, 2020
kaicataldo added a commit that referenced this issue Apr 4, 2020
kaicataldo added a commit that referenced this issue Apr 7, 2020
* Upgrade: levn@0.4.1 (fixes #9366)

* Use String.raw to improve readability of tests

* Address feedback

* Update optionator@0.9.1
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Oct 5, 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 Oct 5, 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 bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
None yet
6 participants