Strict mode octal literals.

Strict mode octal literals.


BUG=
TEST=test/mjsunit/strict-mode.js

[Lasse Reichstein, 2011-01-21 11:20:20]

LGTM

http://codereview.chromium.org/6248013/diff/1/src/scanner-base.cc
File src/scanner-base.cc (right):

http://codereview.chromium.org/6248013/diff/1/src/scanner-base.cc#newcode101
src/scanner-base.cc:101: octal_loc_.beg_pos = source_pos() - 1;    // Already
advanced
It seems like overkill to store both beg and end pos for an octal literal.
Just store the beginning (and initialize it to -1 until the first one is found,
that will be outside the range of all source code).

http://codereview.chromium.org/6248013/diff/1/src/scanner-base.cc#newcode602
src/scanner-base.cc:602: // TODO(mmaly): Do we want to keep the auto-upgrade to
decimal?
Alas, yes.
Unless it turns out that Safari has changed on that too.
It's only octal escapes that are disallowed as grammar extensions, not decimal
escapes.

http://codereview.chromium.org/6248013/diff/1/src/scanner-base.cc#newcode742
src/scanner-base.cc:742: if (c0_ == '\\') {  // Escape sequence.
We should also add checking of RegExp literals. They may also contain octal
escapes. It is not as simple as checking whether the next character after '\' is
'0'. The sequence '\12' may be the octal escape for newline, or it may be a
backreference to capture 12, or a backreference to capture 1 followed by a "2".
We really need to do a full parse of the regexp to know.
I don't remember off-hand whether we parse the regexp eagerly for syntax errors,
or if we delay it until it's first executed. If the latter, then we probably
need to do it earlier for strict code.

[MarkM, 2011-01-21 17:44:48]

http://codereview.chromium.org/6248013/diff/1/src/scanner-base.cc
File src/scanner-base.cc (right):

http://codereview.chromium.org/6248013/diff/1/src/scanner-base.cc#newcode742
src/scanner-base.cc:742: if (c0_ == '\\') {  // Escape sequence.
On 2011/01/21 11:20:20, Lasse Reichstein wrote:
> I don't remember off-hand whether we parse the regexp eagerly for syntax
errors,
> or if we delay it until it's first executed. If the latter, then we probably
> need to do it earlier for strict code.

ES5.1 Chapter 16 Errors states:

  An implementation must treat any instance of the following kinds of errors as
an early error: [...]
  • Errors in regular expression literals that are not implementation-defined
syntax extensions

This is not conditional on strict mode.

[Martin Maly, 2011-01-21 23:32:57]

Thanks for the review, Lasse, I changed the code accordingly, please take
another peek.

As for the regular expressions, I propose to make it another patch. Trying to
keep them small.

Thank you!
Martin

http://codereview.chromium.org/6248013/diff/1/src/scanner-base.cc
File src/scanner-base.cc (right):

http://codereview.chromium.org/6248013/diff/1/src/scanner-base.cc#newcode101
src/scanner-base.cc:101: octal_loc_.beg_pos = source_pos() - 1;    // Already
advanced
On 2011/01/21 11:20:20, Lasse Reichstein wrote:
> It seems like overkill to store both beg and end pos for an octal literal.
> Just store the beginning (and initialize it to -1 until the first one is
found,
> that will be outside the range of all source code).

Done.

http://codereview.chromium.org/6248013/diff/1/src/scanner-base.cc#newcode602
src/scanner-base.cc:602: // TODO(mmaly): Do we want to keep the auto-upgrade to
decimal?
On 2011/01/21 11:20:20, Lasse Reichstein wrote:
> Alas, yes.
> Unless it turns out that Safari has changed on that too.
> It's only octal escapes that are disallowed as grammar extensions, not decimal
> escapes. 

Done.

http://codereview.chromium.org/6248013/diff/1/src/scanner-base.cc#newcode742
src/scanner-base.cc:742: if (c0_ == '\\') {  // Escape sequence.
On 2011/01/21 11:20:20, Lasse Reichstein wrote:
> We should also add checking of RegExp literals. They may also contain octal
> escapes. It is not as simple as checking whether the next character after '\'
is
> '0'. The sequence '\12' may be the octal escape for newline, or it may be a
> backreference to capture 12, or a backreference to capture 1 followed by a
"2".
> We really need to do a full parse of the regexp to know.
> I don't remember off-hand whether we parse the regexp eagerly for syntax
errors,
> or if we delay it until it's first executed. If the latter, then we probably
> need to do it earlier for strict code.

I saw the octal parsing code. I propose to do this in another patch ... trying
to keep them small. let me know if you prefer I add it to this one though.

[Lasse Reichstein, 2011-01-24 07:59:37]

LGTM

http://codereview.chromium.org/6248013/diff/1/src/scanner-base.cc
File src/scanner-base.cc (right):

http://codereview.chromium.org/6248013/diff/1/src/scanner-base.cc#newcode742
src/scanner-base.cc:742: if (c0_ == '\\') {  // Escape sequence.
Even so, we currently don't catch the syntax error until the RegExp literal is
evaluated. If we change this to an early syntax error, we again risk the chance
of breaking existing pages (it is an incompatible change from ES3 to ES5 to
*require* early errors here).

As a side note, our preparser means that we completely skip parsing of some
functions until they are first called. That means that to catch errors early,
the preparser must catch all early errors, since it's the only code to see the
source in time. Since it doesn't build an AST or record most of its tokens,
there are a number of early errors it can't catch in its current form.

In either case, keeping RegExp validation in a separate patch is the right thing
to do.

http://codereview.chromium.org/6248013/diff/6001/src/scanner-base.cc#newcode68
src/scanner-base.cc:68: 
Make -1 a constant in the Scanner class.
Scanner::kNoOctalLocation would be a fine name.
And give it a comment that says that -1 is outside the range of real source
code.

[Martin Maly, 2011-01-24 17:21:24]

Done, please let me know whether it's ok to commit.

http://codereview.chromium.org/6248013/diff/6001/src/scanner-base.cc
File src/scanner-base.cc (right):

http://codereview.chromium.org/6248013/diff/6001/src/scanner-base.cc#newcode68
src/scanner-base.cc:68: : octal_pos_(-1) { }
On 2011/01/24 07:59:37, Lasse Reichstein wrote:
> Make -1 a constant in the Scanner class.
> Scanner::kNoOctalLocation would be a fine name.
> And give it a comment that says that -1 is outside the range of real source
> code.

Done.

[Lasse Reichstein, 2011-01-24 17:43:46]

It's fine, go ahead and commit!