How to make an aop package (aspectd) in a decent way?

kangwang1988 commented 5 years ago

This issue is related to https://github.com/flutter/flutter/pull/35803

Recently, I've developed a package aiming at implement the AOP(aspect oriented programming) feature for dart based on kernel-to-kernel transformation which is available at: https://github.com/alibaba-flutter/aspectd

A design diagram is given below: Aspectd Diagram

Though it is expected to do aop without invasion, there is still one invasion point.

Given an app package(where original codes is given), aspectd requires a new aop package where aop codes are written, so that when compiling the aop package, both app and aop codes can be all compiled into the app.dill. Henceforth, a hook is needed in the app building process(flutter_tools) so that aop package can be compiled when app package is compiled successfully, both in jit/aot mode. After the aop package is compiled into dill successfully, a dill transformer is needed to extract the annotations and transform the component object so that aop feature can be implemented as expected.

More generally, different from gradle, design of flutter_tools make it hard for developers if they want to do inject some customized hooks before/after running flutter commands.

It would be better if a generalized way is provided.

Current patching logic to flutter_tools is given below: a. build_aot.dart

Screen Shot 2019-07-23 at 4 34 49 PM

b.build_bundle.dart Screen Shot 2019-07-23 at 4 34 57 PM

c.aspectd.dart Not a problem as totally new.

cc @jonahwilliams

kangwang1988 commented 5 years ago

@Hixie Can you comment on this issue？

jonahwilliams commented 5 years ago

Hi @kangwang1988, I'm really sorry for the delay in getting back to you. I have a few high level thoughts:

This seems like a really useful framework, so I'm interested in making it easier to use for you & others.
Injection of custom hooks/scripts into the build system is something we've been discussing. Its been a bit blocked behind other work to clean up the build system itself so that it is configurable/testable (from our end). Recently @blasten floated the idea of making our system full scriptable/controllable by developers too.

I think it would be useful to separate these two concerns a bit. The latter is a large open ended problem that's going to take a long time to scope and design. The former seems more tractable via source generation, which is already fairly well supported by https://pub.dev/packages/build.

Have you looked at source generation instead of custom scripts for this?

blasten commented 5 years ago

@xster wrote a doc recently that considers the benefits of proving custom hooks into the Flutter build system.

xster commented 5 years ago

minor caveat: my proposal relates mainly to how to customize the integration of a Flutter library into a wrapper app.

Conceptually, I think the desired functionality here is independent of wrapper apps. It's about doing annotation parsing and codegen purely inside of a Flutter library whether there is a wrapper app or not. I think the builder package Jonah pointed to likely relates more.

kangwang1988 commented 5 years ago

@jonahwilliams Now we focus on how to do dill transformation based on a given dill, the transformation might be enabled or not. Can the build package be modified to support this? In other words, provides a way to support dill transformation in the build process?

xster commented 5 years ago

I think what might not be clear yet is why can't the aop functionalities by accomplished by dart source generation rather than dill transformations. Can you describe the details a bit more?

kangwang1988 commented 5 years ago

@xster Now, we're doing(planning) several packages based on dill transformation, including AOP(aspectd), JSON<->Model Conversion, LightMirror. IMO, dill transformation is quite similar to source gen as dill is just a structure AST tree for source code. However, there might be still some difference here. For LightMirror, we just need to add some extra apis, which won't modify the original code. In this occasion, either source generation or dill transformation works fine. However, for AOP, we need to modify the original code. If using the source generation approach, the original code will be modified multiple times, which will works unexpected.

kangwang1988 commented 5 years ago

@jonahwilliams Besides, dill transformation is following the principle of --track-widget-creation. The logic is used to be located in engine, now relocated to dart-sdk : https://github.com/dart-lang/sdk/blob/master/pkg/vm/lib/target/flutter.dart

It's a generic approach to do IL(intermediate language) transformation, like what's behind AspectJ in java. It's useful but not limited to AOP. In Dart, kernel to kernel transformation is like class weaver in java. I think it is expected to provider convenient api to do IL transformation, not only to the framework(sdk) developers, but also to the community.

jonahwilliams commented 5 years ago

Providing an API for kernel transformation is a much more restricted problem than supporting arbitrary build scripts for transformation. I agree that sounds like a reasonable goal.

In order to work well with the rest of flutter, it would need to be a part of the existing kernel transformation pipeline. Maybe the easiest way that could be done would be to allow providing your own frontend_server API?

Or teaching it to load plugins/transforms on the fly

kangwang1988 commented 5 years ago

@jonahwilliams Sounds good. The script approach is just a method, not the goal. What we need is to add our kernel transformation logic into the existing pipeline. As long as it's available to us, not limited to dart-sdk, it would be enough for us.

