Handle metadata with type literals or qualified identifiers.

Handle metadata with qualified identifiers.

BUG=http://dartbug.com/11857
BUG=http://dartbug.com/14652
R=gbracha@google.com, hausner@google.com

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

[rmacnak, 2013-10-24 18:45:02]

This crashes when encountering bad metadata; it should produce a compile-time
error. Strangely, it tries to look up a receiver even though the fake function
is static.

@notDefined
class HasBadMetadata{}

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

https://codereview.chromium.org/40863002/diff/1/runtime/vm/parser.cc#newcode904
runtime/vm/parser.cc:904: expr = ParseSelectors(expr, false);
Expects a valid current_function.

[hausner, 2013-10-24 18:54:05]

See comment below.

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

https://codereview.chromium.org/40863002/diff/1/runtime/vm/parser.cc#newcode904
runtime/vm/parser.cc:904: expr = ParseSelectors(expr, false);
You can't use this function here, or you'd accept metadata like @a[0]

[rmacnak, 2013-10-24 21:33:56]

Working but duplicates ~100 lines of parser code.

+Gilad re test

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

https://codereview.chromium.org/40863002/diff/130001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:8433: AstNode* Parser::ParseQualified(AstNode* primary) {
Yuck. This is basically ParseSelectors with about half of the cases chopped out.

https://codereview.chromium.org/40863002/diff/130001/tests/lib/mirrors/metada...
File tests/lib/mirrors/metadata_allowed_values_test.dart (right):

https://codereview.chromium.org/40863002/diff/130001/tests/lib/mirrors/metada...
tests/lib/mirrors/metadata_allowed_values_test.dart:50: @H<int>()  /// 04:
compile-time error
dart2js thinks this okay

https://codereview.chromium.org/40863002/diff/130001/tests/lib/mirrors/metada...
tests/lib/mirrors/metadata_allowed_values_test.dart:96: @V.tearOff
Having code in metadata is weird.

[gbracha, 2013-10-24 22:28:34]

test lgtm

[hausner, 2013-10-24 23:19:47]

I think this can be simplified a lot.

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

https://codereview.chromium.org/40863002/diff/130001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:8433: AstNode* Parser::ParseQualified(AstNode* primary) {
On 2013/10/24 21:33:56, Ryan Macnak wrote:
> Yuck. This is basically ParseSelectors with about half of the cases chopped
out.

I think this is way more complicated than need be. First of all, after you parse
the qualified part (with ParsePrimary), you have only one optional selector to
deal with, so the while loop is wrong. 

I find the name of the function misleading. The qualified identifier has already
been parsed at this point. All you do here is parse one additonal selector of
the form "." ident. That identifier can only refer to a static field of the
primary you parsed earlier, everything else is an error.

https://codereview.chromium.org/40863002/diff/130001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:8463: if (CurrentToken() == Token::kLPAREN) {
This can't happen for metadata.

[rmacnak, 2013-10-25 18:29:10]

Added test cases rejecting type parameters.

Renamed function to ParseMemberSelection.

Removed case handling member invocation.
Removed case handling type parameters.
Removed loop.

[hausner, 2013-10-25 20:24:25]

As discussed in person, I think the only cases that are valid are:

C
L.C
x
C.x
L.C.x

ParsePrimary will consume the x, C or L.C. In the latter two cases, you will get
back a primary node containing a class. If it is followed by .x, x must be a
static const field of that class or it's an error. If it's not followed by .x,
then the primary node refers to a type literal.

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

https://codereview.chromium.org/40863002/diff/220001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:8429: if (left->AsPrimaryNode()->primary().IsFunction()) {
I still don't understand why you handle this case. A selector in metadata can
only refer to a static const field of a class, nothing else.

https://codereview.chromium.org/40863002/diff/220001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:8462: if (primary->primary().IsFunction()) {
ditto

[rmacnak, 2013-10-25 20:37:55]

https://codereview.chromium.org/40863002/diff/220001/tests/lib/mirrors/metada...
File tests/lib/mirrors/metadata_allowed_values_test.dart (right):

https://codereview.chromium.org/40863002/diff/220001/tests/lib/mirrors/metada...
tests/lib/mirrors/metadata_allowed_values_test.dart:103: @topLevelTearOff
Gilad, is this permitted?

The spec does allow for

y() => 4;
const x = y; // ?

main() {
 print(x()); // 4
}

[gbracha, 2013-10-26 18:05:59]

