Closed Bug 1302692 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- fix-optional
firefox52 --- fixed

People

(Reporter: andi, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1372294)

Attachments

(1 file, 1 obsolete file)

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.
Attachment #8791147 - Flags: review?(jorendorff) → review?(jdemooij)
Transferring this to Jan maybe he has time.
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.
Blocks: 1263355
Attached patch PatchSplinter Review
Assignee: bpostelnicu → jdemooij
Status: NEW → ASSIGNED
Attachment #8796134 - Flags: review?(jwalden+bmo)
Comment on attachment 8791147 [details]
Bug 1302692 - remove dead code from Parser<ParseHandler>::continueStatement.

https://reviewboard.mozilla.org/r/78660/#review80610
Attachment #8791147 - Flags: review?(jdemooij)
Attachment #8791147 - Attachment is obsolete: true
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.
Attachment #8796134 - Flags: review?(jwalden+bmo) → review+
(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...
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4bcc0fe30b5
Fix error messages for labeled continue statements, remove some dead code. r=jwalden
https://hg.mozilla.org/mozilla-central/rev/a4bcc0fe30b5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Mark 51 as fix-optional. If it's worth uplifting to 51, feel free to nominate it.
You need to log in before you can comment on or make changes to this bug.