Update documentation on Symbol. Make validation match.

Update documentation on Symbol. Make validation match.

Changed documentation of the Symbol constructor to correctly state
which inputs are accepted.
Changed validation to match.
Disallow keywords as identifiers.
No longer accepts "x.y=" symbols which are not the name of anything
in the source.

BUG= http://dartbug.com/13716
R=ahe@google.com

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

[floitsch, 2014-01-31 14:37:24]

FYI.

Somebody else needs to ok the actual documentation and semantics change.
("foo._bar" vs "_foo.bar", ...).

https://codereview.chromium.org/140643006/diff/1/runtime/vm/service_test.cc
File runtime/vm/service_test.cc (right):

https://codereview.chromium.org/140643006/diff/1/runtime/vm/service_test.cc#n...
runtime/vm/service_test.cc:335:
"{\"type\":\"Class\",\"id\":\"classes\\/1010\",\"name\":\"A\","
Lasse just committed a similar change.
You might need to rebase/merge.

https://codereview.chromium.org/140643006/diff/1/sdk/lib/core/symbol.dart
File sdk/lib/core/symbol.dart (right):

https://codereview.chromium.org/140643006/diff/1/sdk/lib/core/symbol.dart#new...
sdk/lib/core/symbol.dart:24: *   also not begin with an underscore ("`_`").
Why is _foo.bar not allowed, but foo._bar is?

https://codereview.chromium.org/140643006/diff/1/sdk/lib/internal/symbol.dart
File sdk/lib/internal/symbol.dart (right):

https://codereview.chromium.org/140643006/diff/1/sdk/lib/internal/symbol.dart...
sdk/lib/internal/symbol.dart:20:
r'assert|break|c(?:a(?:se|tch)|lass|on(?:st|tinue))|d(?:efault|o)|'
I would have just written them out. It would be almost 30chars shorter, and a
good regexp-engine should just transform it into a KMP or Boyer-Moore search.

https://codereview.chromium.org/140643006/diff/1/sdk/lib/internal/symbol.dart...
sdk/lib/internal/symbol.dart:34: r'(?!(?:''$reservedWord'r')\b)[a-zA-Z$][\w$]*'
this is hard to read (the ''...'r'). Maybe use "+", or add spaces, or new lines.

https://codereview.chromium.org/140643006/diff/1/sdk/lib/internal/symbol.dart...
sdk/lib/internal/symbol.dart:34: r'(?!(?:''$reservedWord'r')\b)[a-zA-Z$][\w$]*'
You have to explain how negative lookahead works.
In general: I'm pretty good at regexps, and it took me some time to understand
this line. You really have to document it better.

Also use indentation to show where parenthesis start/end.

[Lasse Reichstein Nielsen, 2014-01-31 18:19:25]

https://codereview.chromium.org/140643006/diff/1/runtime/vm/service_test.cc
File runtime/vm/service_test.cc (right):

https://codereview.chromium.org/140643006/diff/1/runtime/vm/service_test.cc#n...
runtime/vm/service_test.cc:335:
"{\"type\":\"Class\",\"id\":\"classes\\/1010\",\"name\":\"A\","
I know :)

https://codereview.chromium.org/140643006/diff/1/sdk/lib/core/symbol.dart
File sdk/lib/core/symbol.dart (right):

https://codereview.chromium.org/140643006/diff/1/sdk/lib/core/symbol.dart#new...
sdk/lib/core/symbol.dart:24: *   also not begin with an underscore ("`_`").
Because I saw no need to disallow foo._bar.
The problem is the interpretation of "_foo".
If it is a "declaration name", it is private.
If it is a library name, is it private or not?
It's hard to distinguish, so we disallow library names that start with '_'.
Underscores later are not a problem, because it is obviously a library name. I
can easily disallow them if it's a problem.
We could even allow underscores at the beginning if there is a '.' in there, but
I didn't want to complicate stuff even more.

https://codereview.chromium.org/140643006/diff/1/sdk/lib/internal/symbol.dart
File sdk/lib/internal/symbol.dart (right):

