1102131 - Log warnings and collect telemetry for deprecated let blocks and let expressions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: part-1-fix-spelling.patch

ES6 does not spec let blocks and let expressions. Before we remove them, we should log console warnings and collect parser telemetry about their usage on the web.

Part 1: Fix spelling of "LetExpresion".

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: part-2-collect-telemetry.patch

Part 2: Collect telemetry about deprecated let blocks and expressions.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: part-3-log-expression-warnings.patch

Part 3: Log console warnings for deprecated let expressions.

let expressions were added in JavaScript 1.7, but they are not version-gated. Is mentioning "JavaScript 1.7" in the console warning useful or a distraction?

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: part-4-log-block-warnings.patch

Part 4: Log console warnings for deprecated let blocks.

I separated the warning patches for let blocks and let expressions because I will probably wait to land the let block warnings in Firefox 37. I found only a few uses of let expressions in Firefox code, whereas there are more uses of let blocks that should be fixed before logging warnings.

Alternatively, this patch could be adapted to only warn about web content's use of let blocks (by checking whether filename starts with "http[s]://" like the parser telemetry code already does).

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: part-5-fix-toolkit-warnings.patch

Part 5: Remove deprecated let blocks and expressions in toolkit (and clean up some extra whitespace)

toolkit/components/places/nsPlacesAutoComplete.js:1047:    let (params = query.params) {
toolkit/components/places/nsPlacesAutoComplete.js:1070:    let (params = query.params) {
toolkit/components/places/nsPlacesAutoComplete.js:1107:    let (params = query.params) {
toolkit/components/places/nsPlacesAutoComplete.js:1129:    let (params = query.params) {
toolkit/components/places/nsPlacesAutoComplete.js:1542:    let (params = query.params) {

toolkit/content/widgets/autocomplete.xml:1365:          regions = regions.sort(function(a, b) let (start = a[0] - b[0])

toolkit/mozapps/extensions/internal/XPIProviderUtils.js:546:        let (str = {}) {
toolkit/mozapps/extensions/nsBlocklistService.js:766:      let (str = {}) {

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: part-6-fix-DownloadsCommon-jsm.patch

Part 6: Remove deprecated let block in DownloadsCommon.jsm.

JavaScript warning: resource:///modules/DownloadsCommon.jsm, line 418: JavaScript 1.7's let blocks are deprecated

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8525875 [details] [diff] [review]
part-1-fix-spelling.patch

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

Nice.

Comment 7

[Shu-yu Guo [:shu]]

Comment on attachment 8525877 [details] [diff] [review]
part-2-collect-telemetry.patch

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

I don't really know anything about Histograms.json but that just looks like changing the legend.

::: js/src/frontend/Parser.cpp
@@ +8338,5 @@
>          DeprecatedDestructuringForIn = 1, // JS 1.7 only
>          DeprecatedLegacyGenerator = 2,    // JS 1.7+
>          DeprecatedExpressionClosure = 3,  // Added in JS 1.8, but not version-gated
> +        DeprecatedLetBlock = 4,           // Added in JS 1.7, but not version-gated
> +        DeprecatedLetExpression = 5,      // Added in JS 1.7, but not version-gated

It's legal to have a trailing comma on the final item in an enum? I never knew.

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8525880 [details] [diff] [review]
part-3-log-expression-warnings.patch

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

Don't forget to increment subtrahend for the XDR bytecode version since you changed js.msg.

::: js/src/frontend/Parser.cpp
@@ +3669,3 @@
>          sawDeprecatedLetExpression = true;
> +        if (!report(ParseWarning, false, null(), JSMSG_DEPRECATED_LET_EXPRESSION))
> +            return null();

Please pass in |pc->sc->strict| and |expr| for the 2nd and 3rd arguments, unless not passing in strictness is intentional.

Comment 9

[Shu-yu Guo [:shu]]

Comment on attachment 8525883 [details] [diff] [review]
part-4-log-block-warnings.patch

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

Don't forget the increment the subtrahend, but no need to do it twice for both parts 3 and 4, obviously.

::: js/src/frontend/Parser.cpp
@@ +3663,3 @@
>          sawDeprecatedLetBlock = true;
> +        if (!report(ParseWarning, false, null(), JSMSG_DEPRECATED_LET_BLOCK))
> +            return null();

Ditto comment as part 3.

Comment 10

[Chris Peterson [:cpeterson]]

Landed parts 1–3,5,6:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe4bdefa88d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/81f50ea78442
https://hg.mozilla.org/integration/mozilla-inbound/rev/818d3fd048a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/2404514c2165
https://hg.mozilla.org/integration/mozilla-inbound/rev/942b4e3c6a07

Depending on early telemetry numbers for let block usage, I may postpone landing part 5 (console warnings for let blocks) until Nightly 37.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fe4bdefa88d1
https://hg.mozilla.org/mozilla-central/rev/81f50ea78442
https://hg.mozilla.org/mozilla-central/rev/818d3fd048a4
https://hg.mozilla.org/mozilla-central/rev/2404514c2165
https://hg.mozilla.org/mozilla-central/rev/942b4e3c6a07

Comment 12

[Florian Scholz (Open Web Docs)]

let expressions and let blocks are marked as deprecated on the let page already with a big warning
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let#Non-standard_let_extensions

Firefox 36 for developers mentions the warning and discourages the usage:
https://developer.mozilla.org/en-US/Firefox/Releases/36#JavaScript

Comment 13

[Jeff Walden [:Waldo]]

(In reply to Shu-yu Guo [:shu] from comment #7)
> It's legal to have a trailing comma on the final item in an enum? I never
> knew.

Since C99/C++11, yeah.  We compile all C++ in C++11 mode, so always good there.  (Slightly less sure about .c files, but I think those are c99 as well, so good there too.)

Comment 14

[Chris Peterson [:cpeterson]]

Part 4: Log console warnings for deprecated let blocks.

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

Comment 15

[Phil Ringnalda (:philor)]

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