Add support for generic type arguments in annotations #283

jacksonjude · 2022-07-20T01:46:20Z

Overview

Adds support for generic annotations, a feature added in Dart SDK 2.14.

Take this class for example:

@reflector
class Person {
  @TypeHelper<Soul>(42, "The meaning")
  String? life;
}

class TypeHelper<T> {
  const TypeHelper(int var1, String var2);
}

class Soul { }

The annotation would have previously been identified as having the type argument dynamic:

ClassMirror personClassMirror = reflector.reflectType(Person) as ClassMirror;
VariableMirror field = personClassMirror.declarations["life"] as VariableMirror;
var annotation = field.metadata[0];
print(annotation.runtimeType); // TypeHelper<dynamic>

It will now be correctly identified as having the type argument Soul:

print(annotation.runtimeType); // TypeHelper<Soul>

This is done by appending the type arguments (if any exist) after the annotation name during the _extractMetadataCode function of builder_implementation.dart.

google-cla · 2022-07-20T01:46:24Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

eernstg · 2022-07-21T16:22:42Z

Hello @jacksonjude, thanks for the PR! This is something reflectable should support. I'm really busy now, but I'll take a look as soon as possible.

jacksonjude · 2022-08-14T09:16:15Z

Hey there, sorry if you are still busy, but can you take another look at this?

eernstg · 2022-11-06T21:39:09Z

Sorry about the delay — yep, I'm still busy, but I will take a look at it tomorrow.

eernstg

Thanks again for the input! We need to make the code a bit more robust. Here is the beginnings of a test that is intended to verify that the new feature is working:

@c
library test_reflectable.test.metadata_test;

import 'package:reflectable/reflectable.dart';
import 'package:test/test.dart';
import 'metadata_test.reflectable.dart';

class MyReflectable extends Reflectable {
  const MyReflectable()
      : super(metadataCapability, instanceInvokeCapability,
            staticInvokeCapability, declarationsCapability, libraryCapability);
}

const myReflectable = MyReflectable();

const c = [
  Bar<num>({'a': 14})
];

class K {
  static const p = 2;
}

@myReflectable
@Bar<Object?>({
  b: deprecated,
  c: Deprecated('tomorrow'),
  1 + 2: (d ? 3 : 4),
  identical(1, 2): 's',
  K.p: 6
})
@Bar<Never>({})
@Bar<void>.namedConstructor({})
@c
class Foo {
  @Bar<bool>({})
  @Bar<Bar<Bar>>({})
  @Bar<MyReflectable>.namedConstructor({})
  @Bar<void Function()>.namedConstructor({})
  @c
  void foo() {}
  var x = 10;
}

class Bar<X> {
  final Map<Object, X> m;
  const Bar(this.m);
  const Bar.namedConstructor(this.m);

  @override
  String toString() => 'Bar<$X>($m)';
}

void main() {
  initializeReflectable();

  test('metadata on class', () {
    expect(myReflectable.reflectType(Foo).metadata, const [
      MyReflectable(),
      Bar<Object?>({
        b: deprecated,
        c: Deprecated('tomorrow'),
        3: 3,
        false: 's',
        2: 6,
      }),
      [
        Bar<num>({'a': 14})
      ],
    ]);

    var fooMirror = myReflectable.reflectType(Foo) as ClassMirror;
    expect(fooMirror.declarations['foo']!.metadata, const [
      Bar<bool>({}),
      Bar<Bar<Bar<dynamic>>>({}),
      Bar<MyReflectable>({}),
      Bar<void Function()>({}),
      [
        Bar<num>({'a': 14})
      ],
    ]);

    // The synthetic accessors do not have metadata.
    expect(fooMirror.instanceMembers['x']!.metadata, []);
    expect(fooMirror.instanceMembers['x=']!.metadata, []);

    // Test metadata on libraries
    expect(myReflectable.reflectType(Foo).owner!.metadata, [c]);
  });
}

