1231509 - Using more than just an expression in XBL fields confuses the eslint xbl preprocessor

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[:Gijs (he/him)]

gkruitbosch-16516:fx-team gkruitbosch$ ./mach eslint  browser/base/content/
 0:00.21 Running /usr/local/bin/eslint
 0:00.21 /usr/local/bin/eslint --ext [.js,.jsm,.jsx,.xml] browser/base/content/

/Users/gkruitbosch/dev/fx-team/browser/base/content/socialchat.xml
  395:14  error  Parsing error: Unexpected identifier
  407:14  error  Parsing error: Unexpected identifier
  420:14  error  Parsing error: Unexpected identifier

/Users/gkruitbosch/dev/fx-team/browser/base/content/tabbrowser.xml
  4423:16  error  Parsing error: Unexpected token -

/Users/gkruitbosch/dev/fx-team/browser/base/content/urlbarBindings.xml
  1600:16  error  Parsing error: Unexpected identifier
  1855:10  error  Parsing error: Unexpected token let
  2684:16  error  Parsing error: Unexpected identifier

✖ 7 problems (7 errors, 0 warnings)

Comment 1

[:Gijs (he/him)]

(filing this under testing because I'm assuming that the processor is still generating slightly busted things because this many syntax errors on tip seems unlikely - though I guess it's possible!)

Comment 2

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

Some of the files no longer exist and this error does not seem to surface anymore just running mach eslint browser/base/content. On the other hand, I am getting errors that are very similar when fixing bug 1280883 and I have an idea where these issues might be from.

urlbarBindings.xml, autocomplete.xml, preferences.xml and tabbox.xml use XBL fields that don't just contain an expression but a whole code snippet. To fix this bug I'd like to change those fields to properties that calculate themselves once, similar to XPCOMUtils.generateLazyGetter().

I'm moving this out of ESLint since the fix is actually in toolkit and browser. The patch is ready, I just need to do a try run.

Comment 3

[:Gijs (he/him)]

(In reply to Philipp Kewisch [:Fallen] from comment #2)
> Some of the files no longer exist and this error does not seem to surface
> anymore just running mach eslint browser/base/content. On the other hand, I
> am getting errors that are very similar when fixing bug 1280883 and I have
> an idea where these issues might be from.
> 
> urlbarBindings.xml, autocomplete.xml, preferences.xml and tabbox.xml use XBL
> fields that don't just contain an expression but a whole code snippet. To
> fix this bug I'd like to change those fields to properties that calculate
> themselves once, similar to XPCOMUtils.generateLazyGetter().
> 
> I'm moving this out of ESLint since the fix is actually in toolkit and
> browser. The patch is ready, I just need to do a try run.

This doesn't seem like the right fix. If this works in XBL, shouldn't we just fix the mozilla-provided/written XBL-to-js preprocessor that we're using with eslint?

Comment 4

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

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Philipp Kewisch [:Fallen] from comment #2)
> > urlbarBindings.xml, autocomplete.xml, preferences.xml and tabbox.xml use XBL
> > fields that don't just contain an expression but a whole code snippet. To
> > fix this bug I'd like to change those fields to properties that calculate
> > themselves once, similar to XPCOMUtils.generateLazyGetter().
> This doesn't seem like the right fix. If this works in XBL, shouldn't we
> just fix the mozilla-provided/written XBL-to-js preprocessor that we're
> using with eslint?

I don't think the extra logic in the xbl preprocessor needed to handle both cases is worth the effort. We'd need to check the textContent to see if it is a single expression or not, extract the last single expression if not, then wrap that into a function that returns the last expression. All of this needs to be done with string functions only, or you need to run a separate copy of espree on each field's textContent, then extract the right nodes that way.

This is much more brittle and performance intensive than just rewriting those few fields to self-replacing properties. I'm open to other ideas though.

Comment 5

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

Attachment: Bug 1231509 - Using more than just an expression in XBL fields confuses the eslint xbl preprocessor.

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

Comment 6

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfe8935421b3
(also contains some other patches related to what I am working on, let me know if you prefer a separate try run)

Comment 7

[Dave Townsend [:mossop]]

(In reply to Philipp Kewisch [:Fallen] from comment #4)
> (In reply to :Gijs Kruitbosch from comment #3)
> > (In reply to Philipp Kewisch [:Fallen] from comment #2)
> > > urlbarBindings.xml, autocomplete.xml, preferences.xml and tabbox.xml use XBL
> > > fields that don't just contain an expression but a whole code snippet. To
> > > fix this bug I'd like to change those fields to properties that calculate
> > > themselves once, similar to XPCOMUtils.generateLazyGetter().
> > This doesn't seem like the right fix. If this works in XBL, shouldn't we
> > just fix the mozilla-provided/written XBL-to-js preprocessor that we're
> > using with eslint?
> 
> I don't think the extra logic in the xbl preprocessor needed to handle both
> cases is worth the effort. We'd need to check the textContent to see if it
> is a single expression or not, extract the last single expression if not,
> then wrap that into a function that returns the last expression. All of this
> needs to be done with string functions only, or you need to run a separate
> copy of espree on each field's textContent, then extract the right nodes
> that way.

I don't think it is actually that complicated. Eslint doesn't actually care what variables are set to. In fact I don't think we even do anything clever that relies on field names being defined as set to something anyway. We could probably just drop that logic from the transformer and instead just convert every field into something that looks like a function, same as we do for getter properties.

That still might be more complex than just fixing the few edge cases we have here. Thoughts?

Comment 8

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

Right now it is converted into a function that returns the value, i.e.

<field name="foo">123123</field>

gets converted to:

...
foo: function() {
  return
  123123
},
...

and to fix bug 1280883 soon:

...
foo: function() {
  return (
    123123
  );
},
...


If we just drop the return statement and have it be a function, then we will have trouble with http://eslint.org/docs/rules/no-unused-expressions due to the last line in affected fields not actually doing anything.

Comment 9

[Dave Townsend [:mossop]]

(In reply to Philipp Kewisch [:Fallen] from comment #8)
> If we just drop the return statement and have it be a function, then we will
> have trouble with http://eslint.org/docs/rules/no-unused-expressions due to
> the last line in affected fields not actually doing anything.

Ah yeah. I guess we could disable that rule inside fields but that is getting sorta silly at that point. If only XBL was sane. Ok let's go with the simple fix for now and see if it bites us.

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8792657 [details]
Bug 1231509 - Using more than just an expression in XBL fields confuses the eslint xbl preprocessor.

https://reviewboard.mozilla.org/r/79576/#review78542

I slightly lean towards using XPCOMUtils.defineLazyGetter in the <constructor> rather than the property approach but not enough to deman you change it all.

Comment 11

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

Thanks. Doing so would mean importing XPCOMUtils in the constructor too, not sure if you'd want this as a global import or into a separate scope. I'm going to go ahead and land this patch as is now, if required I or someone else can change it in the future.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9fb7792946fb
Using more than just an expression in XBL fields confuses the eslint xbl preprocessor. r=mossop

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9fb7792946fb