dart-lang / language

Design of the Dart language
Other
2.65k stars 201 forks source link

Macros: It is inconvenient to have to use Identifiers for everything, including what's inside the Dart SDK #3808

Open rrousselGit opened 4 months ago

rrousselGit commented 4 months ago

Hello!

It appears that at the moment, we don't have a way to manually define non-prefixed imports. This has the unintended consequence of having to use prefixes for everything, including int, Future, and more.

My default, it is possible to use what's inside dart:core in the global scope. But the problem is, this stops being possible as soon as we rely on user inputs.

Take a macro that generates a copyWith + hashCode. It may be used as:

@data
class Example {
  final int field;
}

And then, the generated code should be:

augment class Example {
  Example copyWith({int? field}) => ....;

  @override
  int get hashCode => Object.hash(...);
}

But the reality is, because Example.field is typed as int, that will cause the macro SDK to import dart:core using a prefix when we try to use its identifier to generate our copyWith.

As such, without hashCode, the code now looks like:

import 'dart:core' as prefix0;
augment class Example {
  Example copyWith({prefix0.int? field}) => ....;
}

But this has a nasty consequence: It is now no-longer feasible to use dart:core without relying in the prefix. As such, our previous hashCode override now has to be:

@prefix0.override
prefix0.int get hashCode => prefix0.Object.hash(...);

This significantly complexify the logic for writing the prototype of the hashCode override.

Proposal 1: Always globally import dart:core regardless.

Chances are that import 'dart:core' as prefix0 could be removed. This would allow to keep using the content of the Dart SDK without relying on identifiers.

Proposal 2: Allow defining custom non-prefixed imports

To simplify generating code that's not really dependent on how the macro is used, we could allow macros to manually import certain libraries. For example, a macro could manually import dart:async or a package.

Of course, this might require having one virtual file per unique macro , to avoid possible name conflicts.

scheglov commented 4 months ago

Do you worry that using declarations from dart:core is hard to do when you are implementing a macro, or that the code generated in result is full of import prefixes?

For removing import prefixes we had an implementation that started somewhere in here, but was turned off for now because CFE cannot yet support it.

rrousselGit commented 4 months ago

I'm mainly concerned about how the generated code is implemented.

Without this issue, we can do:

DeclarationCode.fromString('''
  @override
  int get hashCode => Object.hash(${fields.map((e) => e.name).join(', ')});
''');

With this issue, we have to do:

final override = await builder.resolveIndentifier(Uri.parse('dart:core'), 'override');
final integer = await builder.resolveIndentifier(Uri.parse('dart:core'), 'int');
final obj = await builder.resolveIndentifier(Uri.parse('dart:core'), 'Object');

DeclarationCode.fromParts([
  '@',
  override,
  '\n',
  integer,
  ' get hashCode => ',
  obj,
  '.hash(${...}'),
]);
scheglov commented 4 months ago

We have to use Identifier because there might be another macro, that you don't know of, applied to the same library, and it adds class int {}.

rrousselGit commented 4 months ago

I understand. But then I think we need a simpler way to deal with this.

For example, maybe string templates could support inlining identifiers?

Like:

DeclarationCode.fromString('''
  @{{override}}
  {{int}} get hashCode => {{Object}}.hash(...));
''');

And for package-specific stuff, we could have:

DeclarationCode.fromString('''
  final {{package:riverpod/riverpod.dart#Provider}} provider;
''');

edit: Just saw your message on Discord. I guess we're on the same line here

jakemac53 commented 4 months ago

Definitely agreed that we want both:

lrhn commented 4 months ago

If we don't do something, I'll expect someone to create a package with a coreNames that exposes all the public names of dart:core as identifiers, with a single async factory, and people using that. What means it looks up every name, whether it needs it or not, which can probably be (a little) bad for performance.

jakemac53 commented 4 months ago

If we don't do something, I'll expect someone to create a package with a coreNames that exposes all the public names of dart:core as identifiers, with a single async factory, and people using that.

Fwiw, even the JSON macro does essentially this via its own utility already. It is quite annoying, and does mean we resolve things we don't necessarily need to.

rrousselGit commented 4 months ago

I ended-up implementing custom string interpolation.

I have:

await resolve('{{Object}}.hash(...)'); // Alias for dart:code#Object
\\ '{{package:foo/foo.dart#Type}}' // self evident
\\ '{{.#Type}})' // Alias for package:my_package/my_package.dart#Type
\\ '{{./foo.dart#Type}}' // Alias for package:my_package/foo.dart#Type

await resolve(
  args: {'value': someIdentifier},
  "print('{{value}}');",  // optionally allow passing custom arguments.
);

So to generate (prefixes from snippet for simplicity):

@override
final Ref<UserDefined> ref;

I wrote:

builder.declareInType(
  await builder.resolve(
    args: {'RefT': userDefinedIdentifier},
    '''
  @{{override}}
  final {{.#Ref}}<{{RefT}}> ref;
'''),
);
davidmorgan commented 4 months ago

Thanks Remi! I've written a similar parser ... I expect we'll end up offering some nicer "happy path" here.

lrhn commented 4 months ago

One way to make identifier lookup not be asynchronous could be to have an Identifier that isn't looked up eagerly. Maybe it contains a future, or an ID representing a lookup result that hasn't necessarily completed yet, and the identifier can be combined into a Code object and sent back to the macro service before it ever completes. The requester can choose a unique ID for the request, start the async query, and return an object containing that ID. It can even fill in the object when the request ends, if it wants to, or keep the data in the macro system API layer.

If the name doesn't actually exist, you will get a late error, but you can keep going optimistically for names that your are certain of. There can still be a way to asynchronously validate the identifier (or a bunch of identifiers at a time, requests can be buffered and done in bunches).

The identifier can even be converted to a string, since the name was given, as long as the library import prefix is chosen eagerly. The macro system will then still have to remember to check that the declaration exists and is of the kind that is expected, so the macro API should remember that the lookup was made, and that the identifier was toString'ed and therefore presumably used.

Consider something like:

  var pathJoin = builder.library("package:path/path.dart").function("join");
  var coreHashMap = builder.library("dart:core").type("List").constructor("filled");

where type returns an Identifier that also implements an interface for types that may have members, allowing further lookup.

As long as the operations are synchronous, anyone can build an efficient helper API on top, like the templates above, or late final LibraryLookupHelper dartCore = LibraryLookupHelper(builder.library('dart:core')); ... var listFilled = dartCore["List.filled"];.

Not having an async step makes creating wrappers much simpler.

None of these lookups have to happen immediately, they can just choose a unique ID for the result, store that in the object, and

rrousselGit commented 4 months ago

Maybe we could support URIs as parts?

DeclarationCode.fromParts([
  'final ',
  Uri.parse('package:riverpod/riverpod.dart#Ref'),
  ' ref;',
]);

And the lookup would be performed at the time of writing the code, rather than at the time of creating the Declaration.

One benefit is, we could have a script that generates all URIs for the public APIs of a package ; to make it simpler to manipulate.

This could replace the deprecated resolveIdentifier.

jakemac53 commented 4 months ago

We could also just add a public constructor to the Identifier class, which takes a uri (probably as string) + name (+ scope?). Essentially, it encapsulates a resolveIdentifier call, but any failures happen lazily when the Code object is deserialized on the compiler end of things.

bivens-dev commented 4 months ago

Would this proposal help here? https://github.com/dart-lang/language/blob/main/working/tagged-strings/feature-specification.md

jakemac53 commented 4 months ago

That proposal will help in general with constructing code objects, and would also make it more elegant to create your own templates which do some sort of async identifier resolution such as what some people have already implemented.