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

Allow MethodReference to support a more flexible signature #29005

Closed
snicoll opened this issue Aug 24, 2022 · 5 comments
Closed

Allow MethodReference to support a more flexible signature #29005

snicoll opened this issue Aug 24, 2022 · 5 comments
Assignees
Labels
theme: aot An issue related to Ahead-of-time processing type: enhancement A general enhancement
Milestone

Comments

@snicoll
Copy link
Member

snicoll commented Aug 24, 2022

While working on #28976, it became apparent that a BeanFactory initializer registered in BeanFactoryInitializationCode could use a more flexible signature. Rather than injecting the DefaultListableBeanFactory the contribution needs a ConfigurableEnvironment and a ResourceLoader. The former can be retrieved by the bean factory (although a bit odd). The latter is harder.

MethodReference provides a very nice abstraction and we've almost what we need but I'd like to use the opportunity to see if we can avoid adding yet another toCodeBlock method as the current ones are already a bit confusing IMO.

GeneratedMethod

It's often needed to get a MethodReference from a GeneratedMethod. It seems it would be relatively easy to pass a ClassName to GeneratedMethods so that the GenerateMethod it produces can return a MethodReference (toMethodReference or something like that).

Also a method reference that's built from a MethodSpec this way knows about its name, kind, and declaring class.

MethodReference getDeclaringClass and getMethodName

Both of these seem to be unused so they should probably be removed.

CodeBlock patterns

MethodReference provides several ways of invoking the method:

  • toCodeBlock(): is producing a method reference, expecting the current context to match the signature and the method to be defined in the current class (if not static).
  • toCodeBlock(String instanceVariable): is producing a method reference, expecting the current context to match the signature and the method to be defined by the class referred to the instanceVariable argument. (It looks like it's only used in tests or by toCodeBlock that's calling with a null instance variable).
  • toInvokeCodeBlock(CodeBlock... args): is producing a method invocation expected to match the arguments and the method to be defined in the current class (if not static)
  • toInvokeCodeBlock(String instanceVariable, CodeBlock... args): is producing a method invocation expected to match the arguments and the method to be defined by the class referred to the instanceVariable argument.

I wonder if a unique toCodeBlock that provides a context could let the MethodReference decide for itself how the method invocation should occur. The following information would be needed:

  • A resolver for arguments that could work at two potential levels:
    • Provide a CodeBlock for a given Class
    • Expected well-known variable name to be present
  • If the declaring class of the method is the current class. If not, the value of an instanceVariable.
  • Whether or not lambda references are possible (that last bit is still a bit blurry).
@snicoll snicoll added type: enhancement A general enhancement theme: aot An issue related to Ahead-of-time processing labels Aug 24, 2022
@snicoll snicoll added this to the 6.0.0-M6 milestone Aug 24, 2022
@snicoll
Copy link
Member Author

snicoll commented Aug 24, 2022

InstanceSupplierCodeGenerator also has a method that could become irrelevant to some extent:

private CodeBlock generateReturnStatement(GeneratedMethod getInstanceMethod) {
	return CodeBlock.of("$T.$L()", this.className, getInstanceMethod.getName());
}

@snicoll
Copy link
Member Author

snicoll commented Aug 24, 2022

toInvokeCodeBlockForInstance also creates a new instance of the declaring class which looks a very specific decision such a generic method would take.

@snicoll
Copy link
Member Author

snicoll commented Aug 24, 2022

Most of the of static factory method of MethodReference are only used in tests.

snicoll added a commit to snicoll/spring-framework that referenced this issue Aug 24, 2022
This commit updates GeneratedMethod and its underlying infrastructure
to be able to produce a MethodReference. This simplifies the need when
such a reference needs to be created manually and reuses more of what
MethodReference has to offer.

See spring-projectsgh-29005
snicoll added a commit to snicoll/spring-framework that referenced this issue Aug 24, 2022
@snicoll
Copy link
Member Author

snicoll commented Sep 5, 2022

I've had a brainstorming session with @wilkinsona and @bclozel today. We've discussed various options and the pros and cons and we conclude that it is overkill to try to avoid both side having some sort of shared context. Trying to do that means too much context needs to be provided by the caller and MethodReference has to know too many things to take the right decision.

We came back with the original need of offering a more flexible signature and we'd like to explore how toInvokeCodeBlock could be changed to take something that resolves a limited number of arguments, rather than the current CodeBlock vararg.

@snicoll snicoll self-assigned this Sep 6, 2022
snicoll added a commit to snicoll/spring-framework that referenced this issue Sep 6, 2022
This commit updates GeneratedMethod and its underlying infrastructure
to be able to produce a MethodReference. This simplifies the need when
such a reference needs to be created manually and reuses more of what
MethodReference has to offer.

See spring-projectsgh-29005
snicoll added a commit to snicoll/spring-framework that referenced this issue Sep 6, 2022
This commit moves MethodReference to an interface with a default
implementation that relies on a MethodSpec. Such an arrangement avoid
the need of specifying attributes of the method such as whether it is
static or not.

The resolution of the invocation block now takes an
ArgumentCodeGenerator rather than the raw arguments. Doing so gives
the opportunity to create more flexible signatures.

