Evaluate compile-time constants of metadata.

Evaluate compile-time constants of metadata.

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

[Johnni Winther, 2012-08-22 13:45:40]

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
File dart/lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
dart/lib/compiler/implementation/elements/elements.dart:1602: final Element
enclosingElement;
enclosingElement is a bad name if it is always the compilation unit. I would
also expect the element to be the annotated element (which isn't enclosing
either).

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
dart/lib/compiler/implementation/elements/elements.dart:1606:
[this.resolutionState = ClassElement.STATE_NOT_STARTED]);
Why allow for another initial resolutionState?

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
File dart/lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
dart/lib/compiler/implementation/resolver.dart:406:
annotation.enclosingElement);
The element kind should be something else. We cannot risk code wrongfully
relying on fakeElement to be a function.

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
dart/lib/compiler/implementation/resolver.dart:411: print(annotation);
Debug output.

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
File dart/lib/compiler/implementation/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
dart/lib/compiler/implementation/scanner/listener.dart:589: beginToken));
Does this always define the scope for the metadata resolution?  Couldn't
metadata refer to a static const defined in the same class?

[ahe, 2012-08-22 14:01:54]

Hi Johnni,

Thank you for taking a look.

I have answered some of your comments below, but I haven't uploaded a new patch
yet.

Cheers,
Peter

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
File dart/lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
dart/lib/compiler/implementation/elements/elements.dart:1602: final Element
enclosingElement;
On 2012/08/22 13:45:40, Johnni Winther wrote:
> enclosingElement is a bad name if it is always the compilation unit. I would
> also expect the element to be the annotated element (which isn't enclosing
> either).

It isn't supposed to always be a compilation unit. I just haven't written a test
where it isn't.

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
dart/lib/compiler/implementation/elements/elements.dart:1606:
[this.resolutionState = ClassElement.STATE_NOT_STARTED]);
On 2012/08/22 13:45:40, Johnni Winther wrote:
> Why allow for another initial resolutionState?

I need that for PatchMetadataAnnotation.

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
File dart/lib/compiler/implementation/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
dart/lib/compiler/implementation/scanner/listener.dart:589: beginToken));
On 2012/08/22 13:45:40, Johnni Winther wrote:
> Does this always define the scope for the metadata resolution?  Couldn't
> metadata refer to a static const defined in the same class?

This is what I assume:

@foo // Error: foo cannot be resolved.
class Foo {
  static const foo = null;
}

class Bar {
  @foo // OK
  static const foo = null;
}

[Johnni Winther, 2012-08-22 14:08:36]

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
File dart/lib/compiler/implementation/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
dart/lib/compiler/implementation/scanner/listener.dart:589: beginToken));
On 2012/08/22 14:01:54, ahe wrote:
> On 2012/08/22 13:45:40, Johnni Winther wrote:
> > Does this always define the scope for the metadata resolution?  Couldn't
> > metadata refer to a static const defined in the same class?
> 
> This is what I assume:
> 
> @foo // Error: foo cannot be resolved.
> class Foo {
>   static const foo = null;
> }
> 
> class Bar {
>   @foo // OK
>   static const foo = null;
> }

But what about:

class Foo {
  static const foo = null;

  @foo
  void bar() {};
}

Should @foo be allowed at bar?

[ngeoffray, 2012-08-22 14:28:56]

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
File dart/lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
dart/lib/compiler/implementation/resolver.dart:404: Element fakeElement = new
Element(const SourceString(''),
Should we have a metadata element instead? This sounds like a big hack around
the system.

[ahe, 2012-08-22 17:23:02]

Hi Nicolas and Johnni,

Thank you for your suggestions. Please take another look.

Cheers,
Peter

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
File dart/lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
dart/lib/compiler/implementation/elements/elements.dart:1602: final Element
enclosingElement;
On 2012/08/22 14:01:54, ahe wrote:
> On 2012/08/22 13:45:40, Johnni Winther wrote:
> > enclosingElement is a bad name if it is always the compilation unit. I would
> > also expect the element to be the annotated element (which isn't enclosing
> > either).
> 
> It isn't supposed to always be a compilation unit. I just haven't written a
test
> where it isn't.

You convinced me offline that it should be annotatedElement instead. Works much
better, thank you.

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
File dart/lib/compiler/implementation/resolver.dart (right):

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
dart/lib/compiler/implementation/resolver.dart:404: Element fakeElement = new
Element(const SourceString(''),
On 2012/08/22 14:28:56, ngeoffray wrote:
> Should we have a metadata element instead? This sounds like a big hack around
> the system.

Done.

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
dart/lib/compiler/implementation/resolver.dart:406:
annotation.enclosingElement);
On 2012/08/22 13:45:40, Johnni Winther wrote:
> The element kind should be something else. We cannot risk code wrongfully
> relying on fakeElement to be a function.

Done.

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
dart/lib/compiler/implementation/resolver.dart:411: print(annotation);
On 2012/08/22 13:45:40, Johnni Winther wrote:
> Debug output.

Done.

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
File dart/lib/compiler/implementation/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/10870010/diff/1/dart/lib/compiler/impl...
dart/lib/compiler/implementation/scanner/listener.dart:589: beginToken));
On 2012/08/22 14:08:36, Johnni Winther wrote:
> On 2012/08/22 14:01:54, ahe wrote:
> > On 2012/08/22 13:45:40, Johnni Winther wrote:
> > > Does this always define the scope for the metadata resolution?  Couldn't
> > > metadata refer to a static const defined in the same class?
> > 
> > This is what I assume:
> > 
> > @foo // Error: foo cannot be resolved.
> > class Foo {
> >   static const foo = null;
> > }
> > 
> > class Bar {
> >   @foo // OK
> >   static const foo = null;
> > }
> 
> But what about:
> 
> class Foo {
>   static const foo = null;
> 
>   @foo
>   void bar() {};
> }
> 
> Should @foo be allowed at bar?

