Add console warnings for JS1.7 legacy generators

RESOLVED FIXED in Firefox 57

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: cpeterson, Assigned: emk)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

Reporter

Updated

5 years ago
Blocks: 867615
Reporter

Updated

5 years ago
Blocks: 968038
Reporter

Updated

5 years ago
Blocks: 1083485
Reporter

Updated

5 years ago
Blocks: 867612
Reporter

Updated

4 years ago
Blocks: 1104014
Reporter

Updated

4 years ago
No longer blocks: 1083485
Blocks: 1083482
No longer blocks: 968038
Comment hidden (mozreview-request)
Oh this sounds nice! Does this warning trigger at all? I seem to remember that we still had a lot of legacy generator functions in chrome code, so just enabling this would overwhelm everything.
(In reply to Limited Availabilty till Oct | Tom Schuster [:evilpie] from comment #2)
> I seem to remember
> that we still had a lot of legacy generator functions in chrome code, so
> just enabling this would overwhelm everything.

for mozilla-central, most of them should be removed in bug 968038 and depending bugs.
there may be some remainder tho.

for comm-central, there's 2 bugs under bug 1123122, but can be fixed within 2 weeks I hope.
Comment on attachment 8894217 [details]
Bug 1083476 - Add console warnings for JS1.7 legacy generators.

Oh wow you fixed a ton of stuff, I had no idea. You get the honor of review it :)
Attachment #8894217 - Flags: review?(evilpies) → review?(arai.unmht)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8894217 [details]
Bug 1083476 - Add console warnings for JS1.7 legacy generators.

https://reviewboard.mozilla.org/r/165296/#review170876

Thank you for taking this :D
It looks almost perfect, except one cosmetic fix below.
(iiuc, we cannot use "r+ with fix" in MozReview)

::: js/src/frontend/Parser.cpp:6805
(Diff revision 1)
>  
>          pc->functionBox()->setGeneratorKind(LegacyGenerator);
>          addTelemetry(DeprecatedLanguageExtension::LegacyGenerator);
> +        if (!warnOnceAboutLegacyGenerator()) {
> +            return null();
> +        }

Please remove braces around one-liner if

::: js/src/jit-test/tests/parser/yield-without-operand.js:8
(Diff revision 1)
>  load(libdir + "asserts.js");
>  
> -assertNoWarning(() => Function("yield"), SyntaxError,
> +let GeneratorFunction = Object.getPrototypeOf(function*(){}).constructor;
> +
> +assertNoWarning(() => GeneratorFunction("yield"),
>                  "yield followed by EOF is fine");

we can just remove this file.
it's no more testing what the testcase is initially for, after several changes.

::: js/src/jscompartment.h:627
(Diff revision 1)
>      bool                         isSelfHosting;
>      bool                         marked;
> -    bool                         warnedAboutDateToLocaleFormat;
> -    bool                         warnedAboutExprClosure;
> -    bool                         warnedAboutForEach;
> +    bool                         warnedAboutDateToLocaleFormat : 1;
> +    bool                         warnedAboutExprClosure : 1;
> +    bool                         warnedAboutForEach : 1;
> +    bool                         warnedAboutLegacyGenerator : 1;

Nice change :)
Attachment #8894217 - Flags: review?(arai.unmht)
Attachment #8894217 - Flags: feedback+
(In reply to Tooru Fujisawa [:arai] from comment #5)
> Thank you for taking this :D
> It looks almost perfect, except one cosmetic fix below.
> (iiuc, we cannot use "r+ with fix" in MozReview)

We can. :) Mozreview prevents landing until all issues are marked resolved. The r+ carries forward to the next push, and the issues need to be marked resolved before landing.

Comment 7

2 years ago
mozreview-review
Comment on attachment 8894217 [details]
Bug 1083476 - Add console warnings for JS1.7 legacy generators.

https://reviewboard.mozilla.org/r/165296/#review170902

(In reply to Kris Maglione [:kmag] from comment #6)
> (In reply to Tooru Fujisawa [:arai] from comment #5)
> > Thank you for taking this :D
> > It looks almost perfect, except one cosmetic fix below.
> > (iiuc, we cannot use "r+ with fix" in MozReview)
> 
> We can. :) Mozreview prevents landing until all issues are marked resolved.
> The r+ carries forward to the next push, and the issues need to be marked
> resolved before landing.

Thank you for letting me know that :D

okay, then r+ with the braces removed.
whether removing yield-without-operand.js or not is up to you.
Attachment #8894217 - Flags: review+
Assignee

Updated

2 years ago
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Assignee

Comment 9

2 years ago
mozreview-review-reply
Comment on attachment 8894217 [details]
Bug 1083476 - Add console warnings for JS1.7 legacy generators.

https://reviewboard.mozilla.org/r/165296/#review170876

> Please remove braces around one-liner if

Oh, I didn't realize SpiderMonkey did not change the coding rule even after gotofail. Removed the braces.

> we can just remove this file.
> it's no more testing what the testcase is initially for, after several changes.

Removed.

Comment 10

2 years ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/3f4d9d490af2
Add console warnings for JS1.7 legacy generators. r=arai

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3f4d9d490af2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.