842438 - Remove @line support

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

SpiderMonkey supports "//@line" syntax, which is a bit like #line in C.

We should remove this support, for the following reasons.

- The semantics are unclear, especially if a filename is given.  (E.g. see bug 663934.)

- It's only used in the browser for files that go through Preprocessor.py, which isn't is compelling use... *especially* considering that @line support is disabled in the browser(!)

- JS source maps are the new, better way to do this kind of thing.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove @line support from SpiderMonkey.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Don't generate @line comments in Preprocessor.py.

These lines no longer do anything.  glandium, in bug 797322 you enabled these
comments for .jsm files -- was that just for consistency?  Presumably, since
atline support has been disabled in SpiderMonkey for a while.  Also, using
//@line comments in Java code doesn't achieve anything, AFAIK...

Comment 3

[Mike Hommey [:glandium]]

Related: bug 797325

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 715314 [details] [diff] [review]
(part 2) - Don't generate @line comments in Preprocessor.py.

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

On a purely code level, this is r+, but I'm not sure we want to walk that path.

Comment 5

[Nicholas Nethercote [inactive]]

> On a purely code level, this is r+, but I'm not sure we want to walk that
> path.

Can you explain more?

Comment 6

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> > On a purely code level, this is r+, but I'm not sure we want to walk that
> > path.
> 
> Can you explain more?

@line is useful, at the very least for tests.

Comment 7

[Nicholas Nethercote [inactive]]

> @line is useful, at the very least for tests.

It's disabled by default in the browser.  Some of the tests enable it in order to test it, but what's the point of testing a feature that isn't used outside of tests?

Comment 8

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> > @line is useful, at the very least for tests.
> 
> It's disabled by default in the browser.  Some of the tests enable it in
> order to test it, but what's the point of testing a feature that isn't used
> outside of tests?

That's the point of bug 797325

Comment 9

[Nicholas Nethercote [inactive]]

> That's the point of bug 797325

I don't see much advantage to the //@line comments even if they (a) are on, and (b) actually work.  The cases I've seen are ones like browser/components/nsBrowserGlue.js, which is pre-processed to give something like $OBJ/dist/Nightly.app/Contents/MacOS/components/nsBrowserGlue.js, and the difference between the two is minor.  What's wrong with the error numbers being reported in terms of the latter file?

Comment 10

[Mike Hommey [:glandium]]

There are files with large parts enclosed in #ifdef XP_something. When looking at tbpl logs that makes error lines completely off, and looking at your local copy of the preprocessed file doesn't help. I've had the problem enough that i think it's a problem but not so much as to make me want to actively fix it (but then, this is largely because i haven't touched to preprocessed javascript for a while).

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

One thing that would improve this process would be to change the preprocessor to insert blank lines in place of the #ifdef material that was left out. That would preserve line numbers in that case. It wouldn't work for #includes, but those seem to be more rare.

Comment 12

[Nicholas Nethercote [inactive]]

(In reply to Bill McCloskey (:billm) from comment #11)
> One thing that would improve this process would be to change the
> preprocessor to insert blank lines in place of the #ifdef material that was
> left out. That would preserve line numbers in that case. It wouldn't work
> for #includes, but those seem to be more rare.

Yes, they are rarer.  Interestingly enough, they cause Preprocessor.py to include @lines for multiple files in a single generated .js file.  SpiderMonkey currently doesn't correctly handle that case -- IIRC it just records the first (or is it the last?) file seen in an @line and uses that file for the whole script.  So the error messages in that case would be totally busted anyway even with @line support enabled.  What a mess.

So my inclination is to add the whitespace as billm suggested.  That way, in files that don't use #include, we'll end up with correct line numbers.  In files that do use #include the line numbers/filenames will be broken, but that's true even if @line is enabled.

And then I can remove @line support, and if someone wants to fix the #include case properly they can use source maps, once they're supported.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Don't generate @line comments in Preprocessor.py.

This patch replaces line removed by the preprocessor with comment lines, so
that the line number match up.  An example... lines 21--23 of browser.js look
like this:

  #ifndef XP_MACOSX
  var gEditUIVisible = true; 
  #endif

The processed output looks like this:

  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:21
  var gEditUIVisible = true;
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:23

I chose to include the filename/linenum instead of a blank line because it's
more useful.

When #includes are present, the comments are less helpful.  E.g. from
browser.js lines 7437--7444:

  XPCOMUtils.defineLazyGetter(window, "gShowPageResizers", function () {
  #ifdef XP_WIN
    // Only show resizers on Windows 2000 and XP
    return parseFloat(Services.sysinfo.getProperty("version")) < 6;
  #else 
    return false;
  #endif
  });

becomes lines 14480--14487 of the processed file:

  XPCOMUtils.defineLazyGetter(window, "gShowPageResizers", function () {
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7438
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7439
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7440
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7441
    return false; 
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7443
  });

because there are 7043 lines pulled in prior to this via #include.  So error
numbers reported won't be accurate, but at least it's easy to see how the
processed file was produced from the input files.

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Don't generate @line comments in Preprocessor.py.

This patch replaces line removed by the preprocessor with comment lines, so
that the line number match up.  An example... lines 21--23 of browser.js look
like this:

  #ifndef XP_MACOSX
  var gEditUIVisible = true; 
  #endif

The processed output looks like this:

  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:21
  var gEditUIVisible = true;
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:23

I chose to include the filename/linenum instead of a blank line because it's
more useful.

When #includes are present, the comments are less helpful.  E.g. from
browser.js lines 7437--7444:

  XPCOMUtils.defineLazyGetter(window, "gShowPageResizers", function () {
  #ifdef XP_WIN
    // Only show resizers on Windows 2000 and XP
    return parseFloat(Services.sysinfo.getProperty("version")) < 6;
  #else 
    return false;
  #endif
  });

becomes lines 14480--14487 of the processed file:

  XPCOMUtils.defineLazyGetter(window, "gShowPageResizers", function () {
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7438
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7439
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7440
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7441
    return false; 
  // this line was /home/njn/moz/mi3/browser/base/content/browser.js:7443
  });

because there are 7043 lines pulled in prior to this via #include.  So error
numbers reported won't be accurate, but at least it's easy to see how the
processed file was produced from the input files.

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 721561 [details] [diff] [review]
(part 2) - Don't generate @line comments in Preprocessor.py.

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

That's probably going to make some files substantially bigger. Have you measured the difference, especially on mobile?

Comment 16

[Nicholas Nethercote [inactive]]

> That's probably going to make some files substantially bigger. Have you
> measured the difference, especially on mobile?

No.

BTW, fixing bug 747831 is a high priority for the upcoming GDC (games) conference, and this bug is blocking it.  As a result, my current plan is to land patch 1 soon and worry about patch 2 later :/

Comment 17

[Nicholas Nethercote [inactive]]

Part 1:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22835e17f50a

glandium, the ball is in your court w.r.t. whether Preprocessor.py gets changed.  (My inclination is to land the obsoleted patch which just removes the @line comments altogether, and then wait for source maps.)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22835e17f50a