Closed Bug 1435818 Opened 3 years ago Closed 1 year ago

Enable numeric separators in release builds

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox60 --- wontfix
firefox70 --- fixed

People

(Reporter: till, Assigned: rhunt)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, parity-chrome)

Attachments

(1 file)

... once we're confident there'll be no more churn in the spec.
Blocks: es-proposals-stage-3
No longer blocks: 1435813
Till, can you provide a better description into what this is supposed to be and why firefox 60 (P1) is flagged as affected?
Flags: needinfo?(till)
Priority: -- → P1
FF60 being affected is simply the default for a new bug. It's also true in that FF60 doesn't have this enabled, but, once the blocking bug is fixed, could. That doesn't mean it should be P1 at all.

As for the description, I added a link to the spec proposal as the URL, I hope that's sufficient.
Flags: needinfo?(till)
Priority: P1 → P3

Is the fix in bug 1421400 Nightly-only?

(In reply to j.j. from comment #3)

Is the fix in bug 1421400 Nightly-only?

Answering myself: Yes, seems so.
This should land in Beta 68 for Chrome compat and for 68 ESR and Fennec.

Chrome 75 ships Numeric Separators in two weeks
https://www.chromestatus.com/feature/5829906369871872

Flags: needinfo?(till)

(In reply to j.j. from comment #4)

This should land in Beta 68 for Chrome compat and for 68 ESR and Fennec.

I don't think numeric separators will be used in code shipping to browsers quickly enough that we'd have to worry about Chrome compat here. Even for the ESR I really don't think that this needs to be uplifted.

What would make sense to me is to let it ride the trains either this or the next release—it seems sufficiently low-risk that we don't have to wait very long.

Flags: needinfo?(till)

Spec: https://github.com/tc39/proposal-numeric-separator

This was released in V8 release 75 [1], and it sounds like spec concerns have
been figured out and we can let this ride to release.

[1] https://v8.dev/blog/v8-release-75#numeric-separators

Bug 1566516 imported tests from the WASM spec repository that use numeric separators, which caused beta simulation failures in bug 1568806. I can modify the import script to not generate '_' in numeric literals, but it sounds like this is ready to ride the trains to release.

If no one has objections, I'd like to let this ride the trains to release.

Assignee: nobody → rhunt

https://treeherder.mozilla.org/#/jobs?repo=try&revision=96e7ee047e71d911bb47e7e66c102fb49f287503

I'm unsure if this requires updates to test262-update.py or not. I just removed the NIGHTLY_BUILD's from the initial implementation patch.

Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/d730bc44f2d7
JS: Let numeric separators ride the trains to release. r=jorendorff
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

Doc updates:

fwiw, I think the error messages in Chrome are better.

  • We mostly throw: "SyntaxError: missing digit after '_' numeric separator"
  • Chrome throws:
    • "Numeric separators are not allowed at the end of numeric literals" for 1_
    • "Only one underscore is allowed as numeric separator" for 1__1
  • Separator after null is fine, though
    • Firefox: "numeric separators '_' are not allowed in numbers that start with '0'"
    • Chrome: "Numeric separator can not be used after leading 0."

:till, can you review if the docs look alright?
Also, do you think we should make our error messages better?

Flags: needinfo?(till)

The docs look great! The only small nit is that "Only one underscore is allowed" is slightly ambiguous, but I don't think that's a problem in practice, as the example right below makes clear what it means.

Making the error messages better would be good, yes. Could you file a new bug for that?

Flags: needinfo?(till)

Florian; see Till's comments above.

Flags: needinfo?(fscholz)

Thanks for your feedback, Till!
Reworded to remove ambiguity and filed https://bugzilla.mozilla.org/show_bug.cgi?id=1589072 for potentially improving the error messages.

Flags: needinfo?(fscholz)
You need to log in before you can comment on or make changes to this bug.