dart-lang / language

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

Macros: Allow learning instantiation of types #3589

Open simolus3 opened 7 months ago

simolus3 commented 7 months ago

The prototype macros API exposes the StaticType class for resolved types. That supports some basic operations such as subtype checking, but it doesn't expose any type parameters for interface types, and it has no method to instantiate a type to another one.

For example, consider a JSON serializable variant that may support explicit serializers for some fields:

@JsonSerializable
class User {
  final String name;
  @JsonKey(serializeWith: LocaleSerializer())
  final Locale locale;
}

class LocalSerializer extends Serializer<Locale, String> {}

When being applied, the macro sees the actual LocalSerializer type. To generate code, it might want to know that this type:

  1. It is a subtype of Serializer (already possible with StaticType).
  2. That, more specifically, it is a subtype of Serializer<Locale, String>. This is not exposed by the API today.

This seems to be implemented in https://dart-review.googlesource.com/c/sdk/+/250244/2/pkg/_fe_analyzer_shared/lib/src/macros/api/introspection.dart. cc @jakemac53

simolus3 commented 7 months ago

I've experimented with this in https://dart-review.googlesource.com/c/sdk/+/348920

jakemac53 commented 7 months ago

Note that you can create a StaticType with type arguments, the resolve method accepts any TypeAnnotationCode object, so you can represent any type.

But, you can't actually get at those type arguments once the StaticType is created, or get at a specific supertype with its instantiated type arguments, etc, which is what it looks like you are adding here.

It is worth mentioning a previous attempt/approach I had which was here https://dart-review.googlesource.com/c/sdk/+/250244. I do think the approach you have is better though, as it avoids re-implementing potentially very complicated type operations, at the cost of making them async operations, but I think that is fine.

Should we consider making the resolve function generic so you can specify the kind of StaticType you expect to get back?

We also probably want to look at all the other subtypes of DartType in the analyzer, and make the corresponding equivalents here, as appropriate.

simolus3 commented 7 months ago

Should we consider making the resolve function generic so you can specify the kind of StaticType you expect to get back?

I think users can always cast or pattern match over the different StaticTypes, so personally I don't really see the benefit here. Even if we have a NamedTypeAnnotationCode, it's not obvious that we'll get a NamedStaticType back because the code could reference a type alias to something else.

We also probably want to look at all the other subtypes of DartType in the analyzer, and make the corresponding equivalents here, as appropriate.

Definitely, should I try doing that in the same CL?

I'm not sure if you've seen my comment on the CL, so I want to bring this up again: One issue that came up in the implementation is that StaticType subtypes are, as it seems, the first objects in the macro API that both describe things (like the declaration of a named type) and have remote-callable methods (like subtype checks). This leads to awkward stubs that always throw in introspection_impls.dart. It will also require the analyzer and CFE implementation of those types to keep references to a bunch of things that don't really belong to the type (like TypeSystem or CoreTypes, respectively).

If I haven't overlooked anything, I think the current approach is also incorrect because the CFE / analyzer type impls may get garbage collected while still having a remote-id based reference on the other end. That's another problem we wouldn't have if StaticTypes were just a stateless description of the underlying type, similar to TypeDeclarations.

Do you think it might make sense to move isSubtypeOf, isExactly and asInstanceOf to the DeclarationPhaseInterceptor instead? StaticType and the different subtypes could then be simple and stateless objects.

jakemac53 commented 7 months ago

Definitely, should I try doing that in the same CL?

I generally prefer smaller more targetted cls, so I would probably do it separately. It is more that as I was reviewing this stuff I noticed that we were missing things for function types etc.

Ideally, one CL per new static type.

jakemac53 commented 7 months ago

I'm not sure if you've seen my comment on the CL, so I want to bring this up again: One issue that came up in the implementation is that StaticType subtypes are, as it seems, the first objects in the macro API that both describe things (like the declaration of a named type) and have remote-callable methods (like subtype checks).

It is a bit awkward, yes. It just seemed the most intuitive user-facing API at the time. But we could instead make these all methods on the builder classes, which take a left/right type as arguments.

If I haven't overlooked anything, I think the current approach is also incorrect because the CFE / analyzer type impls may get garbage collected while still having a remote-id based reference on the other end. That's another problem we wouldn't have if StaticTypes were just a stateless description of the underlying type, similar to TypeDeclarations.

This won't happen because for any remote instance, there is a local cache, which will keep them from getting garbage collected.

Basically at the start of each macro application (or phase maybe, can't remember?), we create a "serialization zone" which holds a cache of everything it has ever sent between the macro and the host (compiler, analyzer). This allows us to just send IDs back and forth as much as possible, and also ensures nothing gets garbage collected as long as the zone is alive.

The cache is specific to the zone, and it gets cleaned up only once the entire operation is done (so the host/client are done talking).

The exact lifecycle of these zones is up to the host implementation, they tell the client when to clean up the corresponding remote zones etc (see DestroyRemoteInstanceZoneRequest). New remote instance zones are IIRC implicitly created when a new ID is seen, and all requests have a zone ID attached.

jakemac53 commented 7 months ago

Do you think it might make sense to move isSubtypeOf, isExactly and asInstanceOf to the DeclarationPhaseInterceptor instead? StaticType and the different subtypes could then be simple and stateless objects.

Basically, the justification for not doing it this way today is that I just think it isn't the best user facing API. An API that takes two types, is more confusing because you have to think about which of the types is the "left" type vs the "right" type. Where typeOne.isSubtypeOf(typeTwo) is more obvious imo.