Improve error message when linting fails due to the use of unrecognized JavaScript features

[Rob--W]

Describe the problem and steps to reproduce it:

The addons-linter relies on eslint for validating the source code, and eslint has a very conservative policy to linting. In particular, new JavaScript language features are not supported until they have matured, by reaching stage 4 in the TC39 process. We have had several reports about this in the past, and this is not a new issue. What we can do, however, is to provide a more useful description than a plain "syntax error".

For example, submit an add-on that uses public class field syntax, as shown in #3062

What happened?

An error message complaining about a JavaScript syntax error.

What did you expect to happen?

An error message complaining about a JavaScript syntax error, plus a reference to documentation and/or a mention that some experimental JavaScript features that aren't an official part of the language specification are not supported yet.

[willdurand]

@rpl @Rob--W would something like this work for you?

Before:

❯ ./bin/addons-linter test-ext
Validation Summary:

errors          1
notices         0
warnings        0

ERRORS:

Code              Message                   Description                                                                                 File    Line   Column
JS_SYNTAX_ERROR   JavaScript syntax error   There is a JavaScript syntax error in your code; validation cannot continue on this file.   cs.js   2      5

After:

❯ ./bin/addons-linter test-ext
Validation Summary:

errors          1
notices         0
warnings        0

ERRORS:

Code              Message                                   Description                                                                        File    Line   Column
JS_SYNTAX_ERROR   JavaScript syntax error (Parsing error:   There is a JavaScript syntax error in your code, which might be related to some    cs.js   2      5
                  Unexpected token =)                       experimental JavaScript features that aren't an official part of the language
                                                            specification and therefore not supported yet. The validation cannot continue on
                                                            this file.

[rpl]

@rpl @Rob--W would something like this work for you?

❯ ./bin/addons-linter test-ext
Validation Summary:

errors          1
notices         0
warnings        0

ERRORS:

Code              Message                                   Description                                                                        File    Line   Column
JS_SYNTAX_ERROR   JavaScript syntax error (Parsing error:   There is a JavaScript syntax error in your code, which might be related to some    cs.js   2      5
                  Unexpected token =)                       experimental JavaScript features that aren't an official part of the language
                                                            specification and therefore not supported yet. The validation cannot continue on
                                                            this file.

@willdurand I think that would work if we can be sure that the JavaScript error we are going to show in the message column is going to be the one that does point the developer to the real parsing error.

The code that does try to parse the file as an esm and fallback to a script sourceType that I did mention yesterday is this one:

addons-linter/src/scanners/javascript.js

Lines 222 to 246 in 31da884

	/*
	Analyze the source-code by by parsing the source code manually and
	check for import/export syntax errors.
	This returns `script` or `module`.
	*/
	detectSourceType(filename) {
	// Default options taken from eslint/lib/linter:parse
	const parserOptions = {
	filePath: filename,
	sourceType: 'module',
	ecmaVersion: ECMA_VERSION,
	};
	let sourceType = 'module';
	try {
	const ast = espree.parse(this.code, parserOptions);
	sourceType = this._getSourceType(ast);
	} catch (exc) {
	sourceType = 'script';
	}
	return sourceType;
	}

Unfortunately the fallback mechanism isn't exactly as I did recall, we try to parse it as an esm in that function (as I recalled) but we don't try to parse it again as a sourceType 'script' to avoid parsing the same file 3 times (because eslint will also parse it on its own), and so the implementation strategy may not be exactly like the one that I did mention yesterday and it may not allow us to include just a single parsing error in the message and be absolutely sure that it is going to be the right one.

A tweaked version of that strategy may be to report both the Javascript syntax errors and in the message say something like:

Javascript syntax error (Parsing as ESM error: ...., Parsing as script error: ...) 

generated by collecting the parsing error we got when we tried to parse the file as a 'module' (instead of throwing it away), then if we can detect that eslint did fail to parse the file as a 'script' we generate the error message with both the errors we got.

How that sounds to you?

[willdurand]

Unfortunately the fallback mechanism isn't exactly as I did recall, we try to parse it as an esm in that function (as I recalled) but we don't try to parse it again as a sourceType 'script' to avoid parsing the same file 3 times (because eslint will also parse it on its own), and so the implementation strategy may not be exactly like the one that I did mention yesterday and it may not allow us to include just a single parsing error in the message and be absolutely sure that it is going to be the right one.

mm I am not sure to understand. The message/line/column comes from somewhere when we format the message here:

addons-linter/src/scanners/javascript.js

Line 122 in 31da884

results.forEach((result) => {

My impression is that the message will be correct.

[rpl]

@willdurand let's say that the file is an esm module (and so with import and export statements)

if parsing the file as an sourceType = 'module' did fail in the detectSourceType(filename) method I did link in #3482 (comment), e.g. as in #3062 because the same file is also using a public class field syntax which isn't yet supported, then I guess that the following will happen:

• we will run eslint with sourceType = 'script'
• eslint will fail on the first line that has a syntax invalid with sourceType = 'script', e.g. the first import or export statement
  and in that case the error that we will report in the message would be reporting the first parsing error

This will happen when the syntax that eslint doesn't support is included in a file that has to be parsed as an esm module (but it wouldn't be an issue if the same file doesn't use any syntax only allowed in an esm module).

[willdurand]

We had a short meeting about this issue last Friday: it turns out that knowing why some syntax isn't supported is tricky because of the way we detect the source type (script vs module).

[willdurand]

@rpl what do you think of:

 ./bin/addons-linter test-addon
Validation Summary:

errors          1
notices         0
warnings        0

ERRORS:

Code              Message                                             Description                                                                                        File    Line   Column
JS_SYNTAX_ERROR   JavaScript syntax error (Parsing as ESM error:      There is a JavaScript syntax error in your code, which might be related to some experimental       cs.js   1      1
                  Unexpected token = at line: 4 and column: 5)        JavaScript features that aren't an official part of the language specification and therefore not
                  (Parsing as script error: Parsing error: 'import'   supported yet. The validation cannot continue on this file.
                  and 'export' may appear only with 'sourceType:
                  module' at line: 1 and column: 1)

I have to add lines/columns because the main table will likely contain the wrong values so now each error has its own column/line pair.

[rpl]

I have to add lines/columns because the main table will likely contain the wrong values so now each error has its own column/line pair.

as briefly discussed over zoom, the message doesn't look pretty but at least it does provide all the details needed to understand what was the actual error :-), and so 👍 from my side on proceeding with this approach.

I agree that it make sense to include line and column in the message when we do have two different errors to show for both sourceType module and script.

A potential slightly improvement may be to omit the error we got from sourceType: 'script' parsing if we detect that it is a "... may appear only with 'sourceType' module" kind of error, but I would be ok to do that in a separate follow up if that makes the change easier and/or safer.

[ioanarusiczki]

@willdurand I verified this issue with addons-linter 3.0.0

The new linter message is displayed:
on AMO dev:

locally:

right now the validation fails when using any of the examples from this page https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Public_class_fields#Public_fields