Bug 23037 - Parsing and reparsing disagree on automatic semicolon insertion
Summary: Parsing and reparsing disagree on automatic semicolon insertion
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Cameron Zwarich (cpst)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-12-30 11:28 PST by Cameron Zwarich (cpst)
Modified: 2009-01-11 01:46 PST (History)
2 users (show)

See Also:


Attachments
Proposed patch (8.05 KB, patch)
2008-12-30 11:35 PST, Cameron Zwarich (cpst)
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Zwarich (cpst) 2008-12-30 11:28:16 PST
Parsing and reparsing disagree about automatic semicolon insertion, so that a function like

function() { a = 1, }

is parsed as being syntactically valid but gets a syntax error upon reparsing. This leads to an assertion failure in Parser::reparse(). It is not that big of an issue in practice, because in a release build such a function will return 'undefined' when called. Other browsers would throw a syntax error, and there aren't many people who write incorrect JS like this and only check their sites with WebKit.

In this case, we are not following the spec and it should be a syntax error. However, unless there is a newline separating the ',' and the '}', JavaScriptCore would never treat it as a syntax error in the past either. It will be a bit of work to make our automatic semicolon insertion match the spec, so I am just going to match our past behaviour.
Comment 1 Cameron Zwarich (cpst) 2008-12-30 11:29:10 PST
<rdar://problem/6467124>
Comment 2 Cameron Zwarich (cpst) 2008-12-30 11:35:42 PST
Created attachment 26314 [details]
Proposed patch
Comment 3 Darin Adler 2008-12-30 12:10:01 PST
Comment on attachment 26314 [details]
Proposed patch

I don't think that "during reparsing" is a good name for a boolean. I think that "is reparsing" would be better, or possibly "is reparsing underway".

I don't see any code setting m_duringReparsing back to false. How can subsequent parsing possibly work properly?
Comment 4 Sam Weinig 2008-12-30 13:01:48 PST
It is reset in Lexer::clear()
Comment 5 Cameron Zwarich (cpst) 2008-12-30 15:36:19 PST
(In reply to comment #3)
> (From update of attachment 26314 [details] [review])
> I don't think that "during reparsing" is a good name for a boolean. I think
> that "is reparsing" would be better, or possibly "is reparsing underway".

I had it like that originally, and it seemed awkward. I'll change it back.

> I don't see any code setting m_duringReparsing back to false. How can
> subsequent parsing possibly work properly?

Like Sam said, it is reset during clear().
Comment 6 Cameron Zwarich (cpst) 2008-12-30 16:10:35 PST
Landed in r39521 with Darin's comment.