tools: non-Ascii linter for /lib only

[SirR4T]

Non-ASCII characters in /lib get compiled into the node binary,
and may bloat the binary size unnecessarily. A linter rule may
help prevent this.

Fixes: #11209

Checklist

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

Affected core subsystem(s)

eslint

[addaleax]

I would make it clear somewhere in a comment that this only catches characters in literals, not the source code itself (which is what we would eventually want to have instead)

[SirR4T]

oh? then @hkal 's approach of parsing all tokens would be preferred? I had assumed the eslint selector would select the source code too, i see where i might have gone wrong 😅

Also, doubts:

• the regexp /[^\n\x20-\x7e]/ doesn't capture \r, which I have seen some instances of, while running the linter. I should be adding that in the regexp, right?
• I have seen instances of \u0000 and \u000c etc. in code. I should be disabling this rule for these, right?

[addaleax]

@SirR4T I’m not sure how to do it, but yes, you want all tokens and even comments.

I have seen instances of \u0000 and \u000c etc. in code. I should be disabling this rule for these, right?

It’s fine to have the escape sequences spelled out as escape sequences in the source code. What matters is whether the files themselves contain characters that don’t fit into the ASCII range, because that’s what the script that bakes those files into the node binary looks for.

I hope that’s helpful. :)

[SirR4T]

now I'm running into a very peculiar error, while doing make lint-js:

I'm unable to disable the below error, via the regular methods (//eslint-disable-line, //eslint-disable-next-line, etc.)

$ make lint-js
Running JS linter...
/Users/sarat/Play/Github/node/lib/internal/test/unicode.js
1:1 error Non-ASCII character '✓' detected non-ascii-character
✖ 1 problem (1 error, 0 warnings)
1 error, 0 warnings potentially fixable with the `--fix` option.
make: *** [lint-js] Error 1

[SirR4T]

@addaleax : review latest commit? This was the approach followed in the (now closed) pull #11371, but I think that this is catching much lesser violations.

[not-an-aardvark]

I'm a bit confused about why non-ascii characters would bloat the binary size. Aren't the files encoded as UTF-8?

[SirR4T]

@not-an-aardvark that is what I understood, from #11129 (comment) :

I would have to assume it is because the external string API does not directly support UTF8. That is, if you look at ExternalStringResource, it assumes a utf16_t* buffer while ExternalOneByteStringResource assumes const char*. If the strings were stored as UTF8 we would incur an additional cost at startup that is not necessary.

Is that not correct?

[addaleax]

@SirR4T Yes, that is correct. Files that don’t fit into ASCII are currently saved as UTF-16 instead:

node/tools/js2c.py

Lines 230 to 240 in 1e0f331

	defRender(var, data):
	# Treat non-ASCII as UTF-8 and convert it to UTF-16.
	ifany(ord(c) >127forcindata):
	template=TWO_BYTE_STRING
	data=map(ord, data.decode('utf-8').encode('utf-16be'))
	data= [data[i] *256+data[i+1] foriinxrange(0, len(data), 2)]
	data=ToCArray(data)
	else:
	template=ONE_BYTE_STRING
	data=ToCString(data)
	returntemplate.format(var=var, data=data)

[addaleax]

I mean, this still looks fine to me, but I’m not an expert for eslint anyway.

If it doesn’t catch the diagram in lib/timers.js and the check mark in lib/internal/test/unicode.js (for both of which we’ll want eslint-disable comments), it’s probably not quite strict enough, though.

[Trott]

Any reason not to use a RegExp literal like this?:

constnonAsciiRegexPattern=/[^\r\n\x20-\x7e]/;

Seems more readable to me and also more in line with our general coding style. (I'm kind of surprised this isn't caught by a lint rule itself, to be honest. Or maybe it is?)

[not-an-aardvark]

It might be better to match on source.text rather than each individual token, to ensure that non-ascii whitespace is detected (which would not be part of any token or comment).

[SirR4T]

Yay! 🎉

Could catch the lib/timers.js too, using source.text. But am still unable to bypass / skip the rule, for it ☹️

Any ideas? @Trott@not-an-aardvark

[SirR4T]

cc: @Trott@not-an-aardvark@addaleax

can someone help me out here? I'm unable to skip the lint checks, inside lib/ folder. Need to skip the rules for lib/timers.js, and the check mark in lib/internal/test/unicode.js.

[not-an-aardvark]

I think there are a few issues:

• You're using a comment like // eslint-disable non-ascii-character, but I think it should be a block comment (/* eslint-disable non-ascii-character */)
• When the rule reports a problem, the location of the problem is currently always the top of the file, because you're passing the Program node to context.report. Instead, it would be better if the report location were the point in the file that contains the non-ascii character. To do this, you could find the index of the non-ascii character in sourceCode.text, and then pass { ... loc: sourceCode.getLocFromIndex(theIndex) } to context.report.

If you make both of those changes, I think the disable comments will work as expected.

[SirR4T]

Thanks @not-an-aardvark ! worked like a charm! pushing the latest changes, once the build is done.

[BridgeAR]

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

@not-an-aardvark PTAL

[addaleax]

Landed in c45afe8

[MylesBorins]

Should this be backported to v8.x-staging or v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.