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

Access to full AST for the immediate declaration #6

Open
jakemac53 opened this issue Jun 25, 2021 · 16 comments
Open

Access to full AST for the immediate declaration #6

jakemac53 opened this issue Jun 25, 2021 · 16 comments
Labels
enhancement New feature or request

Comments

@jakemac53
Copy link
Owner

At least in the definition phase we should consider exposing a Code object (or similar) for the immediately annotated declaration. For a class this could include all the members of the class.

cc @davidmorgan can you expand a bit on the use cases for this?

@jakemac53 jakemac53 added the enhancement New feature or request label Jun 25, 2021
@davidmorgan
Copy link

It's possible that the needs for AST mostly don't apply with macros.

The first use case is validating integration points.

e.g. if for class Foo we generate implementation _$Foo, then the constructor of Foo needs to be a factory constructor that delegates to _$Foo.

Typically in these cases we can already generate the exact string we want to appear in the code at that point, and just do a string compare, issuing an error "please change to ...string..." if it doesn't match.

The second use case is for figuring out how to refer to types in a way that works in context (for generated part files)--i.e. we need the name with the import prefix. I think I read that macros will handle references to types specially anyway to support this?

Thanks.

@jakemac53
Copy link
Owner Author

e.g. if for class Foo we generate implementation _$Foo, then the constructor of Foo needs to be a factory constructor that delegates to _$Foo.

There would be essentially no integration points here, you could generate this delegating constructor :).

The second use case is for figuring out how to refer to types in a way that works in context (for generated part files)--i.e. we need the name with the import prefix. I think I read that macros will handle references to types specially anyway to support this?

Yes this isn't fully fleshed out yet, but its a well known issue and we have some general idea of what we want to do. I can't replicate it in this prototype though as it will involve some special magic in the compilers.

@davidmorgan
Copy link

SG, thanks :)

@TimWhiting
Copy link
Contributor

I tried replicating part of the freezed code generator using the macro system, but got stuck here. Freezed uses a user defined factory constructor to generate a data class / union class by the name of the redirected constructor. Problem is that the constructor macro does not have access to the redirected name, even though it is in the AST.

@jakemac53
Copy link
Owner Author

I tried replicating part of the freezed code generator using the macro system, but got stuck here. Freezed uses a user defined factory constructor to generate a data class / union class by the name of the redirected constructor. Problem is that the constructor macro does not have access to the redirected name, even though it is in the AST.

Can you replicate the same behavior in another way? If you can give a more concrete example I might be able to provide some suggestions.

Note that we don't yet allow reading of metadata on members, but plan to.

@TimWhiting
Copy link
Contributor

TimWhiting commented Jul 21, 2021

The use case is enumerated data classes

@freezed
class EnumClass {
  factory EnumClass.data(String data) = EnumData;
  factory EnumClass.loading(int seconds) = EnumLoading;
  factory EnumClass.error(String err, StackTrace st) = EnumError;
}

=> generates three data classes (with copyWith, constructors, fields, ==, hashCode etc)...

/// Full dataclass (full implementation omitted for brevity)
@sealed
class EnumData extends EnumClass {
  EnumData(this.data);
  String data;
}
@sealed
class EnumLoading extends EnumClass {
  EnumData(this.seconds);
  int seconds;
}
@sealed
class EnumError extends EnumClass {
  EnumData(this.err, this.st);
  String err;
  StackTrace st;
}

