1280883 - ESLint shows unreachable statement for xbl fields

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[Philipp Kewisch [:Fallen][📱📆]]

If you use the following in xbl:

<field name="mArray">[]</field>

The preprocessor generates this:

     get mArray() {
        return
        []
      },

Which causes an error for an unreachable statement. I proposed changing the generated js to:

     get mAlarm() {
        return (
          []
        );
      },

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v1

Comment 2

[Andrew Halberstadt [:ahal]]

Comment on attachment 8763507 [details] [diff] [review]
Fix - v1

Review of attachment 8763507 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not qualified to review this. I'd suggest either :Mossop or :miker, though both appear to be on PTO atm unfortunately.

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8763507 [details] [diff] [review]
Fix - v1

Review of attachment 8763507 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the note, I assumed so from your name appearing in hg log, but I guess you also just submitted a patch :) Deferring to :miker for when he is back.

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: [backed out] Fix - v2

Patch with updated commit message for checkin.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/b82022d2347a
ESLint shows unreachable statement for xbl fields. r=miker

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

IIRC you'll also need to:

1. Bump the version number[1] of the ESLint plugin
2. Update the version the mach command checks for to match (so local users know to update)
3. Update the ESLint archive in tooltool using the update script[3] so automation uses the new code

[1]: https://dxr.mozilla.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/package.json
[2]: https://dxr.mozilla.org/mozilla-central/source/tools/lint/mach_commands.py#36
[3]: https://dxr.mozilla.org/mozilla-central/source/tools/lint/eslint/update

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Thanks for the note! I'll take care of that once I have all calendar rules enabled, in case I find more issues.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b82022d2347a

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Unfortunately, this change appears to cause errors of its own:

TEST-UNEXPECTED-ERROR | browser/base/content/socialchat.xml:98:23 | Parsing error: Unexpected token ; (null)
TEST-UNEXPECTED-ERROR | browser/base/content/tabbrowser.xml:43:66 | Parsing error: Unexpected token ; (null)
TEST-UNEXPECTED-ERROR | browser/base/content/urlbarBindings.xml:57:93 | Parsing error: Unexpected token ; (null)

I guess we didn't realize until now since it was never packaged for automation.  I believe we should now require the steps I mentioned in comment 6 as part of any change to the Mozilla plugin to avoid this.  A commit hook for Mozilla plugin changes would also help.

Backout:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd4fd5d4436f

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

Ah I see, sorry about that. All usages in comm-central/calendar actually omit the final ; so I didn't notice. I will run it against m-c next time. I think it would be impractical to bump the version on each patch, but on the other hand since changes to this preprocessor are not so often it may be ok.

Now to fix it correctly, I guess it depends on what we expect the right syntax to be.

1) Accept both with and without semicolon (just strip it in the preprocessor)
2) Force use of semicolon (by adding it to the end of the return statement if it is there, and then relying on the semi rule)
3) Force omitting semicolon (by keeping the patch as is?)

As I have noticed, if a lot of rules are enabled there are inevitably conflicts (indent being the worst). Adding () as this patch did also triggers other rules (I believe it was no-extra-parens). I have another patch pending that adds eslint-disable-line to each generated line so that most issues are not a problem. Therefore I would like to avoid any tricks that are too smart, because there is bound to be a rule that breaks it.

I would prefer either (1) or alternatively (3). I think fields look better without semicolon. What do you think? Here a few examples from code to help the thought process:

<field name="_remote">false</field>
<field name="closingTabsEnum" readonly="true">({ ALL: 0, OTHER: 1, TO_END: 2 });</field>
<field name="mTabBox" readonly="true">
    document.getAnonymousElementByAttribute(this, "anonid", "tabbox");
</field>

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I think 1 is best so stripping it in the preprocessor makes sense.

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Philipp Kewisch [:Fallen] from comment #10)
> Ah I see, sorry about that. All usages in comm-central/calendar actually
> omit the final ; so I didn't notice. I will run it against m-c next time. I
> think it would be impractical to bump the version on each patch, but on the
> other hand since changes to this preprocessor are not so often it may be ok.

Unfortunately for the moment I think we need to bump the version on any changes to the ESLint plugin and go through the automation steps because otherwise we don't know if they actually work everywhere and local users (other than yourself as the patch author) won't be updated to the changes either.

Hopefully the process can be improved, but that's what we have for the moment.

> 1) Accept both with and without semicolon (just strip it in the preprocessor)
> 2) Force use of semicolon (by adding it to the end of the return statement
> if it is there, and then relying on the semi rule)
> 3) Force omitting semicolon (by keeping the patch as is?)

I don't have a strong opinion about the change here as long as it works. :)

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Bug 1280883 - Avoid effects of ASI when preprocessing XBL for eslint.

Review commit: https://reviewboard.mozilla.org/r/84758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84758/

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8799620 [details]
Bug 1280883 - Avoid effects of ASI when preprocessing XBL for eslint.

https://reviewboard.mozilla.org/r/84758/#review85482

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8799620 [details]
Bug 1280883 - Avoid effects of ASI when preprocessing XBL for eslint.

https://reviewboard.mozilla.org/r/84758/#review85484

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8799620 [details]
Bug 1280883 - Avoid effects of ASI when preprocessing XBL for eslint.

https://reviewboard.mozilla.org/r/84758/#review85486

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e5a650aca25
Avoid effects of ASI when preprocessing XBL for eslint. r=miker

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4e5a650aca25