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

ScopeFactory Proposal #108

Closed
Leland-Takamine opened this issue Aug 15, 2019 · 12 comments
Closed

ScopeFactory Proposal #108

Leland-Takamine opened this issue Aug 15, 2019 · 12 comments
Labels

Comments

@Leland-Takamine
Copy link
Collaborator

Leland-Takamine commented Aug 15, 2019

[Obsolete] See #125

This proposal describes an API to instantiate a Motif Scope that doesn't have a parent Scope. The child method api will still exist - this only applies to the instantiation of root Scopes.

Existing Pattern

@Scope
interface FooScope {
    String string();
  
    @motif.Dependencies
    interface Dependencies {
        String string();
    }
}

// Main
FooScopeImpl.Dependencies dependencies = ...;
FooScope fooScope = new ScopeImpl(dependencies);

Limitations

The pattern above requires you to reference generated code (FooScopeImpl and FooScopeImpl.Dependencies). There are a few problems with this:

Proposed API

@Scope
interface FooScope {
    String string();
}

interface FooDependencies {
    String string();
}

class FooScopeFactory extends ScopeFactory<FooScope, FooDependencies> {}

// Main
FooDependencies dependencies = ...;
FooScope fooScope = new FooScopeFactory().create(dependencies);

Benefits

You no longer need to reference generated code which unlocks the following benefits:

Trade-offs

  • This is a breaking API change since it would replace the @Dependencies feature.
  • Requires a small amount of reflection. However, we can ensure complete compile time safety with validation at annotation processing time.

Implementation

  1. Validate that dependencies and Scope result in a valid graph.
  2. Generate FooScopeFactoryHelper class which allows us to instantiate FooScope given FooDependencies.
  3. The framework ScopeFactory.create method calls the generated FooScopeFactoryHelper.create method reflectively.

Generated Code

public class FooScopeFactoryHelper {

    public static FooScope create(final FooDependencies dependencies) {
        return new FooScopeImpl(new FooScopeImpl.Dependencies() {
            @Override
            public String string() {
                return dependencies.string();
            }
        });
    }
}

New Framework Classes

// @Inherited to allow the annotation processor to inspect all subclasses of ScopeFactory
@Inherited
@interface ScopeFactoryMarker {}

@ScopeFactoryMarker
public class ScopeFactory<S, D> {

    public S create(D dependencies) {
        Class<?> factoryHelperClass = getFactoryHelperClass();
        Method createMethod = factoryHelperClass.getDeclaredMethods()[0];
        try {
            return (S) createMethod.invoke(null, dependencies);
        } catch (IllegalAccessException e) {
            throw new RuntimeException(e);
        } catch (InvocationTargetException e) {
            throw new RuntimeException(e);
        }
    }

    private Class<?> getFactoryHelperClass() {
        try {
            return Class.forName(getClass().getName() + "Helper", false, getClass().getClassLoader());
        } catch (ClassNotFoundException e) {
            throw new RuntimeException(e);
        }
    }
}

Rollout

  1. Land changes to add the ScopeFactory API as a beta feature
  2. If the ScopeFactory API proves to work better than the @Dependencies API, we will move ScopeFactory out of beta and remove the @Dependencies API.
@Ericliu001
Copy link
Contributor

👍

@TonyTangAndroid
Copy link
Contributor

interface FooDependencies {
    String string();
}

Regarding to the above code block, does this means that we have to explicitly list all the dependencies in the Scope when it is defined? This could be a burden that requires us to make effort in figuring out. One of the benefits that I quite enjoy using Motif is that I do NOT have to worry about the dependencies from parent scopes. All I have to focus on is to list out the current dependencies that is used to build the current Scope. Super neat. And the deeper the scope becomes, the better it could be.

@Leland-Takamine
Copy link
Collaborator Author

@TonyTangAndroid I added a bit more clarification in the description. This API change would only apply to root Scopes. The child method API would still exist and any Scopes who are created by some parent Scope do not need to define their dependencies explicitly.

@TonyTangAndroid
Copy link
Contributor

@TonyTangAndroid I added a bit more clarification in the description. This API change would only apply to root Scopes. The child method API would still exist and any Scopes who are created by some parent Scope do not need to define their dependencies explicitly.

I see. That is very good to know. Does this mean that the java refection part is also only applied to Root scopes? Let me know when there is a snapshot version that I could try it out for this feature.

@Leland-Takamine
Copy link
Collaborator Author

Yes that's right, reflection is only required to instantiate a Motif root scope from outside of Motif. I do want to reiterate here though that it will still be completely compile-time safe since we can verify correctness at annotation processing time.

Currently working on implementing this - Will put up a PR to check out once this is ready.

@TonyTangAndroid
Copy link
Contributor

I see. Cool. It makes sense. Looking forward to the snapshot version.

@Leland-Takamine
Copy link
Collaborator Author

PR is ready #113

@tyvsmith
Copy link
Member

This seems like a positive direction to go. We're less than a 1.0, so a breaking API change at this early stage of the project is probably ok. That being said.

  1. Does the rollout break the API, or only once we've removed @deprecated?
  2. What's the perf hit of the reflection cost?
  3. What's the change in number of AP runs?

@Leland-Takamine
Copy link
Collaborator Author

Leland-Takamine commented Aug 21, 2019

  1. Does the rollout break the API, or only once we've removed @deprecated?

The breaking API change would be the removal of the @Dependencies API which would only happen after we've vetted the ScopeFactory API in production.

  1. What's the perf hit of the reflection cost?

Reflection is minimal and the performance cost is negligible since it only happens once on the instantiation of a root scope.

  1. What's the change in number of AP runs?

This depends on the situation. Since the main benefit of this API would be avoiding referencing generated code, I think we can simply say that annotation-processing rounds will either stay the same or be reduced but will never increase with this change.

@TonyTangAndroid
Copy link
Contributor

Before I forget, I am wondering whether there is test case to reproduce google/auto#660 and cover google/auto#660 for Scope Factory?

@Leland-Takamine
Copy link
Collaborator Author

Leland-Takamine commented Aug 22, 2019

Before I forget, I am wondering whether there is test case to reproduce google/auto#660 and cover google/auto#660 for Scope Factory?

Great idea - I'll add that in.

Update: Added a test to ensure that a Scope can extend a user-defined dependencies interface. That said, the new API avoids referencing generated code, so we can't test that issue directly since it's not longer applicable.

@Leland-Takamine
Copy link
Collaborator Author

Closing this out in favor of #125

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

No branches or pull requests

4 participants