The test probably needs to be corrected here and there (but it already shows that the new code gets a null reference error, so it's a useful starting point).

The test should be added to the repository at test_reflectable/test/generic_metadata_test.dart. The lower bound of the SDK constraint in test_reflectable/pubspec.yaml` should be changed to 2.17 (such that the generic-metadata feature is included in the language).

reflectable/lib/src/builder_implementation.dart

eernstg · 2022-11-07T18:21:31Z

reflectable/lib/src/builder_implementation.dart

+        String typeArguments = annotationNode.typeArguments!.arguments
+            .map((TypeAnnotation typeArgument) {
+              LibraryElement library = typeArgument.type!.element!.library!;
+              String prefix = importCollector._getPrefix(library);


For the prefix we need to check that the given library is importable (_isImportable). For instance, dart:_internal is not importable by user code.

I think I've resolved this, I currently have the type argument default to 'dynamic' if the library can't be imported, though I'm not sure if this is the correct behavior.

eernstg · 2022-11-07T18:24:38Z

reflectable/lib/src/builder_implementation.dart

+            .map((TypeAnnotation typeArgument) {
+              LibraryElement library = typeArgument.type!.element!.library!;
+              String prefix = importCollector._getPrefix(library);
+              return '$prefix$typeArgument';


I think this won't work in the case where the type argument is a parameterized type of the form C<D> where C is imported from prefix1 and D is imported from prefix2: We should get prefix1.C<prefix2.D>, but the code above simply uses toString() on the given TypeAnnotation, and that will yield syntax which is only valid for the location where that type annotation actually occurs in the program (and at this point we are writing some code that should denote the same type, but in a different library).

Check out _typeCodeOfTypeArgument, you might be able to call that function in order to get the right string.

Having some trouble calling _typeCodeOfTypeArgument since it is a function of _ReflectorDomain, which I can't seem to access from within _extractMetadataCode. However, it is very possible I may have missed something given that I'm not super familiar with the layout of the library.

Co-authored-by: Erik Ernst <eernst@google.com>

jacksonjude · 2022-11-08T09:16:16Z

Thanks again for the input! We need to make the code a bit more robust. Here is the beginnings of a test that is intended to verify that the new feature is working:

@c
library test_reflectable.test.metadata_test;

import 'package:reflectable/reflectable.dart';
import 'package:test/test.dart';
import 'metadata_test.reflectable.dart';

class MyReflectable extends Reflectable {
  const MyReflectable()
      : super(metadataCapability, instanceInvokeCapability,
            staticInvokeCapability, declarationsCapability, libraryCapability);
}

const myReflectable = MyReflectable();

const c = [
  Bar<num>({'a': 14})
];

class K {
  static const p = 2;
}

@myReflectable
@Bar<Object?>({
  b: deprecated,
  c: Deprecated('tomorrow'),
  1 + 2: (d ? 3 : 4),
  identical(1, 2): 's',
  K.p: 6
})
@Bar<Never>({})
@Bar<void>.namedConstructor({})
@c
class Foo {
  @Bar<bool>({})
  @Bar<Bar<Bar>>({})
  @Bar<MyReflectable>.namedConstructor({})
  @Bar<void Function()>.namedConstructor({})
  @c
  void foo() {}
  var x = 10;
}

class Bar<X> {
  final Map<Object, X> m;
  const Bar(this.m);
  const Bar.namedConstructor(this.m);

  @override
  String toString() => 'Bar<$X>($m)';
}

void main() {
  initializeReflectable();

  test('metadata on class', () {
    expect(myReflectable.reflectType(Foo).metadata, const [
      MyReflectable(),
      Bar<Object?>({
        b: deprecated,
        c: Deprecated('tomorrow'),
        3: 3,
        false: 's',
        2: 6,
      }),
      [
        Bar<num>({'a': 14})
      ],
    ]);

    var fooMirror = myReflectable.reflectType(Foo) as ClassMirror;
    expect(fooMirror.declarations['foo']!.metadata, const [
      Bar<bool>({}),
      Bar<Bar<Bar<dynamic>>>({}),
      Bar<MyReflectable>({}),
      Bar<void Function()>({}),
      [
        Bar<num>({'a': 14})
      ],
    ]);

    // The synthetic accessors do not have metadata.
    expect(fooMirror.instanceMembers['x']!.metadata, []);
    expect(fooMirror.instanceMembers['x=']!.metadata, []);

    // Test metadata on libraries
    expect(myReflectable.reflectType(Foo).owner!.metadata, [c]);
  });
}

The test probably needs to be corrected here and there (but it already shows that the new code gets a null reference error, so it's a useful starting point).

The test should be added to the repository at test_reflectable/test/generic_metadata_test.dart. The lower bound of the SDK constraint in test_reflectable/pubspec.yaml` should be changed to 2.17 (such that the generic-metadata feature is included in the language).

One more question: the variables b and d don't seem to be defined in the sample test code you have provided. Is this intentional?

eernstg · 2022-12-07T12:24:46Z

(No, I haven't forgotten this. Just busy again. Will try to get to it real soon now. ;-)

eernstg · 2023-01-30T10:38:04Z

(Busy, busy, busy, but I will take a look later today.)

codekeyz · 2023-12-27T00:45:25Z

@eernstg

Sten435 · 2024-02-07T20:24:00Z

@codekeyz @eernstg Any updates on this ?

It has been a year ?

jacksonjude · 2024-08-13T00:01:05Z

Hi again @eernstg, what changes need to be made for this PR to be ready to merge?

eernstg · 2024-08-22T14:32:25Z

Hi @jacksonjude, thanks for contributing! I'm sincerely sorry about not being able to get around to this for so long.

I took another look at it today, and I can see that it generates code where a prefix is missing when this test is executed:

@c
library test_reflectable.test.generic_metadata_test;

import 'package:reflectable/reflectable.dart';
import 'package:test/test.dart';
import 'metadata_test.reflectable.dart';

class MyReflectable extends Reflectable {
  const MyReflectable()
      : super(metadataCapability, instanceInvokeCapability,
            staticInvokeCapability, declarationsCapability, libraryCapability);
}

const myReflectable = MyReflectable();

const b = 0;

const c = [
  Bar<num>({'a': 14})
];

const d = true;

class K {
  static const p = 2;
}

@myReflectable
@Bar<Object?>({
  b: deprecated,
  c: Deprecated('tomorrow'),
  1 + 2: (d ? 3 : 4),
  identical(1, 2): 's',
  K.p: 6
})
@Bar<Never>({})
@Bar<void>.namedConstructor({})
@c
class Foo {
  @Bar<bool>({})
  @Bar<Bar<Bar>>({})
  @Bar<MyReflectable>.namedConstructor({})
  @Bar<void Function()>.namedConstructor({})
  @c
  void foo() {}
  var x = 10;
}

class Bar<X> {
  final Map<Object, X> m;
  const Bar(this.m);
  const Bar.namedConstructor(this.m);

  @override
  String toString() => 'Bar<$X>($m)';
}

void main() {
  initializeReflectable();

  test('metadata on class', () {
    expect(myReflectable.reflectType(Foo).metadata, const [
      MyReflectable(),
      Bar<Object?>({
        b: deprecated,
        c: Deprecated('tomorrow'),
        3: 3,
        false: 's',
        2: 6,
      }),
      [
        Bar<num>({'a': 14})
      ],
    ]);

    var fooMirror = myReflectable.reflectType(Foo) as ClassMirror;
    expect(fooMirror.declarations['foo']!.metadata, const [
      Bar<bool>({}),
      Bar<Bar<Bar<dynamic>>>({}),
      Bar<MyReflectable>({}),
      Bar<void Function()>({}),
      [
        Bar<num>({'a': 14})
      ],
    ]);

    // The synthetic accessors do not have metadata.
    expect(fooMirror.instanceMembers['x']!.metadata, []);
    expect(fooMirror.instanceMembers['x=']!.metadata, []);

    // Test metadata on libraries
    expect(myReflectable.reflectType(Foo).owner!.metadata, [c]);
  });
}

The error from the analyzer looks as follows:

  error • test/generic_metadata_test.reflectable.dart:66:45 • The name 'Bar' isn't a type, so it can't be used as a type argument. Try correcting the name to an existing type, or defining a type named 'Bar'. • non_type_as_type_argument

If I edit the generated code to add the missing prefix then I get 'Null check operator used on a null value'.

I think it shouldn't be hard to fix. It would be great to get it landed!

Add support for generic metadata annotations

e2c1291

Merge branch 'google:master' into master

0589b2b

eernstg requested changes Nov 7, 2022

View reviewed changes

jacksonjude and others added 5 commits November 7, 2022 21:53

Update reflectable/lib/src/builder_implementation.dart

89f905c

Co-authored-by: Erik Ernst <eernst@google.com>

Update reflectable/lib/src/builder_implementation.dart

43a5b25

Co-authored-by: Erik Ernst <eernst@google.com>

Fixed variable name conflict

cc88ab5

Added _isImportable check

17fc587

Added null check for typeArgumentType.element?.library

171e9ee

Merge branch 'google:master' into master

bb94778

Merge branch 'google:master' into master

004fc23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for generic type arguments in annotations #283

Add support for generic type arguments in annotations #283

jacksonjude commented Jul 20, 2022 •

edited

Loading

google-cla bot commented Jul 20, 2022

eernstg commented Jul 21, 2022

jacksonjude commented Aug 14, 2022

eernstg commented Nov 6, 2022

eernstg left a comment

eernstg Nov 7, 2022

jacksonjude Nov 8, 2022

eernstg Nov 7, 2022

jacksonjude Nov 8, 2022

jacksonjude commented Nov 8, 2022 •

edited

Loading

eernstg commented Dec 7, 2022

eernstg commented Jan 30, 2023

codekeyz commented Dec 27, 2023

Sten435 commented Feb 7, 2024

jacksonjude commented Aug 13, 2024

eernstg commented Aug 22, 2024

Add support for generic type arguments in annotations #283

Are you sure you want to change the base?

Add support for generic type arguments in annotations #283

Conversation

jacksonjude commented Jul 20, 2022 • edited Loading

Overview

google-cla bot commented Jul 20, 2022

eernstg commented Jul 21, 2022

jacksonjude commented Aug 14, 2022

eernstg commented Nov 6, 2022

eernstg left a comment

Choose a reason for hiding this comment

eernstg Nov 7, 2022

Choose a reason for hiding this comment

jacksonjude Nov 8, 2022

Choose a reason for hiding this comment

eernstg Nov 7, 2022

Choose a reason for hiding this comment

jacksonjude Nov 8, 2022

Choose a reason for hiding this comment

jacksonjude commented Nov 8, 2022 • edited Loading

eernstg commented Dec 7, 2022

eernstg commented Jan 30, 2023

codekeyz commented Dec 27, 2023

Sten435 commented Feb 7, 2024

jacksonjude commented Aug 13, 2024

eernstg commented Aug 22, 2024

jacksonjude commented Jul 20, 2022 •

edited

Loading

jacksonjude commented Nov 8, 2022 •

edited

Loading