Prefix string literal must be identifier

Add negative test for invalid prefixes.
Update dartc's parser to validate identifier in string literal.
Add new error code.
http://code.google.com/p/dart/issues/detail?id=266

R=mmendez
BUG=
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=1099

[mmendez, 2011-11-02 13:45:58]

LGTM with comments

http://codereview.chromium.org/8441001/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):

http://codereview.chromium.org/8441001/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:365: private
boolean VerifyStringIdentifier(String id) {
Nit: VerifyString... -> verifyString...

Please add comment about why '$' is special here.

http://codereview.chromium.org/8441001/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/parser/ParserErrorCode.java (right):

http://codereview.chromium.org/8441001/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/ParserErrorCode.java:26:
EXPECTED_PREFIX_IDENTIFIER("Expected quoted identifier"),
Nit: you could say that the prefix string can only contain valid identifier
characters.

http://codereview.chromium.org/8441001/diff/1/tests/language/language.status
File tests/language/language.status (right):

http://codereview.chromium.org/8441001/diff/1/tests/language/language.status#...
tests/language/language.status:50: Prefix18NegativeTest: Fail # Issue 307
Nit: CC Ivan from the VM team as an FYI.

[codefu, 2011-11-02 14:32:57]

http://codereview.chromium.org/8441001/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):

http://codereview.chromium.org/8441001/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:365: private
boolean VerifyStringIdentifier(String id) {
On 2011/11/02 13:45:58, mmendez wrote:
> Nit: VerifyString... -> verifyString...
> 
> Please add comment about why '$' is special here.

Done.

http://codereview.chromium.org/8441001/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/parser/ParserErrorCode.java (right):

http://codereview.chromium.org/8441001/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/ParserErrorCode.java:26:
EXPECTED_PREFIX_IDENTIFIER("Expected quoted identifier"),
On 2011/11/02 13:45:58, mmendez wrote:
> Nit: you could say that the prefix string can only contain valid identifier
> characters.

Done.

http://codereview.chromium.org/8441001/diff/1/tests/language/language.status
File tests/language/language.status (right):

http://codereview.chromium.org/8441001/diff/1/tests/language/language.status#...
tests/language/language.status:50: Prefix18NegativeTest: Fail # Issue 307
On 2011/11/02 13:45:58, mmendez wrote:
> Nit: CC Ivan from the VM team as an FYI.

I filed a bug, 307, and assigned it to the VM area, along with a link to the
issue this CL covers.  Will CC as well.

[Ivan Posva, 2011-11-02 15:20:25]

This new test needs split up into multiple files.

-Ivan

http://codereview.chromium.org/8441001/diff/1/tests/language/src/Prefix18Nega...
File tests/language/src/Prefix18NegativeTest.dart (right):

http://codereview.chromium.org/8441001/diff/1/tests/language/src/Prefix18Nega...
tests/language/src/Prefix18NegativeTest.dart:8: #import("library1.dart",
prefix:"lib1.invalid");
How are you going to test that all of these different invalid prefixes are going
to fail? Don't you need three different tests for that?

[codefu, 2011-11-02 15:22:19]

http://codereview.chromium.org/8441001/diff/1/tests/language/src/Prefix18Nega...
File tests/language/src/Prefix18NegativeTest.dart (right):

http://codereview.chromium.org/8441001/diff/1/tests/language/src/Prefix18Nega...
tests/language/src/Prefix18NegativeTest.dart:8: #import("library1.dart",
prefix:"lib1.invalid");
On 2011/11/02 15:20:25, Ivan Posva wrote:
> How are you going to test that all of these different invalid prefixes are
going
> to fail? Don't you need three different tests for that?

Will do.  Still waiting for a response from Gilad about the use of escaped $ in
this prefix.

[codefu, 2011-11-02 16:04:59]

http://codereview.chromium.org/8441001/diff/4003/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):

http://codereview.chromium.org/8441001/diff/4003/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/parser/DartParser.java:381: if(id == null
|| !id.matches("[_a-zA-Z]([_A-Za-z0-9]*)")) {
Looking further at the spec and searching for just "$", it appears identifiers
cannot have the $ in them if they are in a string; hence this simplification.

[mmendez, 2011-11-02 16:52:01]

LGTM

On 2011/11/02 16:04:59, codefu wrote:
>
http://codereview.chromium.org/8441001/diff/4003/compiler/java/com/google/dar...
> File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):
> 
>
http://codereview.chromium.org/8441001/diff/4003/compiler/java/com/google/dar...
> compiler/java/com/google/dart/compiler/parser/DartParser.java:381: if(id ==
null
> || !id.matches("[_a-zA-Z]([_A-Za-z0-9]*)")) {
> Looking further at the spec and searching for just "$", it appears identifiers
> cannot have the $ in them if they are in a string; hence this simplification.

[Ivan Posva, 2011-11-02 16:54:58]

Negative tests need to end with "NegativeTest.dart", your tests should be
failing all over the place. Actually since they do not end with "Test.dart" they
are not even run.

Please fix before committing.

-Ivan



On 2011/11/02 16:52:01, mmendez wrote:
> LGTM
> 
> On 2011/11/02 16:04:59, codefu wrote:
> >
>
http://codereview.chromium.org/8441001/diff/4003/compiler/java/com/google/dar...
> > File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):
> > 
> >
>
http://codereview.chromium.org/8441001/diff/4003/compiler/java/com/google/dar...
> > compiler/java/com/google/dart/compiler/parser/DartParser.java:381: if(id ==
> null
> > || !id.matches("[_a-zA-Z]([_A-Za-z0-9]*)")) {
> > Looking further at the spec and searching for just "$", it appears
identifiers
> > cannot have the $ in them if they are in a string; hence this
simplification.

[codefu, 2011-11-02 16:58:49]

On 2011/11/02 16:54:58, Ivan Posva wrote:
> Negative tests need to end with "NegativeTest.dart", your tests should be
> failing all over the place. Actually since they do not end with "Test.dart"
they
> are not even run.
> 
> Please fix before committing.

Getting a fix together now.

[mmendez, 2011-11-02 17:00:50]

Oh, that's right -- the naming scheme.  John, Ivan is correct the test needs to
end in NegativeTest.dart.  

Is there something that the presubmit can do to check that tests listed in the
status file were not even run?

On 2011/11/02 16:54:58, Ivan Posva wrote:
> Negative tests need to end with "NegativeTest.dart", your tests should be
> failing all over the place. Actually since they do not end with "Test.dart"
they
> are not even run.
> 
> Please fix before committing.
> 
> -Ivan
> 
> 
> 
> On 2011/11/02 16:52:01, mmendez wrote:
> > LGTM
> > 
> > On 2011/11/02 16:04:59, codefu wrote:
> > >
> >
>
http://codereview.chromium.org/8441001/diff/4003/compiler/java/com/google/dar...
> > > File compiler/java/com/google/dart/compiler/parser/DartParser.java
(right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8441001/diff/4003/compiler/java/com/google/dar...
> > > compiler/java/com/google/dart/compiler/parser/DartParser.java:381: if(id
==
> > null
> > > || !id.matches("[_a-zA-Z]([_A-Za-z0-9]*)")) {
> > > Looking further at the spec and searching for just "$", it appears
> identifiers
> > > cannot have the $ in them if they are in a string; hence this
> simplification.