Disentangle AstNode::Name and AstNode::ShortName.

Disentangle AstNode::Name and AstNode::ShortName.

The Name function is used for pretty printing except that for a few
node types we rely on a specific name for implementation correctness.
Overriding the pretty printing function to get out a type name, or a
variable name, or an operator symbol name, is risky.

1. Code that relies on the semantics of the name will break if the
   pretty name is changed.

2. Pretty printing is less useful.  For instance, LoadLocalNode just
   prints the variable name (twice, once as the AST constructor and
   once in quotes as the variable name).  This is confusing if the
   variable happens to have the same name as one of the other pretty
   names, e.g., args or seq.

Now, there is a virtual function to get the pretty name but classes do
not change it to communicate other information (type names, variable
names, operator names).

BUG=
R=regis@google.com, srdjan@google.com

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

[Kevin Millikin (Google), 2013-09-05 13:53:45]

I'm not sure who to direct this review to.  Redirect as necessary.

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

https://codereview.chromium.org/23960003/diff/1/runtime/vm/parser.cc#newcode8229
runtime/vm/parser.cc:8229: const String& name =
String::ZoneHandle(Symbols::New("primary"));
I know this seems wrong (and not useful), but it matches the previous behavior
where PrimaryNode::Name() was not overridden.

This is dead code as far as our tests go, and it's sufficiently complicated that
I didn't dare change it.  Maybe someone on the team knows enough to take a look?

[srdjan, 2013-09-05 18:57:26]

LGTM

[regis, 2013-09-05 19:42:12]

LGTM with a comment

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

https://codereview.chromium.org/23960003/diff/1/runtime/vm/parser.cc#newcode8229
runtime/vm/parser.cc:8229: const String& name =
String::ZoneHandle(Symbols::New("primary"));
On 2013/09/05 13:53:46, kmillikin wrote:
> I know this seems wrong (and not useful), but it matches the previous behavior
> where PrimaryNode::Name() was not overridden.
> 
> This is dead code as far as our tests go, and it's sufficiently complicated
that
> I didn't dare change it.  Maybe someone on the team knows enough to take a
look?

This is not dead code. You can trigger this code with the test below. Maybe we
should add it as a test, or feel free to file a bug against me, or at least add
a TODO(regis).

class C<T> {
  foo() {
    T();
  }
}

main() {
  new C().foo();
}

As expected, 'primary' wrongly shows up in the error:

Unhandled exception:
No static method 'primary' declared in class 'C'.

NoSuchMethodError : method not found: 'primary'
Receiver: Type: class 'C'
Arguments: [...]
#0      NoSuchMethodError._throwNew (dart:core-patch/errors_patch.dart:99:5)
#1      C.foo (file:///usr/local/google/home/regis/bug2.dart:3:5)
#2      main (file:///usr/local/google/home/regis/bug2.dart:8:14)


So the name should be 'T', not 'primary'. Something like this should do:
Instead of "primary", use
TypeParameter::Cast(primary->primary()).name()

The next question is whether the error message makes sense. I need to check the
spec for the appropriate error to report here. It would make sense in a static
context, but in a dynamic context, I would rather expect "class 'C' has no
method 'T'".

[Kevin Millikin (Google), 2013-09-06 11:54:48]

Committed patchset #2 manually as r27244 (presubmit successful).

[Kevin Millikin (Google), 2013-09-06 12:18:45]

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

https://codereview.chromium.org/23960003/diff/1/runtime/vm/parser.cc#newcode8229
runtime/vm/parser.cc:8229: const String& name =
String::ZoneHandle(Symbols::New("primary"));
Thanks for the clarification.  I filed issue 13134 and added TODO(regis) with
the issue number at this site in parser.cc.