https://codereview.chromium.org/140643006/diff/1/sdk/lib/internal/symbol.dart...
sdk/lib/internal/symbol.dart:20:
r'assert|break|c(?:a(?:se|tch)|lass|on(?:st|tinue))|d(?:efault|o)|'
This is being used as part of another regexp, so no searching will be done. 
A good regexp engine might be able to make something just as fast anyway. I
should do a benchmark :)

https://codereview.chromium.org/140643006/diff/1/sdk/lib/internal/symbol.dart...
sdk/lib/internal/symbol.dart:34: r'(?!(?:''$reservedWord'r')\b)[a-zA-Z$][\w$]*'
Spaces or newlines will do. String plus is not a compile-time constant
operation, so I try to avoid it in expressions that would otherwise be constant.

https://codereview.chromium.org/140643006/diff/1/sdk/lib/internal/symbol.dart...
sdk/lib/internal/symbol.dart:34: r'(?!(?:''$reservedWord'r')\b)[a-zA-Z$][\w$]*'
Ok, will add more comments.

[floitsch, 2014-01-31 18:23:05]

https://codereview.chromium.org/140643006/diff/1/sdk/lib/internal/symbol.dart
File sdk/lib/internal/symbol.dart (right):

https://codereview.chromium.org/140643006/diff/1/sdk/lib/internal/symbol.dart...
sdk/lib/internal/symbol.dart:20:
r'assert|break|c(?:a(?:se|tch)|lass|on(?:st|tinue))|d(?:efault|o)|'
On 2014/01/31 18:19:26, Lasse Reichstein Nielsen wrote:
> This is being used as part of another regexp, so no searching will be done. 
That's not necessarily the case. A good regexp engine could definitely use one
of the good algorithms in the middle of a bigger regexp. But apparently they
don't do yet. (You should know ;)
> A good regexp engine might be able to make something just as fast anyway. I
> should do a benchmark :)

[Lasse Reichstein Nielsen, 2014-01-31 20:00:43]

https://codereview.chromium.org/140643006/diff/1/sdk/lib/internal/symbol.dart
File sdk/lib/internal/symbol.dart (right):

