Skip to content
This repository has been archived by the owner on Feb 4, 2025. It is now read-only.

Macro API design discussion #91

Closed
jakemac53 opened this issue Oct 9, 2024 · 4 comments
Closed

Macro API design discussion #91

jakemac53 opened this issue Oct 9, 2024 · 4 comments

Comments

@jakemac53
Copy link
Contributor

jakemac53 commented Oct 9, 2024

I wanted to discuss the overall design of the macro APIs that people are coding against. Ultimately, my goal is to move them much closer to the existing APIs with the exception of the introspection APIs.

Some specific changes I want to make:

  • Remove the description field entirely from the macro class, and move to the macro interfaces we currently have - essentially wholesale copy these apis, but with tweaks to the transitively exposed types there.
  • Add Builder APIs for how you actually perform augmentation, replacing the AugmentationResponse and Augmentation classes we have today. The AugmentationResponse would change how it is structured to match something more like the MacroExecutionResult.
    • This allows us to easily merge augmentations together like we currently do (without parsing them), which makes the resulting files look nicer when many macros are running.
    • It keeps developers on the rails, so they don't try to produce augmentations they aren't allowed to based on their current phase.
    • It handles the hard parts of producing augmentation declarations for you (this could be done in a separate helper possibly though).
    • Lastly, it is much easier for us to validate that the returned augmentations are not violating any rules.

cc @davidmorgan maybe we should discuss tomorrow am?

@jakemac53
Copy link
Contributor Author

Another area we need to evaluate is the Query API and how we want to make that slot into the phases, but I don't have a proposal for it.

@davidmorgan
Copy link
Contributor

Thanks for pushing on this Jake ... there is a surprisingly large amount to unpack here :)


Re: "description field".

With Macro application annotation identification we haven't fleshed out how the host finds out implementation detail of the macro, such as phases.

What we currently have in v2 is that macro implementation detail is sent by the macro on startup in MacroStartedRequest.

So to support fine-grained interfaces per the v1 API we would have:

  • _macro_builder analyzes the macro code to get the set of interfaces implemented, injects this into the bootstrap script so it gets added to MacroStartedRequest, along with the set of phases needed
  • the host can now check whether where the macro was applied matches the set of places the macro knows how to handle
  • AugmentRequest gets a new field, the target type
  • _macro_client dispatches based on target type and phase to the fine-grained interfaces

and then we have a few more questions about how this whole thing ties together:

  • v1 macro gets its own metadata through the constructor, how does v2 get it?
  • v1 macro gets the target Declaration passed in, what does v2 get?
  • v1 gets a builder: this is a new type of thing we probably want for v2

Metadata and Declaration are mostly interesting for the question of how they interact with queries and the Model. Possibly what a v2 macro gets is a default initial query and it immediately gets passed a Model which is the result of that query. But then the macro hasn't had a chance to specify the details of that query, e.g. whether to fetch all the types mentioned. So that's not 100% clear.

Another consideration here is that I think the Macro interface is tied to the major protocol version: when the major protocol version changes the Macro interface becomes Macro2 and all the types change. So the macro_client dispatch to macro methods has to be able to do this. (Should be easy enough, particularly since we fully control the generation of the bootstrap code).

I think a good path is to push for one e2e example that works like a v1 macro, it's not clear to me that we're headed for something exactly like v1 macros, but if there are going to be any differences then we can figure out what+why along the way.


Builder APIs: yes, I think we want these, and yes to flushing out AugmentationResponse correspondingly.

Just one note here. If I understood correctly, builders are code running on the client that helps to create Dart code. That's fine, but we should think about forwards compatibility: what happens when a new language version requires something different? We might need a way to introduce new code paths for new language versions.

Again I think probably some simple e2e example to prove out the new ground here is probably a good way to go.


Indeed, let's discuss :) thanks!

@jakemac53
Copy link
Contributor Author

jakemac53 commented Oct 10, 2024

Just one note here. If I understood correctly, builders are code running on the client that helps to create Dart code. That's fine, but we should think about forwards compatibility: what happens when a new language version requires something different? We might need a way to introduce new code paths for new language versions.

It is a combination of client + server side right now. The client side only ever generates a single declaration, and not any surrounding declarations (but will have a reference to which surrounding declaration it is generating it for). The server side then merges all those declarations together from all the macros, under a single augmentation of the surrounding declaration.

We could move literally all of it server side though, if that would be better. It might even be a requirement, because if the queries are not complete, the client might not have the required information to generate the full declaration.

jakemac53 added a commit that referenced this issue Oct 11, 2024
Part of #91

Sending this out a bit early - this is I think the smallest chunk I could tackle independently, but it would be nice to get a review before you leave.

The next step would be passing the actual target through to the macro api, probably instead of the AugmentRequest.
@jakemac53
Copy link
Contributor Author

jakemac53 commented Oct 21, 2024

Copying a comment from #104, we decided on the following additional things regarding builder objects:

  • Make the API for macros only take a builder object as a parameter, with access to the target model via a getter
    • This will also auto merge the results of any queries into the model that is in scope.
    • It also opens up the door to potentially trying to answer queries with the existing model in scope when all data is present, and/or converting queries to minimal ones when additional data is needed.
  • Make the macro methods return FutureOr<void>.
  • Add some convenience methods on QualifiedName to look it up in the model because this will be generally useful.

@davidmorgan davidmorgan closed this as not planned Won't fix, can't repro, duplicate, stale Feb 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

2 participants