lib,test: remove unneeded escaping of /

[Trott]

Checklist

• make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

lib test

Description of change

The / character does not need to be escaped when occurring inside a
character class in a regular expression. Remove such instances of
escaping in the code base.

/cc @silverwind

[princejwesley]

@Trott We can write lint rule to auto fix it, right?

[mscdex]

Commit message either has a missing backtick before / or an unnecessary backtick after /.

[not-an-aardvark]

@princejwesley This is actually enforced by ESLint's no-useless-escape rule. We have this rule enabled on the repo, but before ESLint v3.9.1, the rule had a false negative where it wasn't checking character classes (see eslint/eslint#7425).

The no-useless-escape rule is intentionally not autofixable in ESLint itself, because useless escapes are often due to a programmer error where the code isn't doing what it's supposed to. Of course, Node could create a custom autofixable version of the rule if we wanted to.

[princejwesley]

@not-an-aardvark no-useless-escape rule is too generic. We can target only for regex character classes where the intention is clear. what do you say?

[Trott]

Commit message either has a missing backtick before / or an unnecessary backtick after /.

Fixed, thanks!

[not-an-aardvark]

@princejwesley I don't understand the question.

[princejwesley]

@not-an-aardvark We can write custom rule just for regular expression character classes. no-useless-escape rule is for any (seems) superfluous escapes. In our case, programmer's intention is clear and its a subset of no-useless-escape.

[not-an-aardvark]

Oh, I see. I suppose we could do that, but based on #9449 (comment) I'm not sure whether we want to do it or whether it would be worth it.

[princejwesley]

@Trott Instead of addressing this particular case manually, we can have a rule that will fix automatically. I'll write it if its fine for you 😄

[Trott]

@Trott Instead of addressing this particular case manually, we can have a rule that will fix automatically. I'll write it if its fine for you 😄

I do not want to discourage you from trying! I do suspect some of the resulting changes might be controversial because they reduce readability.

For example, here's the current line 549 of tools/doc/json.js:

var braceExpr = /^(?:property:?\s*)?[^\.\[]+(\[[^\]]+\])\s*?$/i;

Let's break out one of the problematic character classes:

[^\.\[]

Removing the unnecessary escaping of the . seems obvious enough:

[^.\[]

Removing the unnecessary escaping of the [ character kind of makes it a bit confusing:

[^.[]

You can make the case that the above is not all that confusing, but it gets harder to make that case when you restore the character class to its original context, and even more so when you also remove another extra unnecessary escape in the line for a ] character near the end of the regular expression:

var braceExpr = /^(?:property:?\s*)?[^.[]+(\[[^\]]+])\s*?$/i;

The original is a confusing regular expression to try to read, and the explicit (but unnecessary) escaping makes it easier to understand. Removing the unnecessary escaping makes the line actually more difficult to understand.

It's possible that this bit of code could benefit from more refactoring such that the unnecessary escapes are removed and the regular expression is easier to understand.

So given all that, if you want to try to do an auto-fix and see what it looks like, that sounds good to me! Even if they don't all get landed, it may be good to fix as many as we can at one time.

[not-an-aardvark]

@princejwesley If you're going to add an autofixer, I would recommend using this as a starting point. (The rule has changed a bit since that PR, but I think that basically covers the modification that would need to be made to allow autofixing.)

@Trott When we made the bugfix to no-useless-escape, we originally considered adding an option to ignore useless escapes in character classes, but we ended up dropping the idea for now and waiting to see if anyone had readability concerns. If you think regular expressions like these are more readable with the extra escaping, then there are probably other people who agree with you, so feel free to make a proposal in ESLint if you think the rule would be better with an option like that.

[princejwesley]

@Trott we can have configurable option to ignore unnecessary escape for characters like [, ]. Just a thought! let me give it a try, lets see whether it helps or hurts 😄

[Trott]

@princejwesley: Sounds great! Thanks in advance! Looking forward to what you come up with!

@not-an-aardvark: Thanks for the context and additional information!

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/4802/

[Trott]

Landed in 7ef16ee