I assume so.

[ngeoffray, 2012-08-22 21:57:54]

LGTM!

https://chromiumcodereview.appspot.com/10870010/diff/3002/dart/lib/compiler/i...
File dart/lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10870010/diff/3002/dart/lib/compiler/i...
dart/lib/compiler/implementation/elements/elements.dart:1607:
MetadataAnnotation([this.resolutionState = ClassElement.STATE_NOT_STARTED]);
Should these constant values be on some other place than ClassElement?

[ahe, 2012-08-23 05:43:34]

https://chromiumcodereview.appspot.com/10870010/diff/3002/dart/lib/compiler/i...
File dart/lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10870010/diff/3002/dart/lib/compiler/i...
dart/lib/compiler/implementation/elements/elements.dart:1607:
MetadataAnnotation([this.resolutionState = ClassElement.STATE_NOT_STARTED]);
On 2012/08/22 21:57:54, ngeoffray wrote:
> Should these constant values be on some other place than ClassElement?

Done.

[Johnni Winther, 2012-08-23 08:06:18]

lgtm

https://chromiumcodereview.appspot.com/10870010/diff/5012/dart/lib/compiler/i...
File dart/lib/compiler/implementation/patch_parser.dart (right):

https://chromiumcodereview.appspot.com/10870010/diff/5012/dart/lib/compiler/i...
dart/lib/compiler/implementation/patch_parser.dart:295: // Note: we never set
[annotatedElement] for this class.
Why not?

[ahe, 2012-08-23 08:11:55]

https://chromiumcodereview.appspot.com/10870010/diff/5012/dart/lib/compiler/i...
File dart/lib/compiler/implementation/patch_parser.dart (right):

https://chromiumcodereview.appspot.com/10870010/diff/5012/dart/lib/compiler/i...
dart/lib/compiler/implementation/patch_parser.dart:295: // Note: we never set
[annotatedElement] for this class.
On 2012/08/23 08:06:18, Johnni Winther wrote:
> Why not?

See line 294.

[ahe, 2012-08-23 08:30:14]

https://chromiumcodereview.appspot.com/10870010/diff/5012/dart/lib/compiler/i...
File dart/lib/compiler/implementation/patch_parser.dart (right):

https://chromiumcodereview.appspot.com/10870010/diff/5012/dart/lib/compiler/i...
dart/lib/compiler/implementation/patch_parser.dart:295: // Note: we never set
[annotatedElement] for this class.
On 2012/08/23 08:11:55, ahe wrote:
> On 2012/08/23 08:06:18, Johnni Winther wrote:
> > Why not?
> 
> See line 294. 

Perhaps this is no longer true.

[ahe, 2012-08-23 09:04:33]

https://chromiumcodereview.appspot.com/10870010/diff/5012/dart/lib/compiler/i...
File dart/lib/compiler/implementation/patch_parser.dart (right):

https://chromiumcodereview.appspot.com/10870010/diff/5012/dart/lib/compiler/i...
dart/lib/compiler/implementation/patch_parser.dart:295: // Note: we never set
[annotatedElement] for this class.
On 2012/08/23 08:30:15, ahe wrote:
> On 2012/08/23 08:11:55, ahe wrote:
> > On 2012/08/23 08:06:18, Johnni Winther wrote:
> > > Why not?
> > 
> > See line 294. 
> 
> Perhaps this is no longer true. 

I removed the comment in https://chromiumcodereview.appspot.com/10876036.