Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow generic metadata annotations #1297

Closed
ghost opened this issue Nov 9, 2020 · 17 comments
Closed

Allow generic metadata annotations #1297

ghost opened this issue Nov 9, 2020 · 17 comments
Assignees
Labels
small-feature A small feature which is relatively cheap to implement.

Comments

@ghost
Copy link

ghost commented Nov 9, 2020

Feature specification: https://github.com/dart-lang/language/blob/master/accepted/2.14/small-features-21Q1/feature-specification.md


Annotations (metadata) aren't allowed to use type arguments, but it is allowed to make annotations using a const of a type that takes a type argument:

class MyAnnotation<T> {
  const MyAnnotation();
}

const MA = MyAnnotation<String Function(int)>();

@MA  // <- Works.
random(int weather) => '7';

@MyAnnotation<String Function(int)>()  // <- Fails.
alsoRandom(int lucky) => '7';

main() => random(0) + alsoRandom(13);

Gives:

../../annotation_type_generic.dart:10:14: Error: An annotation (metadata) can't use type arguments.
@MyAnnotation<String Function(int)>()
             ^

See also: dart-lang/sdk#44120

@ghost
Copy link
Author

ghost commented Nov 9, 2020

+CC: @mkustermann, @lrhn

@eernstg
Copy link
Member

eernstg commented Nov 9, 2020

This would certainly make sense, and we've discussed it already a long time ago. I'll move this issue to the language repository where it fits better.

@eernstg eernstg transferred this issue from dart-lang/sdk Nov 9, 2020
@mkustermann
Copy link
Member

@eernstg @lrhn It might be really helpful if we could get this soon due to a use case in the VM. The constant evaluator is now in the CFE and shared across backends, so it might not be very hard to support this.

@eernstg eernstg added the small-feature A small feature which is relatively cheap to implement. label Nov 9, 2020
@eernstg
Copy link
Member

eernstg commented Nov 9, 2020

Maybe you could say a bit more about that use case in the VM and why it is important?

@mkustermann
Copy link
Member

Maybe you could say a bit more about that use case in the VM and why it is important?

Sure. We currently have VM-specific syntax in Dart files for specifying natives. That looks e.g. like this:

double sqrt(double arg) native "Math_sqrt";

This uses our old, heavy weight runtime call mechanism.

Now we would like to make it possible to use the new Dart FFI for such natives instead. Possibly even replacing all of our old native calls with faster FFI natives.

Ideally we would do this in a declarative way, just as our natives before.

Though we will need to have a way to specify not only the name of the C function (as above) but also it's signature in C code. One option would be to make them look like this:

import 'dart:ffi';

@Native<Float Function(Float)>("Math_sqrt")
extern double sqrt(double arg);

That would remove the VM-specific native "foo"-syntax as well as allowing us to use declaratively specific FFI natives with the corresponding C function signature.

@eernstg
Copy link
Member

eernstg commented Nov 9, 2020

OK, thanks! It would still be possible to use

const native431 = Native<Float Function(Float)>("Math_sqrt");
@native431
extern double sqrt(double arg);

but it is obviously more convenient to avoid the intermediate constant variable, and the need to invent a name for it.

@johnniwinther
Copy link
Member

From the CFE point of view it seems we can support this by just not emitting the error, i.e. by removing one 3-line if-statement. This would also remove the error from the analyzer, but I don't know if more work is needed on the analyzer side. @scheglov Do you if more work is needed on the analyzer side?

@scheglov
Copy link
Contributor

Yes, it will require some work in the analyzer. The class Annotation does not have typeArguments yet, only arguments that does not have type arguments in it (in contrast to kernel's Arguments). Then evaluation will require some changes. Nothing hard though.

@mkustermann
Copy link
Member

@leafpetersen @munificent @lrhn Could you discuss this in next language meeting? Making this small change would get us unblocked.

@leafpetersen
Copy link
Member

This seems fine by me. I think this would be a good candidate for the small and useful features release - it would be good to have it guarded by a language version to avoid breaking on old SDKs.

@ghost
Copy link
Author

ghost commented Nov 30, 2020

What kind of timeline are we roughly looking at for this to land?

@mit-mit
Copy link
Member

mit-mit commented Dec 9, 2020

@cskau-g @mkustermann is the workaround Erik described sufficient to get you started?

@mkustermann
Copy link
Member

Is the workaround Erik described sufficient to get you started?

We knew about the workaround even before filing the issue and of course it works for prototyping. Though I think we should not go ahead start mass migrating dart:* libraries using this hack.

@mit-mit Is there any concrete reason for not just going ahead with this little change: putting it into the spec and getting it implemented?

@mit-mit
Copy link
Member

mit-mit commented Dec 9, 2020

No, we're going to try and build a tentative plan for this. I was just checking.

@mit-mit mit-mit added this to Being discussed in Language funnel Dec 9, 2020
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Feb 3, 2021
See implementation plan PR:
dart-lang/language#1417

Change-Id: Ib75026d7008d313f3dad94d9ccf47e341f945dc9
Bug: dart-lang/language#1297
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/182301
Reviewed-by: Lasse R.H. Nielsen <lrn@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Feb 5, 2021
Currently we don't support generic metadata, but we'll be adding
support soon (dart-lang/language#1297).
When we add it, we'll still want to have tests to make sure that it's
not allowed in older language versions.

The tests we currently have to make sure it's not allowed only test
annotations occurring outside of function bodies, however the front
end code paths for handling code inside function bodies are
sufficiently different that it makes sense to test inside function
bodies too.

Bug: #44838
Change-Id: I846f2fc2e5ae090f21744ed6407b21a48a6d08ff
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/182601
Reviewed-by: Bob Nystrom <rnystrom@google.com>
@leafpetersen leafpetersen moved this from Being discussed to Being implemented in Language funnel Feb 9, 2021
@leafpetersen
Copy link
Member

This is available in beta, and will be in the next stable.

@hellohuanlin
Copy link

hellohuanlin commented Apr 20, 2022

For some reason I am still seeing this issue when building flutter engine:

$  ninja -C out/host_debug_unopt
../../flutter/third_party/tonic/tests/fixtures/tonic_test.dart:152:13: Error: An annotation can't use type arguments.
  @FfiNative<Handle Function(Pointer<Void>, Int64)>(

I have tried both --prebuilt-dart-sdk and --no-prebuilt-dart-sdk and neither worked 😦

@munificent
Copy link
Member

I can't repro the problem with a local build of the Dart SDK. My guess is that this is something specific to how the Flutter engine is setup or which version of the Dart SDK it has (though I'm fairly certain it should have this feature by now). I think you'll probably have better luck asking on the Flutter team if they've seen this problem, and I suspect they won't see the comment on this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
small-feature A small feature which is relatively cheap to implement.
Projects
Status: Done
Development

No branches or pull requests

8 participants