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

RESOLVED FIXED in Firefox 36

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

(Blocks 1 bug, {dev-doc-complete, site-compat})

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox35 wontfix, firefox36 fixed)

Details

Attachments

(6 attachments)

Assignee

Description

5 years ago
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".
Attachment #8525875 - Flags: review?(shu)
Assignee

Comment 1

5 years ago
Part 2: Collect telemetry about deprecated let blocks and expressions.
Attachment #8525877 - Flags: review?(shu)
Assignee

Comment 2

5 years ago
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?
Attachment #8525880 - Flags: review?(shu)
Assignee

Comment 3

5 years ago
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).
Attachment #8525883 - Flags: review?(shu)
Assignee

Updated

5 years ago
Attachment #8525883 - Attachment description: let-part-4-log-block-warnings.patch → part-4-log-block-warnings.patch
Assignee

Comment 4

5 years ago
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 = {}) {
Attachment #8525885 - Flags: review?(dtownsend+bugmail)
Assignee

Comment 5

5 years ago
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
Attachment #8525887 - Flags: review?(mak77)
Attachment #8525885 - Flags: review?(dtownsend+bugmail) → review+

Comment 6

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

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

Nice.
Attachment #8525875 - Flags: review?(shu) → review+

Comment 7

5 years ago
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.
Attachment #8525877 - Flags: review?(shu) → review+

Comment 8

5 years ago
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.
Attachment #8525880 - Flags: review?(shu) → review+

Comment 9

5 years ago
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.
Attachment #8525883 - Flags: review?(shu) → review+
Attachment #8525887 - Flags: review?(mak77) → review+
Assignee

Updated

5 years ago
Blocks: 1103158
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
(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.)
Assignee

Comment 14

5 years ago
Part 4: Log console warnings for deprecated let blocks.

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

Updated

4 years ago
Blocks: 1167029
You need to log in before you can comment on or make changes to this bug.