along with extension methods for switch case on an EnumClass and handle each of the variants.... (omitted here but explained in the freezed documentation https://pub.dev/packages/freezed#when)

I guess you could annotate each factory with a String

class EnumClass {
  @enumclass('EnumData')
  factory EnumClass.data(String data) {}
  @enumclass('EnumLoading')
  factory EnumClass.loading(int seconds) {}
  @enumclass('EnumError')
  factory EnumClass.error(String err, StackTrace st) {}
}

But this is more verbose, requires an empty body for the factory which would have to be changed or implemented via the macro causing more confusion on what is going on, and also uses Strings which are not ideal. The empty body is required for the current build_runner based macro system from my previous attempt at this.

It would be ideal to be able to access the unresolved AST for the constructors from a ClassDeclarationMacro. This way even though the class redirections are not valid to begin with, the class constructors they tear off can be generated during the Declaration phase using the unresolved identifiers in the redirecting factories.

@jakemac53
Copy link
Owner Author

I do generally agree this would be another nice use case for allowing some inspection of the AST. Some other comments below.

I guess you could annotate each factory with a String

Right, so for this example I would suggest inferring the name based on the name of the factory and the base class, and then allowing you to override it with an annotation similar to what you showed.

requires an empty body for the factory which would have to be changed or implemented via the macro causing more confusion on what is going on

You can mark the factory as external external factory EnumClass.data(String data);. That is probably what I would suggest in this case, as it is more clear that this is a generated thing.

@TimWhiting
Copy link
Contributor

TimWhiting commented Jul 21, 2021

Sometimes the user would like to have the name be different from the factory, something shorter usually, or occasionally without the name of the base class. Obviously this could be done via options in the annotation parameters, but looks less 'darty' and more magic and String based (which just reminds me of C macros).

So I guess it is just another use case for the AST inspection to require fewer annotations (1 per base class rather 1 per factory), but doesn't make it impossible to implement the enumerated classes in the current macro system.

The advantage of AST inspection means that you can have fewer magic strings and utilize more of what the programmer has already written to ensure valid programs with raw unresolved identifiers. If you could inspect the AST of the macro arguments as well it could solve the issues with raw identifiers instead of strings in #23 (comment) as well . The nim programming language has a concept of untyped versus typed versus static arguments for their macros. https://nim-lang.org/docs/tut3.html. TLDR; untyped arguments just have to be parseable and are not semantically checked or resolved or expanded, and are passed to the macro as a syntax tree. Typed are similar but are resolved. Static are passed as their value.

In Dart that might look like:

class MyUntypedMacro implements ClassDeclarationMacro {
  MyUntypedMacro(UntypedExpression ast, TypedExpression typedAst, int value);
  void visitClassDeclaration(
      ClassDeclaration declaration, ClassDeclarationBuilder builder){
   }
}
MyUntypedMacro((1 + 2), SomeClass.someMethod, 10);

which could enable:

class EnumClass {
  @enumclass(EnumData)
  external factory EnumClass.data(String data);
  @enumclass(EnumLoading)
  external factory EnumClass.loading(int seconds);
  @enumclass(EnumError)
  external factory EnumClass.error(String err, StackTrace st);
}
// or potentially
class EnumClass {
  @enumclass((String data) => EnumData)
  @enumclass((int seconds) => EnumLoading)
  @enumclass((String err, StackTrace st) => EnumError)
}

Where the macro system encounters an UntypedExpression or TypedExpression in the macro constructor it would hand the unresolved or resolved ast when actually constructing the macro instance, whereas the exact value 10 gets passed into the constructor since it is neither an Untyped or Typed expression. This would obviously be limited to Expressions. From the user's point of view, a TypedExpression or UntypedExpression argument of a macro is essentially typed as dynamic

As far as analyzing the surrounding / annotated AST (not the macro arguments AST) it would be nice if that was available via the ClassDeclaration class in visitClassDeclaration which enables the original freezed syntax at the top of this issue.

@rrousselGit
Copy link
Contributor

I disagree with the proposals of using an annotation for the naming and using external factory.

These are syntax downgrades, and a significant breaking change to existing Freezed users.

@TimWhiting
Copy link
Contributor

TimWhiting commented Jul 21, 2021

@rrousselGit
I agree that I would rather have the current syntax of Freezed, instead of this external factory. Added a paragraph at the end of my prior comment, suggesting the same. Just arguing that the more access we have the the AST the more powerful we can make the macros.

For example with being able to use an unresolved expression AST directly in the macro constructor we could enable functionalWidgets like this:

/// Macro that is a topLevel annotation (would need to enable top level macros or use it to annotate an empty class that becomes the Widget class)
class FunctionalWidget implements TopLevelMacro {
  FunctionalWidget(UntypedExpression className, TypedExpression buildMethod){
     // TODO: Check that className is an identifier and nothing more
     // Generate a widget class with className identifier
     // Add build method to widget class that is just buildMethod but altered to remove the extra parameters other than BuildContext
     // Add constructor to widget class that takes in the extra parameters in the build method
  }
}


// Usage
@FunctionalWidget(MyClassName, (BuildContext context, String param){
   return Text(param);
});

This solves the problem of having a private named function declaration that turns into a public class.

@functionalWidget
_functionalWidget(BuildContext context, String param) {

}

@rrousselGit
Copy link
Contributor

@TimWhiting No worries. I was actually responding to @jakemac53. It's my bad that my message wasn't clear.

Your idea is quite an interesting. I like it.
Although your functional widget proposal has some extra issues: functions within annotations are not allowed.

@TimWhiting
Copy link
Contributor

TimWhiting commented Jul 21, 2021

@rrousselGit
You missed the part where the macro constructor has the second argument typed as a TypedExpression ast rather than a Function. The macro never sees the function, just the AST representation of the function according to my proposal. Of course this isn't possible right now, because none of my proposal is possible right now, it requires good integration with the compiler / analyzer / frontend. The macro system has to first compile the macro and then for any Typed / UntypedExpression argument hand the macro constructor for annotations a resolved / unresolved AST instead of actually compiling the argument. Also would be good to have the statement level macros discussed in #24

@rrousselGit
Copy link
Contributor

You missed the part where the macro constructor has the second argument typed as a TypedExpression ast rather than a Function. The macro never sees the function, just the AST representation of the function according to my proposal

I am aware of this, but it still is problematic: breakpoints won't work.

Users would naturally place their breakpoints in the annotation. But the generated code would never execute the function in the annotation and instead would use clone of the function.

Without something like #13, this could be confusing.

@TimWhiting
Copy link
Contributor

If you used the original AST in the output (by wrapping the function in a build method that passes in the parameters to the original function) maybe an automatic sourcemap could be produced, but #13 would make it more flexible. Agreed that breakpoints would be placed by users in the annotation AST, which would need to be linked to where the generated code uses that statement / line.

@TimWhiting
Copy link
Contributor

Having the AST for the immediate declaration (unresolved / partially resolved / fully resolved depending on the phase of the macro execution) would also solve #7 and #21.

@scheglov
Copy link

FWIW, in the experiment in the analyzer I give macros a complete AST node, e.g. FieldDeclaration that has its parent set, and so it is possible to access the enclosing class or mixin, or even look at sibling top-level declaration in the same unit (but not the library - no access using AST, but theoretically could be done), etc. For declaration phase macros type hierarchies are already built, and all explicit type annotations for source declarations (and previously generated by other declaration macros) are resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants