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

Deprecate LocalVariableTableParameterNameDiscoverer completely (avoiding its exposure in native images) #29531

Conversation

sdeleuze
Copy link
Contributor

DefaultParameterNameDiscoverer should be updated to not use LocalVariableTableParameterNameDiscoverer when running as a native image.

@sdeleuze sdeleuze added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement theme: aot An issue related to Ahead-of-time processing labels Nov 20, 2022
@sdeleuze sdeleuze added this to the 6.0.1 milestone Nov 20, 2022
@sdeleuze sdeleuze self-assigned this Nov 20, 2022
@snicoll
Copy link
Member

snicoll commented Nov 20, 2022

I think this should behave the same way on the JVM. the check should be on AOT not native.

@sdeleuze
Copy link
Contributor Author

Could be great for consistency indeed even if technically on JVM + AOT we have to the capability to use it. Let's validate the choice in today's Framework meeting.

@sdeleuze sdeleuze changed the title Do not use LocalVariableTableParameterNameDiscoverer in native images Do not use LocalVariableTableParameterNameDiscoverer in AOT mode Nov 21, 2022
@sdeleuze sdeleuze changed the title Do not use LocalVariableTableParameterNameDiscoverer in AOT mode Do not use LocalVariableTableParameterNameDiscoverer in AOT mode Nov 21, 2022
@sdeleuze sdeleuze force-pushed the parameter-name-discoverer-native branch from f8fa9d6 to 71bbabc Compare November 21, 2022 09:17
@sdeleuze
Copy link
Contributor Author

I updated the PR accordingly.

@jhoeller
Copy link
Contributor

An interesting case in terms of automatic exclusion indeed.

We certainly don't want to support that parameter name discovery strategy in a native image (where the underlying class files are generally not available), and it's not recommended in any other scenario either... since you can always compile with -parameters instead, with no need to parse class files then. In such a recommended setup, StandardReflectionParameterNameDiscoverer will always be able to resolve the parameter names first, so LocalVariableTableParameterNameDiscoverer will never actually be reached.

From that perspective, for a setup following Java 8+ recommendations, LocalVariableTableParameterNameDiscoverer could even be removed completely. It's only really there for backwards compatibility with older setups that got migrated without -parameters. Baking that assumption into our AOT arrangement - namely that you need to compile with -parameters when you are optimizing for AOT, completely avoiding unnecessary class file parsing - seems sensible.

@sdeleuze sdeleuze closed this in f4e23fe Nov 21, 2022
@jhoeller
Copy link
Contributor

jhoeller commented Nov 22, 2022

I'm afraid we'll have to return to a NativeDetector check here since AotDetector is in the higher-level aot package whereas DefaultParameterNameDiscoverer is a very low-level core component. Since there is no significant difference to be expected in practice, I don't think the cycle-free narrower check matters. After all, LocalVariableTableParameterNameDiscoverer is effectively not applicable in a native image due to its class file parsing approach, so it arguably does make sense to exclude it on that basis and rely on -parameters usage by convention.

@jhoeller jhoeller changed the title Do not use LocalVariableTableParameterNameDiscoverer in AOT mode Do not use LocalVariableTableParameterNameDiscoverer in native images Nov 22, 2022
@jhoeller
Copy link
Contributor

Reopening this one after a team discussion: It is ultimately preferable to deprecate LocalVariableTableParameterNameDiscoverer completely, not using it by default in any setup anymore, since that class file parsing strategy has been long superseded by the Java 8 -parameters flag on javac already.

We have considered doing this before and meant to address the native image impact of it in 6.0 but somehow missed this last week. So let's fix this glitch now, in time for the Boot 3.0 GA release, with a note in the upgrade wiki page.

@jhoeller jhoeller reopened this Nov 22, 2022
@jhoeller jhoeller self-assigned this Nov 22, 2022
@jhoeller jhoeller changed the title Do not use LocalVariableTableParameterNameDiscoverer in native images Deprecate LocalVariableTableParameterNameDiscoverer completely (avoiding its exposure in native images) Nov 22, 2022
@jhoeller jhoeller closed this in 459e8a1 Nov 22, 2022
jhoeller added a commit that referenced this pull request Nov 22, 2022
Also retaining standard Java parameter names for Spring's AspectJ sources now.

See gh-29531
@jhoeller
Copy link
Contributor

As a lenient measure for the transition period, we'll keep LocalVariableTableParameterNameDiscoverer active on the JVM but log a warning for each successful parameter name resolution. This will only be logged when parameter names actually need to be inspected, and only when StandardReflectionParameterNameDiscoverer did not return anything, suggesting that compilation with -parameters has been missed somewhere.

jhoeller added a commit that referenced this pull request Nov 23, 2022
…g entries

For a transition period, LocalVariableTableParameterNameDiscoverer logs a warning for each successful resolution attempt now, suggesting that -parameters was missed.

See gh-29531
See gh-29559
fmbenhassine added a commit to spring-projects/spring-batch that referenced this pull request Feb 9, 2023
This commit adds "-parameters" to the compiler
arguments to remove Spring Framework warnings
about parameter name resolution.

Resolves #4258
Related to spring-projects/spring-framework#29531
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) theme: aot An issue related to Ahead-of-time processing type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants