Closed Bug 1102131 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox35 --- wontfix
firefox36 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(6 files)

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)
Part 2: Collect telemetry about deprecated let blocks and expressions.
Attachment #8525877 - Flags: review?(shu)
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)
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)
Attachment #8525883 - Attachment description: let-part-4-log-block-warnings.patch → part-4-log-block-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 = {}) {
Attachment #8525885 - Flags: review?(dtownsend+bugmail)
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 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 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 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 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+
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.)
Part 4: Log console warnings for deprecated let blocks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd61fe48048b
Blocks: 1167029
You need to log in before you can comment on or make changes to this bug.