1302692 - [Static Analysis][Logically dead code] In function Parser<ParseHandler>::continueStatement

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity detected that in the following IfStmt the last IfStmt cannot be true:

>>    if (label) {
>>        bool foundTarget = false;
>>        ParseContext::Statement* stmt = pc->innermostStatement();
>>
>>        for (;;) {
>>            stmt = ParseContext::Statement::findNearest(stmt, isLoop);
>>            if (!stmt) {
>>                report(ParseError, false, null(), JSMSG_BAD_CONTINUE);
>>                return null();
>>            }
>>
>>            // Is it labeled by our label?
>>            stmt = stmt->enclosing();
>>            while (stmt && stmt->is<ParseContext::LabelStatement>()) {
>>                if (stmt->as<ParseContext::LabelStatement>().label() == label) {
>>                    foundTarget = true;
>>                    break;
>>                }
>>                stmt = stmt->enclosing();
>>            }
>>            if (foundTarget)
>>                break;
>>        }
>>
>>        if (!foundTarget) {
>>            report(ParseError, false, null(), JSMSG_LABEL_NOT_FOUND);
>>            return null();
>>        }
>>    }

You can see that |foundTarge| is true than the first for will break leading thus to the impossibility of having the last IfStmt evaluated to true.

Comment 1

[Andi [:andi]]

Attachment: Bug 1302692 - remove dead code from Parser<ParseHandler>::continueStatement.

Review commit: https://reviewboard.mozilla.org/r/78660/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78660/

Comment 2

[Andi [:andi]]

Transferring this to Jan maybe he has time.

Comment 3

[Jan de Mooij [:jandem]]

Your patch makes sense, but there's actually a bug here as well (regression from bug 1263355 I think).

Before the rewrite, we sometimes reported "label not found":

js> function f() { X: for (var i=0; i<10; i++) { continue Y; } } 
typein:1:54 SyntaxError: label not found:
typein:1:54 function f() { X: for (var i=0; i<10; i++) { continue Y; } } 
typein:1:54 ......................................................^

After the rewrite we always throw "continue must be inside loop":

js> function f() { X: for (var i=0; i<10; i++) { continue Y; } }
typein:1:54 SyntaxError: continue must be inside loop:
typein:1:54 function f() { X: for (var i=0; i<10; i++) { continue Y; } }
typein:1:54 ......................................................^

But that's confusing because it *is* inside a loop, just the wrong label. I'll post a patch.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8791147 [details]
Bug 1302692 - remove dead code from Parser<ParseHandler>::continueStatement.

https://reviewboard.mozilla.org/r/78660/#review80610

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8796134 [details] [diff] [review]
Patch

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

::: js/src/frontend/Parser.cpp
@@ +5569,5 @@
>                  }
>                  stmt = stmt->enclosing();
>              }
>              if (foundTarget)
>                  break;

I kind of wish this has-target-label thing were a separate function stmt->loopHasLabel(HandleAtom label), for clarity and to avoid the double-breaking.  But this gets the job done.

::: js/src/jit-test/tests/parser/break-continue-errors.js
@@ +7,5 @@
> +test("A: continue;", "continue must be inside loop");
> +test("A: continue B;", "continue must be inside loop");
> +test("A: if (false) { continue; }", "continue must be inside loop");
> +test("A: if (false) { continue B; }", "continue must be inside loop");
> +test("while (false) { (() => { continue B; })(); }", "continue must be inside loop");

Worth having this with the loop labeled as B -- loop-continuing should only look up to function boundaries.  And a similar test for breaking.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> I kind of wish this has-target-label thing were a separate function
> stmt->loopHasLabel(HandleAtom label), for clarity and to avoid the
> double-breaking.  But this gets the job done.

Hm I tried this but then you have to do |stmt = stmt->enclosing();| both in the caller and in the LoopHasLabel function. What I like about the nested loop is that each statement is visited at most once. Not that it really matters here...

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4bcc0fe30b5
Fix error messages for labeled continue statements, remove some dead code. r=jwalden

Comment 9

[Phil Ringnalda (:philor)]

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

Comment 10

[Gerry Chang [:gchang]]

Mark 51 as fix-optional. If it's worth uplifting to 51, feel free to nominate it.