Closed Bug 1231509 Opened 4 years ago Closed 3 years ago

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

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox45 --- affected
firefox52 --- fixed

People

(Reporter: Gijs, Assigned: Fallen)

References

Details

Attachments

(1 file)

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)
(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!)
Component: General → ESLint
Assignee: nobody → philipp
Status: NEW → ASSIGNED
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.
Component: Lint → XUL Widgets
Product: Testing → Toolkit
Summary: Errors trying to use eslint against XBL files in browser/base/content/ → Using more than just an expression in XBL fields confuses the eslint xbl preprocessor
(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?
No longer blocks: 1280883
Blocks: 1280883
(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.
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)
(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?
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.
(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 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.
Attachment #8792657 - Flags: review?(dtownsend) → review+
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.
Pushed by mozilla@kewis.ch:
https://hg.mozilla.org/integration/autoland/rev/9fb7792946fb
Using more than just an expression in XBL fields confuses the eslint xbl preprocessor. r=mossop
https://hg.mozilla.org/mozilla-central/rev/9fb7792946fb
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1323600
You need to log in before you can comment on or make changes to this bug.