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

Support varargs invocations in SpEL for varargs array subtype in compiled expressions #32804

Closed
wants to merge 1 commit into from

Conversation

LeMikaelF
Copy link
Contributor

@LeMikaelF LeMikaelF commented May 13, 2024

This PR adds support for varargs invocations where the varargs argument is an array with a component type that is a subtype of the parameter varargs component type. For example, the method accepts an Object... args parameter, and it is passed a String[] argument. #32704 already added support for this in interpreted expressions. There were a few tests that were commented out because compiled expressions didn't support this yet, so I uncommented them and added more tests for functions, methods, and constructors.

I will be happy to address any comments on this PR.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label May 13, 2024
@sbrannen sbrannen self-assigned this May 13, 2024
@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels May 13, 2024
@sbrannen sbrannen added this to the 6.2.0-M2 milestone May 13, 2024
Copy link
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the proposed fix as well as all of the additional tests! 👍

I've left some comments, but there's no need to address those in this PR at this time, since I plan to experiment and potentially make changes locally before merging this work.

@LeMikaelF
Copy link
Contributor Author

Thanks for the proposed fix as well as all of the additional tests! 👍

I've left some comments, but there's no need to address those in this PR at this time, since I plan to experiment and potentially make changes locally before merging this work.

Thank you for the comments! I'll keep an eye out for the final fix.

sbrannen added a commit to sbrannen/spring-framework that referenced this pull request May 13, 2024
See spring-projectsgh-32804

Co-authored-by: Mikaël Francoeur <mikael.francoeur@ticketmaster.com>
sbrannen pushed a commit to sbrannen/spring-framework that referenced this pull request May 13, 2024
This commit introduces support for compiling SpEL expressions that
contain varargs invocations where the supplied array is a subtype of
the declared varargs array type.

See spring-projectsgh-32804
sbrannen added a commit to sbrannen/spring-framework that referenced this pull request May 13, 2024
This commit partially reverts changes to SpelNodeImpl in order to
reduce the scope of the overall change set.

In addition, this commit revises the previous commit to support
type-safe checks for array subtype compatibility.

In order to support backward compatibility, this commit also reintroduces
generateCodeForArguments(MethodVisitor, CodeFlow, Member, SpelNodeImpl[])
in deprecated form.

Closes spring-projectsgh-32804
@sbrannen
Copy link
Member

sbrannen commented May 13, 2024

Current work on this issue can be viewed in the following feature branch which is based on this PR.

main...sbrannen:spring-framework:issues/gh-32804-spel-varargs-array-subtype-compilation

sbrannen added a commit to sbrannen/spring-framework that referenced this pull request May 13, 2024
This commit partially reverts changes to SpelNodeImpl in order to
reduce the scope of the overall change set.

In addition, this commit revises the previous commit to support
type-safe checks for array subtype compatibility.

In order to support backward compatibility, this commit also reintroduces
generateCodeForArguments(MethodVisitor, CodeFlow, Member, SpelNodeImpl[])
in deprecated form.

Closes spring-projectsgh-32804
sbrannen added a commit to sbrannen/spring-framework that referenced this pull request May 13, 2024
This commit partially reverts changes to SpelNodeImpl in order to
reduce the scope of the overall change set.

In addition, this commit revises the previous commit to support
type-safe checks for array subtype compatibility.

In order to support backward compatibility, this commit also reintroduces
generateCodeForArguments(MethodVisitor, CodeFlow, Member, SpelNodeImpl[])
in deprecated form.

Closes spring-projectsgh-32804
sbrannen added a commit that referenced this pull request May 14, 2024
See gh-32804

Co-authored-by: Mikaël Francoeur <mikael.francoeur@ticketmaster.com>
sbrannen pushed a commit that referenced this pull request May 14, 2024
This commit introduces support for compiling SpEL expressions that
contain varargs invocations where the supplied array is a subtype of
the declared varargs array type.

See gh-32804
sbrannen added a commit that referenced this pull request May 14, 2024
This commit first reverts changes to SpelNodeImpl from the previous
commit in order to reduce the scope of the overall change set.

This commit then implements a different approach to support type-safe
checks for array subtype compatibility.

In order to support backward compatibility, this commit also
reintroduces generateCodeForArguments(MethodVisitor, CodeFlow, Member,
SpelNodeImpl[]) in deprecated form.

See gh-32804
@sbrannen sbrannen closed this in 061c13d May 14, 2024
@sbrannen
Copy link
Member

This has been merged into main in 29bb7b9 and 12727a2 and revised in 8fe4493.

To see the combined result, view the merged commit 061c13d.

Thanks again for finding the source of the error and taking the initiative to submit a proposal with comprehensive test coverage! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants