Style guide: ASI

[annevk]

In #5402 @bzbarsky says he cannot review my PR due to JavaScript ASI. Other reviewers have been able to review such patches in the past. Getting different feedback from different reviewers is frustrating. We should put some rules in place.

cc @plehegar @sideshowbarker

[gsnedders]

3757 also touches on this. Myself and @patrickkettner had looked into using eslint and slowly starting to enable more in web-platform-tests to enforce some things, though there's been some pushback from some people at most vendors. Not clear what the right solution is here; even having pre-commit hooks that do automatic reformatting aren't great as they need to be setup per-working tree.

[annevk]

Well, at the very least we could have old-fashioned document that outlines what reviewers are allowed to block on.

[gsnedders]

Well, clicking from the homepage of the docs to reviewing tests gives you a page containing the following: "In general, we tend on the side of merging things with nits (i.e., anything sub-optimal that isn’t absolutely required to be right)".

[annevk]

I'm not sure how that helps with OP.

[gsnedders]

I think it's hard to claim that ASI is "absolutely required to be right" for a test to work?

[bzbarsky]

Just to be clear, my personal position is not "tests that extensively rely on ASI should not merge". It's "I cannot personally review code that is extensively relying on ASI because ASI is pretty complicated and I do not have a good enough understanding of it to even be sure what the code is doing and hence that the test is testing what I think it's testing." If someone else wants to take on the responsibility for reviewing the code and merge it, I'm not going to block it. I do reserve the right to either refuse to edit such files myself or convert them to non-ASI style if I do have to touch them. ;)

I should also note that there is a difference between "There's one semicolon missing somewhere" and the "the entire file is written without any semicolons, except at really unintuitive spots like having to write ;[ stuff ].forEach(func) at global scope". If someone makes the mistake of adding another array before that one and forgets the leading ;, suddenly they're screwed. If they're lucky, they're screwed in a way that causes the test to throw or fail and then can try to take corrective action... Going back to my personal position, I certainly would not have noticed that missing leading ; if it were missing, or even realized it should be there. Because ASI is complicated.

I personally would be pretty happy with eslint stuff, fwiw, but I understand the difficulties that causes with the auto-merging setups...

[jgraham]

FWIW I'm also happy with eslint if we can make it work in a way that vendors can actaully run it on their infrastructure.

[gsnedders]

@patrickkettner did you get around to pushing your branch anywhere, BTW?

[foolip]

It sounds like the rough consensus on ASI is that reviewers can/should ask for more semicolons, or at least that the "minimal semicolons that work" style isn't preferred.

On the linting then, having gone on and on to @lukebjerring and @rwaldron about how you can't safely parse and reserialize HTML and get the same bytes back, I tried https://github.com/BenoitZugmeyer/eslint-plugin-html to see if it works in reality.

I used this .eslintrc.js:

module.exports = {
    "env": {
        "browser": true,
        "es6": true
    },
    "plugins": ["html"],
    "rules": {
        "semi": [
            "error",
            "always"
        ]
    }
};

And then I ran eslint --fix dom/historical.html and it actually worked, semicolons were added in sensible places. I also ran it on webaudio/resources/*.js and the results are sensible.

So... to use eslint is doable, but adds a dependency on nodejs. To fix problems for lints we want to enable also looks feasible, although I still think it would be prudent to compare the before/after byte-by-byte and for this specific case ensure that only 0x3B bytes (semicolons) are added. Like for all lints, we'd still be left with the problem of determining which tests might be deliberately violating some line rule, and how we would know.