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

[WIP] Add macro e2e test #3650

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

[WIP] Add macro e2e test #3650

wants to merge 4 commits into from

Conversation

jakemac53
Copy link
Contributor

I am starting to test out macros in build_runner, compiler integrations are known to need work but I am confused by the analyzer stuff in particular here. I did expect it to work correctly (but inefficiently) and be able to run the macros, however they never seem to be applied.

Specifically in this branch, if you run:

  • cd _test
  • dart pub upgrade
  • dart run build_runner build --enable-experiment=macros --build-filter=web/macros/main.hasDebugToString

And then open up .dart_tool/build/generated/_test/web/macros.hasDebugToString I would expect to see true, indicating the macro had ran. But I get false, indicating it never ran. I am not sure the best way to start debugging that?

@scheglov do I need to toggle anything special to enable macros in the analysis context?

cc @davidmorgan

@scheglov
Copy link
Contributor

scheglov commented Feb 16, 2024

I added some logging to the analyzer, it saw that we don't pass a MacroSupport instance when create AnalysisDriver. Then I realized that package:build goes through a different path than AnalysisContextCollectionImpl. It uses lib/src/clients/build_resolvers/build_resolvers.dart.

With this fixed (I will send the CL), we get a little further:

[INFO] Running build...Unhandled exception:
'file:///_test/lib/macros/debug_to_string.dart.macro': error: /_test/lib/macros/debug_to_string.dart.macro:14:9: Error: Undefined name 'MacroExpansionClient'.
  await MacroExpansionClient.start(
        ^^^^^^^^^^^^^^^^^^^^
#0      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:293:33)
#1      _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:184:12)

I suspect that we need to make portions of _fe_analyzer_shared available. In the analyzer we copy it into the MemoryResourceProvider. We do it here. It is somewhat convoluted because of caching to get maximum speed of running tests.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Feb 17, 2024
dart-lang/build#3650

Change-Id: I9437205dd42335fdba8e59736ef4d6ba557b9aab
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/352997
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
@jakemac53
Copy link
Contributor Author

I suspect that we need to make portions of _fe_analyzer_shared available. In the analyzer we copy it into the MemoryResourceProvider. We do it here. It is somewhat convoluted because of caching to get maximum speed of running tests.

Yeah, we only make available to the analyzer generally the transitive imports of a given library. But when running macros the analyzer needs to create the macro bootstrap program and that imports stuff from _fe_analyzer_shared.

Once we move all this to package:macros, that should make it easier, I don't really want to hardcode stuff in today which I know will be moved soon :). But I might play around with it just to see if things work.

@jakemac53
Copy link
Contributor Author

Nice, I did get things working here. I had to also explicitly make the package config file available to the in memory resource provider (and make it also match up with the one the analyzer is given, which is different from the one on disk).

Would it possibly make sense for the analyzer to pass a package config more explicitly, matching its Packages instance?

@scheglov
Copy link
Contributor

Nice, I did get things working here. I had to also explicitly make the package config file available to the in memory resource provider (and make it also match up with the one the analyzer is given, which is different from the one on disk).

Ah, yes. CFE reads package_config.json file to know how to compile the generated bootstrap code.

Would it possibly make sense for the analyzer to pass a package config more explicitly, matching its Packages instance?

I don't know. To answer this I need to understand better if we need two separate sets of libraries / packages to write macros, and to compile macro into kernels.

@jakemac53
Copy link
Contributor Author

I don't know. To answer this I need to understand better if we need two separate sets of libraries / packages to write macros, and to compile macro into kernels.

It should always use the same package config to compile the macro as the root application. I can't think of a situation in which that wouldn't be the case.

@scheglov
Copy link
Contributor

I don't know. To answer this I need to understand better if we need two separate sets of libraries / packages to write macros, and to compile macro into kernels.

It should always use the same package config to compile the macro as the root application. I can't think of a situation in which that wouldn't be the case.

So, no separate special package config?

Then could we expose / copy the existing package_config.json file through the resourceProvider given to createAnalysisDriver? We probably already have to do this for all *.dart files.

AFAIK we get Packages instance mostly because it is a non-standard one that we cannot build directly from package_config.json, otherwise we would build it from the file.

@jakemac53
Copy link
Contributor Author

So, no separate special package config?

It is special in that it is different from the one the user has on disk (in build_runner). We create consistent, but fake paths for all the assets (consistent across environments).

So we can't just copy the file, but within a single analysis context there should only be one consistent package resolution. My existing prototype just uses the Packages instance to create the package config.

@davidmorgan
Copy link
Contributor

Trying this PR in the mockito repo, I got a macro build error related to 'asset' URIs

RequestChannelException: Crash when compiling asset:mockito/test/end2end/macro.dart:
RequestChannelException: Invalid argument(s): Uri asset:mockito/test/end2end/macro.dart must have scheme 'file:'.
#0      PosixStyle.pathFromUri (package:path/src/style/posix.dart:55:5)
#1      Context.fromUri (package:path/src/context.dart:1040:40)
#2      KernelCompilationService.compile.<anonymous closure>.uriStrToFile (package:analyzer/src/summary2/kernel_compilation_service.dart:106:45)
#3      KernelCompilationService.compile.<anonymous closure>.<anonymous closure> (package:analyzer/src/summary2/kernel_compilation_service.dart:115:25)
#4      RequestChannel._processMessage (package:_fe_analyzer_shared/src/macros/compiler/request_channel.dart:68:20)

which went away if I move macro.dart (containing the macro) under lib instead of test ... no idea what the cause is, if I should file an issue somewhere please let me know :) thanks.

@jakemac53
Copy link
Contributor Author

jakemac53 commented Mar 12, 2024

which went away if I move macro.dart (containing the macro) under lib instead of test ... no idea what the cause is, if I should file an issue somewhere please let me know :) thanks.

Ah interesting, that does make sense, and something probably does need to be updated. Probably those URIs need to be translated to a "file" URI before sending them to the CFE.

Or, we could treat asset: as a multi-root scheme, they will only show up for the root package so we do know the actual "file" root (/packages/<root-package>).

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

Successfully merging this pull request may close these issues.

None yet

3 participants