On 2013/10/25 20:37:55, Ryan Macnak wrote:
>
https://codereview.chromium.org/40863002/diff/220001/tests/lib/mirrors/metada...
> File tests/lib/mirrors/metadata_allowed_values_test.dart (right):
> 
>
https://codereview.chromium.org/40863002/diff/220001/tests/lib/mirrors/metada...
> tests/lib/mirrors/metadata_allowed_values_test.dart:103: @topLevelTearOff
> Gilad, is this permitted?
> 
> The spec does allow for
> 
> y() => 4;
> const x = y; // ?
> 
> main() {
>  print(x()); // 4
> }

Yes. The name of a top level function such as y is a constant. An invocation
such as y() or x() is never a constant.

[gbracha, 2013-10-26 18:07:42]

Test changes lgtm.

[hausner, 2013-10-28 15:37:02]

Gilad, why is referencing a top-level function permitted? The spec contains this
sentence in the section about Metadata:

It is a compile time error if the expression is not one of the following:
- A reference to a compile-time constant variable.
- The name of a class.
- A call to a constant constructor.

A top-level function is none of the above.

[rmacnak, 2013-10-28 20:52:53]

On 2013/10/28 15:37:02, hausner wrote:
> Gilad, why is referencing a top-level function permitted? The spec contains
this
> sentence in the section about Metadata:
> 
> It is a compile time error if the expression is not one of the following:
> - A reference to a compile-time constant variable.
> - The name of a class.
> - A call to a constant constructor.
> 
> A top-level function is none of the above.

Updated tests for forbid top-level or static functions and literals.

[rmacnak, 2013-10-28 22:06:34]

Updated implementation.

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

https://codereview.chromium.org/40863002/diff/390001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:899: ErrorMsg(expr_pos, "class expected");
Is this the appropriate position to point at?

https://codereview.chromium.org/40863002/diff/390001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:903: ErrorMsg(ident_pos, "field expected");
Perhaps "<ident> is not a field of <class>"?

[hausner, 2013-10-28 23:06:01]

One more iteration and I think we'll be good. See comments below.

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

https://codereview.chromium.org/40863002/diff/390001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:883: 
You could simplify the following code a bit if you rewrite it as, roughly:

Class& cls = Class::Handle();
if (expr is primary node) {
  if (primary node contains class) {
    cls = ....
    expr = new TypeNode(...);
  } else {
    error: expression must refer to class or static const field
  }
}
if (CurrentToken() == Token::kPERIOD) {
  error if cls is null.
  error if static field does not exist
  error if field is not const
  expr = GenerateStaticFieldLookup()
}

https://codereview.chromium.org/40863002/diff/390001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:899: ErrorMsg(expr_pos, "class expected");
On 2013/10/28 22:06:35, Ryan Macnak wrote:
> Is this the appropriate position to point at?
yes

https://codereview.chromium.org/40863002/diff/390001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:903: ErrorMsg(ident_pos, "field expected");
On 2013/10/28 22:06:35, Ryan Macnak wrote:
> Perhaps "<ident> is not a field of <class>"?

I like this slightly better if you format the error message with the actual
field and class name.

Also, you should check whether the field is const. It's not strictly necessary,
since non-const fields will fail const expression evaluation, but it gives users
a more specific error message. 

if (!field.is_const()) {
  ErrorMsg(....);
}

[rmacnak, 2013-10-28 23:55:07]

Refactored so we only check for primary + class once.
Better error messages.

[gbracha, 2013-10-29 02:29:04]

tests lgtm

[rmacnak, 2013-10-31 19:54:10]

Updated per latest changes that forbid type literals.

[gbracha, 2013-10-31 19:58:09]

tests lgtm

[rmacnak, 2013-10-31 20:12:59]

On 2013/10/31 19:58:09, gbracha wrote:
> tests lgtm

Also remove tests/language/metadata_syntax_test.dart, which did not force
metadata evaluation.  Coverage is subsumed by
tests/lib/mirrors/metadata_allowed_values_test.dart.

[gbracha, 2013-10-31 20:15:25]

On 2013/10/31 20:12:59, Ryan Macnak wrote:
> On 2013/10/31 19:58:09, gbracha wrote:
> > tests lgtm
> 
> Also remove tests/language/metadata_syntax_test.dart, which did not force
> metadata evaluation.  Coverage is subsumed by
> tests/lib/mirrors/metadata_allowed_values_test.dart.

Excellent. lgtm on that too.

[hausner, 2013-10-31 22:38:41]

Parser not updated yet?

[rmacnak, 2013-10-31 22:40:38]

It is. (But I notice the error message still suggests type literals are
supported.)

[rmacnak, 2013-10-31 22:45:15]

Fixed error message

[hausner, 2013-10-31 23:18:16]

Parser change LGTM.

[rmacnak, 2013-11-01 01:19:42]

Committed patchset #14 manually as r29720 (presubmit successful).