See spring-projectsgh-29005
snicoll added a commit to snicoll/spring-framework that referenced this issue Sep 6, 2022
This commit moves MethodReference to an interface with a default
implementation that relies on a MethodSpec. Such an arrangement avoid
the need of specifying attributes of the method such as whether it is
static or not.

The resolution of the invocation block now takes an
ArgumentCodeGenerator rather than the raw arguments. Doing so gives
the opportunity to create more flexible signatures.

See spring-projectsgh-29005
@snicoll
Copy link
Member Author

snicoll commented Sep 6, 2022

I've made some good progress in https://github.com/snicoll/spring-framework/tree/gh-29005 - One advantage of knowing the ultimate location of the code is that we don't expand the class name for static internal calls. For instance, the following code:

beanDefinition.setInstanceSupplier(ApplicationAvailabilityAutoConfiguration__BeanDefinitions.getApplicationAvailabilityInstanceSupplier());

Is now:

beanDefinition.setInstanceSupplier(getApplicationAvailabilityInstanceSupplier());

However, knowing the ultimate target class isn't always possible so we might need to adapt some API. Perhaps methodReference itself could offer some sort of support for this.

snicoll added a commit to snicoll/spring-framework that referenced this issue Sep 6, 2022
This commit moves MethodReference to an interface with a default
implementation that relies on a MethodSpec. Such an arrangement avoid
the need of specifying attributes of the method such as whether it is
static or not.

The resolution of the invocation block now takes an
ArgumentCodeGenerator rather than the raw arguments. Doing so gives
the opportunity to create more flexible signatures.

See spring-projectsgh-29005
snicoll added a commit to snicoll/spring-framework that referenced this issue Sep 7, 2022
This commit updates GeneratedMethod and its underlying infrastructure
to be able to produce a MethodReference. This simplifies the need when
such a reference needs to be created manually and reuses more of what
MethodReference has to offer.

See spring-projectsgh-29005
snicoll added a commit to snicoll/spring-framework that referenced this issue Sep 7, 2022
This commit moves MethodReference to an interface with a default
implementation that relies on a MethodSpec. Such an arrangement avoid
the need of specifying attributes of the method such as whether it is
static or not.

The resolution of the invocation block now takes an
ArgumentCodeGenerator rather than the raw arguments. Doing so gives
the opportunity to create more flexible signatures.

See spring-projectsgh-29005
snicoll added a commit to snicoll/spring-framework that referenced this issue Sep 7, 2022
This commit allows bean factory initialization to use a more flexible
signature than just consuming the DefaultListableBeanFactory. The
environment and the resource loader can now be specified if necessary.

See spring-projectsgh-29005
snicoll added a commit to snicoll/spring-framework that referenced this issue Sep 9, 2022
This commit updates GeneratedMethod and its underlying infrastructure
to be able to produce a MethodReference. This simplifies the need when
such a reference needs to be created manually and reuses more of what
MethodReference has to offer.

See spring-projectsgh-29005
snicoll added a commit to snicoll/spring-framework that referenced this issue Sep 9, 2022
This commit moves MethodReference to an interface with a default
implementation that relies on a MethodSpec. Such an arrangement avoid
the need of specifying attributes of the method such as whether it is
static or not.

The resolution of the invocation block now takes an
ArgumentCodeGenerator rather than the raw arguments. Doing so gives
the opportunity to create more flexible signatures.

See spring-projectsgh-29005
@snicoll snicoll changed the title Review MethodReference to support more scenarios All MethodReference to support a more flexible signature Sep 12, 2022
snicoll added a commit that referenced this issue Sep 12, 2022
This commit updates GeneratedMethod and its underlying infrastructure
to be able to produce a MethodReference. This simplifies the need when
such a reference needs to be created manually and reuses more of what
MethodReference has to offer.

See gh-29005
snicoll added a commit that referenced this issue Sep 12, 2022
This commit moves MethodReference to an interface with a default
implementation that relies on a MethodSpec. Such an arrangement avoid
the need of specifying attributes of the method such as whether it is
static or not.

The resolution of the invocation block now takes an
ArgumentCodeGenerator rather than the raw arguments. Doing so gives
the opportunity to create more flexible signatures.

See gh-29005
snicoll added a commit that referenced this issue Sep 12, 2022
This commit allows bean factory initialization to use a more flexible
signature than just consuming the DefaultListableBeanFactory. The
environment and the resource loader can now be specified if necessary.

See gh-29005
snicoll added a commit to snicoll/spring-cloud-stream that referenced this issue Sep 12, 2022
snicoll added a commit to snicoll/spring-cloud-commons that referenced this issue Sep 12, 2022
snicoll added a commit to spring-projects/spring-boot that referenced this issue Sep 12, 2022
olegz pushed a commit to spring-cloud/spring-cloud-stream that referenced this issue Sep 12, 2022
OlgaMaciaszek pushed a commit to spring-cloud/spring-cloud-commons that referenced this issue Sep 12, 2022
@snicoll snicoll changed the title All MethodReference to support a more flexible signature Allow MethodReference to support a more flexible signature Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: aot An issue related to Ahead-of-time processing type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

1 participant