https://codereview.chromium.org/140643006/diff/1/sdk/lib/internal/symbol.dart...
sdk/lib/internal/symbol.dart:20:
r'assert|break|c(?:a(?:se|tch)|lass|on(?:st|tinue))|d(?:efault|o)|'
They could use, e.g., KMP or BMH for searching (and they don't), but in this
case we are not *searching* (there is no .* or similar before). It's just being
used to do a single (negative) match at an already found position.

[floitsch, 2014-01-31 20:07:48]

https://codereview.chromium.org/140643006/diff/1/sdk/lib/internal/symbol.dart
File sdk/lib/internal/symbol.dart (right):

https://codereview.chromium.org/140643006/diff/1/sdk/lib/internal/symbol.dart...
sdk/lib/internal/symbol.dart:20:
r'assert|break|c(?:a(?:se|tch)|lass|on(?:st|tinue))|d(?:efault|o)|'
On 2014/01/31 20:00:43, Lasse Reichstein Nielsen wrote:
> They could use, e.g., KMP or BMH for searching (and they don't), but in this
> case we are not *searching* (there is no .* or similar before). It's just
being
> used to do a single (negative) match at an already found position.
Right.
But that makes it even easier. A good regexp should do exactly what you did by
hand.

[Lasse Reichstein Nielsen, 2014-01-31 23:17:04]

https://codereview.chromium.org/140643006/diff/1/sdk/lib/internal/symbol.dart
File sdk/lib/internal/symbol.dart (right):

https://codereview.chromium.org/140643006/diff/1/sdk/lib/internal/symbol.dart...
sdk/lib/internal/symbol.dart:23: r'v(?:ar|oid)|w(?:hile|ith)';
Did a benchmark. This version is ~14% faster than the plain version in
Chrome/v8.

[floitsch, 2014-01-31 23:41:44]

https://codereview.chromium.org/140643006/diff/1/sdk/lib/internal/symbol.dart
File sdk/lib/internal/symbol.dart (right):

https://codereview.chromium.org/140643006/diff/1/sdk/lib/internal/symbol.dart...
sdk/lib/internal/symbol.dart:23: r'v(?:ar|oid)|w(?:hile|ith)';
On 2014/01/31 23:17:04, Lasse Reichstein Nielsen wrote:
> Did a benchmark. This version is ~14% faster than the plain version in
> Chrome/v8.

I guess that means that we should file a bug. It clearly shouldn't.

[Lasse Reichstein Nielsen, 2014-02-13 12:29:30]

Ping Ahe.

[ahe, 2014-02-13 21:56:53]

https://codereview.chromium.org/140643006/diff/1/sdk/lib/core/symbol.dart
File sdk/lib/core/symbol.dart (right):

https://codereview.chromium.org/140643006/diff/1/sdk/lib/core/symbol.dart#new...
sdk/lib/core/symbol.dart:24: *   also not begin with an underscore ("`_`").
On 2014/01/31 18:19:26, Lasse Reichstein Nielsen wrote:
> Because I saw no need to disallow foo._bar.
> The problem is the interpretation of "_foo".
> If it is a "declaration name", it is private.
> If it is a library name, is it private or not?
> It's hard to distinguish, so we disallow library names that start with '_'.
> Underscores later are not a problem, because it is obviously a library name. I
> can easily disallow them if it's a problem.
> We could even allow underscores at the beginning if there is a '.' in there,
but
> I didn't want to complicate stuff even more.

I don't think this is correct, and I look forward to talking through this with
you in person. I'll be back in AAR Tue or Wed (depending on jet-lag).

[Lasse Reichstein Nielsen, 2014-02-20 10:33:23]

Updated regexp to disallow x._y.
Added more tests.
@Ahe: PTAL

[ahe, 2014-02-21 08:11:14]

https://codereview.chromium.org/140643006/diff/130001/sdk/lib/core/symbol.dart
File sdk/lib/core/symbol.dart (right):

https://codereview.chromium.org/140643006/diff/130001/sdk/lib/core/symbol.dar...
sdk/lib/core/symbol.dart:24: *   also not begin with an underscore ("`_`").
We should also support things like:

mylibrary.myclass.*

[Lasse Reichstein Nielsen, 2014-02-21 11:55:59]

Updated per discussion. PTAL

[ahe, 2014-02-21 12:09:28]

https://codereview.chromium.org/140643006/diff/180001/tests/lib/mirrors/symbo...
File tests/lib/mirrors/symbol_validation_test.dart (right):

https://codereview.chromium.org/140643006/diff/180001/tests/lib/mirrors/symbo...
tests/lib/mirrors/symbol_validation_test.dart:35: operator.expand((op) => [op,
"x.op"]).forEach(validSymbol);
x.$op

[ahe, 2014-02-21 12:52:09]

LGTM!

https://codereview.chromium.org/140643006/diff/290001/sdk/lib/internal/symbol...
File sdk/lib/internal/symbol.dart (right):

https://codereview.chromium.org/140643006/diff/290001/sdk/lib/internal/symbol...
sdk/lib/internal/symbol.dart:31: 
Extra line.

https://codereview.chromium.org/140643006/diff/290001/tests/lib/mirrors/symbo...
File tests/lib/mirrors/symbol_validation_test.dart (right):

https://codereview.chromium.org/140643006/diff/290001/tests/lib/mirrors/symbo...
tests/lib/mirrors/symbol_validation_test.dart:55: // Privat symbols.
Privat*e*

[Lasse Reichstein Nielsen, 2014-02-24 08:11:24]

Committed patchset #6 manually as r32952 (presubmit successful).

[blois, 2014-02-24 18:46:39]

On 2014/02/24 08:11:24, Lasse Reichstein Nielsen wrote:
> Committed patchset #6 manually as r32952 (presubmit successful).

FYI- opened https://code.google.com/p/dart/issues/detail?id=17073, this change
appears to have regressed one of Angular's tests.

[ahe, 2014-02-24 19:46:59]

On 2014/02/24 18:46:39, blois wrote:
> On 2014/02/24 08:11:24, Lasse Reichstein Nielsen wrote:
> > Committed patchset #6 manually as r32952 (presubmit successful).
> 
> FYI- opened https://code.google.com/p/dart/issues/detail?id=17073, this change
> appears to have regressed one of Angular's tests.

Pete, please go ahead and revert if this is blocking you. Lasse and I will
investigate further tomorrow. 

Cheers,
Peter