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: Consider doing inference for fields when possible #2154

Closed
jakemac53 opened this issue Mar 10, 2022 · 7 comments · Fixed by #2160
Closed

Macros: Consider doing inference for fields when possible #2154

jakemac53 opened this issue Mar 10, 2022 · 7 comments · Fixed by #2160
Labels
static-metaprogramming Issues related to static metaprogramming

Comments

@jakemac53
Copy link
Contributor

jakemac53 commented Mar 10, 2022

Currently we say that no inference is done until after macros are completed - this is largely because we may have incomplete or non-existent bodies during macro compilation.

Today we were discussing some potential failure modes of this approach. In particular, it means that it would be possible for a library to break its downstream users by adding or removing a type annotation from a field (or return type of a method), whose type is inferred. In general this should not be a breaking change, but it will be with macros since the type will now be invisible to the macros.

Here are some random ideas that could help, in some combination:

  • Allow you to see the results of inference for dependencies, so that you won't be broken if they add or remove "unnecessary" types.
    • This likely implies requiring that all phases of macros are run on dependencies prior to running phase 2 or 3 macros on the current library, so that type inference can take place.
    • You can still break yourself, but you will know if you do that.
  • Specifying that macros cannot change the inferred type of a field or return type of a method when augmenting it.
    • If a field has an initializer, the type of the field is based on inference of the original initializer
    • If a field has no initializer or a method has no body, adding either of those in a macro does not contribute to type inference. The field or method return type will be dynamic unless otherwise specified.
@jakemac53 jakemac53 added the static-metaprogramming Issues related to static metaprogramming label Mar 10, 2022
@jakemac53
Copy link
Contributor Author

@jakemac53
Copy link
Contributor Author

This is also related to #2151

@scheglov
Copy link
Contributor

FWIW, the analyzer always links libraries cycle by cycle, so dependencies are always fully linked when we start a new library cycle, including any inferred typed.

@jakemac53
Copy link
Contributor Author

Good to know - that was our assumption. I believe the CFE also works this way but it would be good for @johnniwinther to confirm.

Given that we are adding some asynchronous behavior to compiling a library I do worry a little bit that compilers might want to change that model, and this change would not allow them to. In a fully synchronous compiler there is no downside to doing it that way but in an asynchronous one you might want to do some other work while waiting on a macro to run (this could also reduce the overhead of macros in general).

@munificent
Copy link
Member

  • This likely implies requiring that all phases of macros are run on dependencies prior to running phase 2 or 3 macros on the current library, so that type inference can take place.

I think we already require that all phases of macros have run on dependencies outside of the current library cycle, because those dependencies may also contain macro declarations that need to be fully compiled so that the current library can apply them.

So as far as I can tell, this requirement shouldn't be any more stringent than what we already expect.

@jakemac53
Copy link
Contributor Author

So as far as I can tell, this requirement shouldn't be any more stringent than what we already expect.

It just has potential impacts for the parallelism of the build. While for a macro dependency yes the macros would be ran to completion, for other dependencies they don't have to be. So you could get some parallelism if you wanted to today. Requiring it removes that possibility.

@jakemac53
Copy link
Contributor Author

jakemac53 commented Mar 15, 2022

We discussed this in our weekly meeting today, and at least for now are going to explore the following solution:

  • Add a new subtype of TypeAnnotation which represents an inferred type. This will have no identifiers or other information, it is just a placeholder.
  • You will be allowed to use these inferred types in your Code objects in phase 1 & 2, and the type they emit will be equivalent to the eventual inferred type. However, it will fail if the inferred type is not something you can reference (for example a private type of some other library).
  • In phase 3, add an api to resolve these inferred type annotations explicitly to a concrete type annotation.

Note that the Code api will need to be slightly adjusted to allow for these instances, currently it does not accept type annotations directly at all.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Mar 18, 2022
Bug: dart-lang/language#2154
Change-Id: Ie48d308a822798da22ea38954774e393f81e2e2a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/237740
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Jake Macdonald <jakemac@google.com>
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Mar 21, 2022
…fer real types from OmittedTypeAnnotations.

Bug:dart-lang/language#2154
Change-Id: I00484ddbd09a3c2371b96e540b9926144d80c832
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/237844
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Jake Macdonald <jakemac@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
static-metaprogramming Issues related to static metaprogramming
Projects
Development

Successfully merging a pull request may close this issue.

3 participants