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

Macros: Could ClassTypeBuilder have access to user-defined class methods? #3809

Open
rrousselGit opened this issue May 15, 2024 · 20 comments
Open
Labels
request Requests to resolve a particular developer problem static-metaprogramming Issues related to static metaprogramming

Comments

@rrousselGit
Copy link

rrousselGit commented May 15, 2024

Hello again!

It appears that there is no way for a ClassTypeBuilder to obtain the methods of the annotated class.
This is particularly problematic in my case, because I need to add a mixin to the class that's based on the return value of a method present in said class.

Consider:

@riverpod
class Foo {
  Model build();
}

I want to generate:

import 'model.dart' as prefix0;

// New type:
class FooProvider extends Provider<prefix0.Model> {...}

// Added a mixin to the type that depend s
augment class Foo with Notifier<prefix0.Model> {
  ...
}

The problem is, both the newly declared type and the mixin applied on the existing type depends on the return value of that build method (and the output is not constant. There's some logic around it).
So I need to find that build method in a phase that's able to add new types and mixin. But there doesn't seem to be a way to do so, as far as I know.

For information, it is an error if users do not define that build method. And I don't care about cases where that build method is coming from a different macro or a mixins or a subclass. That build method should always be directly specified by users.

@rrousselGit rrousselGit added the request Requests to resolve a particular developer problem label May 15, 2024
@rrousselGit
Copy link
Author

This is critical for being able to migrate Riverpod to macros.
Without a solution to this, I wouldn't be able to.

Maybe we could have ClassDeclaration list the user-defined methods ; while explicitly not supporting:

  • inherited methods from extends/with/implements
  • methods added through augmentation

@jakemac53
Copy link
Contributor

I do think it is a critical part of the macros feature that macros can compose well - and that means supporting other macros generating the build method in this case. So, that is the general reasoning for why this is not supported today. You would have to require users to encode this type in the class header somehow, probably by writing with Notifier<Model> in this case.

That being said it should be possible to allow an API which only gives you the user defined methods, or maybe we would specify it more specifically as the pre-augmentation members (which would also mean not including explicit hand written augmentations). From a technical perspective that should be safe to do.

@jakemac53 jakemac53 added the static-metaprogramming Issues related to static metaprogramming label May 15, 2024
@rrousselGit
Copy link
Author

rrousselGit commented May 15, 2024

probably by writing with Notifier in this case.

The whole point of this macro is so that users don't write this with, because there various rules behind which subclass to use based on external factors.
I have like 9 Notifier subclasses at the moment, with more to come to optimise performance for specific scenarios.
This puts a significant burden on the users to ask them to pick the right one themselves.

I do agree that is it valuable for macros is to be composable. But I don't think that should be a strong requirement for all macro APIs.

Otherwise I'd just not be able to upgrade my current syntax to macros at all, and would have to find an API that's likely worse for user (such as with code duplicates or more verbose).

@rrousselGit
Copy link
Author

rrousselGit commented May 15, 2024

Thinking about it, Freezed would have the same issue.
Because it needs to generate 2-3 classes per factory constructor on the annotated class. So we'd need to have the list of constructors in the type phase ; which we can't yet.

@rrousselGit
Copy link
Author

Alternatively, what is the reason why the "declaration" phase can't emit classes?
It can output variables, so I don't get why it can't output classes.

@jakemac53
Copy link
Contributor

jakemac53 commented May 15, 2024

Alternatively, what is the reason why the "declaration" phase can't emit classes?

Because it is allowed to resolve types to their declarations. If macros in this phase could emit new types, it could change how types resolve (maybe changing an error to a success, or maybe a success to a totally different declaration in the case of shadowing).

We try hard to eliminate the cases where a macro might ask a question and get a wrong (or inconsistent) answer. Sometimes, the answer can be incomplete (when asking for members in the declarations phase), but this is the one exception today.

@jakemac53
Copy link
Contributor

jakemac53 commented May 15, 2024

Are the generated types intended to be user visible? Or would they always be private to this library, and in particular private to code generated by this macro? We have considered, and I think maybe the proposal even mentions, the ability to emit macro-private code in later phases, that otherwise wouldn't be allowed. This code would also not be visible to other macros ever.

@rrousselGit
Copy link
Author

rrousselGit commented May 15, 2024

(I deleted my previous message, I think that was incorrect).

My classes may be public. But they aren't used by others. I'm supposed to be the sole consumer of those types, but they are technically visible.
This is because I want to generate a variable that's a public instance of those classes.

Given:

@riverpod
class Foo {
   int build();
}

We have:

const fooProvider = FooProvider<int>._(Foo._);

class FooProvider extends Provider<int> {
  // Only my macro can instantiate this new class, but it is public.
  const FooProvider._(this._create);

  [...]
}

augment class Foo with Notifier<int> {
  // Only my macro is allowed to instantiate the annotated class.
  Foo._(this.ref);

  @override
  final Ref<int> ref;
}

@rrousselGit
Copy link
Author

rrousselGit commented May 15, 2024

Fun fact: My macro can also be applied on functions, like so:

@riverpod
int foo() {

}

// generates:

const fooProvider = FooProvider<int>._(foo);

class FooProvider extends Provider<int> {
  // Only my macro can instantiate this new class, but it is public.
  const FooProvider._(this._create);

  [...]
}

But in this scenario, I have no issue. Because FunctionDeclaration.returnType is available.

But I need to support classes too.

@rrousselGit
Copy link
Author

I'm thing I'm currently looking into is doing:

@Riverpod<int>()
class Foo {
  int build();
}

Although I don't see a way to convert that generic into an Identifier.

And I quite dislike how this involves duplicating the type information.

@jakemac53
Copy link
Contributor

jakemac53 commented May 15, 2024

Yes generic type parameters won't work for macros because we can't take the types from the users program and materialize them in the macro program.

The intention is to pass these as regular arguments, and have them coerced into a TypeAnnotation implicitly, but that isn't implemented yet.

@rrousselGit
Copy link
Author

rrousselGit commented May 15, 2024

I see thanks.

With regards to removing the code duplication: Would it be possible to have users declare build without a return type, and have the augmentation add it?

@Riverpod(int)
class Foo {
   build() => ...;
}

augment class Foo {
  augment int build();
}

This doesn't work yet afaik. But I wonder if that would be reasonable? 🤔

@jakemac53
Copy link
Contributor

jakemac53 commented May 15, 2024

With regards to removing the code duplication: Would it be possible to have users declare build without a return type, and have the augmentation add it?

We don't allow augmentations to change the type of something pre-existing, so no we wouldn't allow this. Same reason we don't allow adding optional parameters. I get why that is limiting, but it is an intentional design choice for readability/usability (fwiw, allowing adding mixins/extends etc I think violates this principle but it was a compromise to allow it, in limited ways).

If all these classes are implementing a T build() method you could consider an interface which is explicit (and maybe you still have subtypes which are more specific but implement that?).

@Riverpod()
class Foo implements Thing<int> {
  build() => ...; // Gets the inferred return type `int`.
}

abstract interface class Thing<T> {
  T build();
}

@rrousselGit
Copy link
Author

rrousselGit commented May 15, 2024

If all these classes are implementing a T build() method you could consider an interface which is explicit (and maybe you still have subtypes which are more specific but implement that?).

No. The prototype of build isn't fixed.
Users can freely add arguments ; which the code-generator deals with.

In particular, the following is valid too:

@Riverpod();
class Foo {
  Future<int> build(int a, {required int named, String optional = 42}) {...}
}

Which then generates an object used as:

ref.watch(fooProvider(42, named: 42));

So there's no shared interface.

@rrousselGit
Copy link
Author

rrousselGit commented May 15, 2024

We don't allow augmentations to change the type of something pre-existing, so no we wouldn't allow this.

Although in this case, we're not replacing something existing. We're specifying something that wasn't specified yet.
That's the difference between OmmitedTypeAnnotation and an actual dynamic value.

Kind of like how we support:

@macro
int fn();

Then followed by an augmentation that adds a body.

(... although this is unimplemented. I assume this will be supported, right?)

@jakemac53
Copy link
Contributor

Omitting a type annotation on a function means it is inferred - and augmentations are not allowed to affect inference (by removing it through making the type explicit or altering the inferred type).

The only actual inference that can take place for return types of methods afaik is copying the type from the super declaration if one exists, otherwise you are getting dynamic.

In other words, while a user did not write a type there, that does not mean there is no type. They just didn't declare the type explicitly.

Similarly, augmentations cannot add types to parameters where one was not declared initially, etc.

@rrousselGit
Copy link
Author

Sounds like you're saying that there's no easy way forward.

Do you think it is realistic to expect ClassDeclaration to expose user-defined methods, even at the cost of composition in this case?

@jakemac53
Copy link
Contributor

I don't think we want to put things directly on the ClassDeclaration instance. But it is possible we could allow asking for members in the types phase, with the understanding that they will not be complete (which is already the case in the declarations phase anyways, to a lesser extent).

@rrousselGit
Copy link
Author

Awesome, thanks!
I really hope that this makes it through. Otherwise it'd be quite hard for my users to upgrade to macros, and the syntax would be worse overall.

@StarProxima
Copy link

to put things directly on the ClassDeclaration instance. But it is possible we could allow asking for members in the types phase, with the understanding that they will not be complete (which is already the case in the declarations phase anyways, to a lesser extent).

I was also experimenting with macros and came across this limitation.

The ability to get constructors, methods and fields, even if only user-defined, in the type phase seems pretty important. I think this is necessary to migrate users of some packages from build_runner to the macro system without pain

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
request Requests to resolve a particular developer problem static-metaprogramming Issues related to static metaprogramming
Projects
None yet
Development

No branches or pull requests

3 participants