kangwang1988 commented 5 years ago

Priorly, when track-widget-creation logic is located in the flutter engine as a separated package and imported&applied by frontend_server, I was thinking it would be better if it's configurable to allow developers to define and inject their own transformers. However, for me, the easiest way to do this is to inject my transformers by hacking the flutter_tools. What's more, let's say in a wider scope. Different from gradle, the flutter build system(flutter_tools) is so hard coded(I have to modify the flutter_tools logic and delete the stamp to make it take effect) and doesn't provide hook points to developers. I would like that a hook mechanism would make the build process more universal and flexible.

kangwang1988 commented 5 years ago

@jonahwilliams As you agree it's would be nice to provide api for kernel to kernel transformation, is there any plan when it might be available? Now we have several projects ongoing and planning, which is depending on this.

jonahwilliams commented 5 years ago

Today we met to discuss the feasibility of incorporating support for arbitrary transformers into our compilation pipeline. Based on our discussion today, the rough design I have to do so is similar to how we generate/integrate source based transformation:

The tool finds certain metadata in pubspec.yaml of the application, and of package dependencies that declare that a transformer exists and how it is to be used.
The tool runs a local source generation to create a new frontend_server implementation which incorporates the declared transformers.
During all existing build/run cases, this local snapshot is delegated to instead of the prebuilt frontend_server.

There are also a few questions we need to answer:

Can we ship the libraries required by the aspectd package with our SDK? This would be required for runtime generation of the new frontend server.
- What metadata does the flutter tool require in order to generate the local frontend server snapshot?
Can the aspectd package be made to correspond to the existing frontend_server requirements? That is, more or less like the old track widget creation transformer.

I'll try and have an answer for the first by the end of the week, though I think that won't be an issue. Next, I'll provide some set of information that I need from the aspectd package, like source location/types/et cetera and @kangwang1988 can propose an initial metadata format. Once both are complete, I will wire up an initial experiment in a branch that performs the process for dill injection.

jonahwilliams commented 5 years ago

Update: I wrote up an initial design for how this would work and started collecting feedback on it. I'm preparing to address some of the initial feedback, and I'll plan move it to an open source location early next week. I hope that you'll be able to take a look then and provide more feedback on the design

kangwang1988 commented 5 years ago

@jonahwilliams Nice, we'll see when it's available.

jonahwilliams commented 5 years ago

@kangwang1988 I originally attempted to use a shared google document but found that difficult to share, so I've written out my design here:

Supporting arbitrary Kernel transformation in the flutter_tool

Background

Currently the flutter_tool uses a precompiled app-jit snapshot of the frontend_server distributed via cached artifacts. This program performs kernel compilation for all (non-web) flutter applications, including applying the built in track widget creation transformer used by the widget inspector. This transformer uses the package:kernel API for AST transformation exposed by the incremental compiler. I propose that we expose this same capability to plug-in arbitrary transformers in the flutter_tool.

Overview

This would initially be available via an experimental opt-in feature initiated by flutter config. I expect that I'll need to present a basic outline of the functionality before the aspectd package can be updated to use it. During that time I expect to encounter more issues that will need to be resolved.

For example, enabling this feature.

flutter config --enable-kernel-transform

To apply a new kernel transformer to an application, there will need to be additional specification in the application's pubspec. I propose adding a new top level field, similar to dependencies. This information, combined with the experiment being enabled about will inform the tool that it needs to switch from the prebuilt frontend_server to a configured server. In addition, placing these dependencies in a separate pubspec section allows us to prune the application's dependencies further - that is, the application itself does not need to import the kernel transformer.

For example, adding a transformer from a package:foo

name: my_app

dependencies:
  flutter:
     sdk: flutter

transformers:
  foo: 1.2.3

Once both of the above are done, rebuilding the application will lead to the configured transformers being automatically applied in both incremental and non-incremental modes. This is assumed to be sufficient for an initial version.

Detailed Design

Selection of transformers

Given that both cases above are met (experiment enabled and transformers: present), The tool will generate a new frontend_server entrypoint and snapshot it. Via our existing artifact APIs, we can update our tool to use the new snapshot without any code changes. That is, this change is highly modular with the rest of the tool.

Note: The reason we need to generate a new snapshot is that there is no way for us to "dynamically" inject user provided transformers into the existing snapshot

The approximate steps for generating this snapshot are:

Download package:kernel and package:vm sources from our prebuilt artifacts.
Generate a synthetic pubspec in .dart_tool/build/transformers containing the above artifacts as path dependencies, as well as the contents of the transformers: section in the pubspec.yaml
Resolve the packages in this subdirectory, reporting any errors in resolution.
If resolution is successful, walk the transformer dependencies and look for a metadata file from each one (Suppose it is called transform.yaml). 4a. For each metadata file we need to know how to import that package's transformer, so at a minimum it should contain the class name and path to the import.
Generate an entrypoint that imports the frontend server, along with each transformer discovered in 4).
Through a constructor parameter in the frontend_server, new each Transformer and provide it to a list. (The order of the transformers should either be specified to match what is in the pubspec or be random)
Make a kernel snapshot of this frontend server and save it in a folder under this application.

Separately, during startup of the tool we need to determine whether or not to run this snapshotting logic. When this mode is enabled, we'll check for the transformer key on startup. If it is present, we'll use a hash computed from the versions of all packages input to the snapshot generation stage above (in addition to the dart version) to determine whether it can be skipped.

Caveats

Performance

The app-jit snapshot provided via our prebuilt artifacts is "trained", meaning that when generating the snapshot we ran the compiler and saved the generated optimized JIT code. We could also train the locally generated frontend_server snapshot, but that will add significant time to the tool initialization every time the dart or transformer version changes. For now I'll plan on running a non-trained snapshot.

Versioning of `package:kernel`

The kernel generated by the transformers needs to match the same kernel version that the flutter engine is using. This means that we cannot pull package:kernel from pub or anywhere else, it must be provided via the cached artifacts and updated lockstep with the Flutter SDK. This also means that transformers that are plugged into the flutter tool either (1) need to be built against the SDK version of package:kernel or (2) should fail when resolving the transformer version if the declared dependency on package:kernel does not match the current Flutter SDK. Initially I think that (1) will be the best approach.

API Scope

We'd like to limit the scope of the APIs exposed by the transformers, as well as limit how much additional code we run when building kernel. Running arbitrary scripts, whether Dart or another language is not a part of this design. Instead the transformers will expect to find, via some metadata, an importable instance of a Transformer that can be created an injected. No additional setup or teardown hooks are provided besides this.

kangwang1988 commented 5 years ago

@jonahwilliams It's a reasonable design. Can we refactor current design to make track-widget-creation also implemented using this approach? Once done, it would be also feasible to all transformers.

jonahwilliams commented 5 years ago

For at least the short-medium term, I don't think we should try and switch to a generated transformer for this one since it is so common. Longer-term, I agree it would be better to have everything on the same path - lets keep this one in mind.

kangwang1988 commented 5 years ago

@jonahwilliams So are we expected to support this aspectd scenario for the short-medium term and consider the track-widget-creation in the longer-term?

jonahwilliams commented 5 years ago

I think track-widget-creation should work fine as a built in for now.

jonahwilliams commented 5 years ago

Update: I have a pending PR which should upload the required sources for regenerating the compiler locally. Separately I have a rough proof of concept test patch for adding this to the tool - it isn't usable yet but once the artifacts are upload I should be able to make progress there

xujim commented 5 years ago

@jonahwilliams sorry for responding this issue late. I am working on it now. :) However, I failed to setup the kernel compiler by your PR. I successfully generated the kernel_compiler folder and its pubspec.ymal file, but it prompts error like "frontend_server from path is forbidden" when I run the pub get in the hello_world example. If I changed the pubspec file to git path, it prompts "frontend_server from git is forbidden" instead. When will you merge this PR? By the way, I changed the pubspec file as following:

name: flutter_tool
environment:
  # The pub client defaults to an <2.0.0 sdk constraint which we need to explicitly overwrite.
  sdk: ">=2.2.2 <3.0.0"

dependencies:
  string_transformer:
    path: ../../../../dev/integration_tests/string_transformer

  frontend_server:
    git:
        url: https://github.com/flutter/engine.git
        ref: da71c381c76b5536a554d11ac771f50ec1b32d0f
        path: frontend_server

xujim commented 5 years ago

➜  hello_world git:(generate_frontend) ✗ flutter pub upgrade
Because every version of frontend_server from git depends on vm any which is forbidden, frontend_server from git is forbidden.
So, because flutter_tool depends on frontend_server from git, version solving failed.
pub get failed (69) -- attempting retry 1 in 1 second...                
generating kernel coompiler...                                         ⡿^C

This is the error message

xujim commented 5 years ago

my flutter:

➜  string_transformer git:(generate_frontend) ✗ flutter --version
Flutter 0.1.8-pre.5660 • channel generate_frontend • git@github.com:jonahwilliams/flutter.git
Framework • revision c5dcb77ce6 (5 周前) • 2019-09-29 17:11:27 -0700
Engine • revision da71c381c7
Tools • Dart 2.6.0 (build 2.6.0-dev.0.0 afac6a3714)

jonahwilliams commented 4 years ago

Currently blocked by https://github.com/flutter/flutter/issues/47162, updating milestone accordingly

flutter / flutter