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

enhancing Java parsers to recover method invocation type attribution when arg types are missing #4596

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nmck257
Copy link
Collaborator

@nmck257 nmck257 commented Oct 21, 2024

(if there's only one valid signature matching the method name and arg count)

What's changed?

see subject

What's your motivation?

Lombok support (#1297) is presumably still far away. In the interim, this PR attempts to "patch over" one hole in type attribution which Lombok repos can introduce.

For example, I frequently work with ( / refactor) code like this:

MyCoolBuilder.builder()
.propA(...)
.propB(...)
.propC(...)
.fancyConfig(...)

Sometimes developers will supply those props from a separate Properties class with getters from Lombok, eg, .propA(lombokPropertyClass.getPropA()). OpenRewrite won't know the type of the getter, because Lombok. That cascades to OpenRewrite not knowing the type of the propA(...) method invocation. That cascades to OpenRewrite not knowing the type of the select for the next method, and so on. If I want to refactor fancyConfig(...), then even if those arg types are known, the select is unknown, and method matchers miss.

Anything in particular you'd like reviewers to focus on?

  • Conceptually, if we add support for annotation processors like Lombok in the future, then I believe this should become dead code as we should have "perfect" type attribution in the first place. But, I think it's worthwhile until then?
  • Known gap: if the relevant methodInvocation is being called without a select, then I don't see a sturdy way to identify the implicit receiver, so basically this new code just doesn't apply
  • I haven't touched the Java Parsers before; I expect by the time I'm done, I'd just need to copy-paste this new source bit (in JDK 8-friendly syntax) to each of the 4 parsers?
  • Not sure if we'd want to do any preemptive integration test for LST generation before merging something like this?

Anyone you would like to review specifically?

maybe @sambsnyd , @knutwannheden

Have you considered any alternatives or workarounds?

  • Wait for Lombok / annotation processor / code gen support
  • Do nothing

Any additional context

It's a draft because I haven't ported to the other JDK versions yet (or tested much beyond the unit tests), and figured I'd solicit feedback first in case there were directional concerns.

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

…d type attribution when arg types are missing if there's only one valid signature matching the method name and arg count
@traceyyoshima
Copy link
Contributor

traceyyoshima commented Oct 21, 2024

Hi Nick, I doubt processors will be introduced any time soon since annotation processing is triggered in JavaCompiler#compile instead of JavaCompiler#parse. It might be simpler to consider saving RetentionPolicy.SOURCE annotations during type attribution or make an exception for lombok types.

Saving the SOURCE annotations would preserve the lombok annotations directly on the types, which could be handled by recipes. Technically, recipes could be used to generate LST elements based on the annotations, and the JavaTemplate could be used to set the appropriate types as well. It's also likely faster and more efficient since recipes are so lightweight compared to compilation.

@timtebeek
Copy link
Contributor

Thanks @nmck257 ! Not sure if you've spoken to Sam about this effort already, but you've at least inspired
main...lombok-spike . Let's see what the outcome of those efforts is as well.

@nmck257
Copy link
Collaborator Author

nmck257 commented Oct 22, 2024

yeah, we've had a few pings across mismatched working hours. the lombok-spike branch does seem promising, and if I understand it right, would remove the need for this PR :)

@knutwannheden
Copy link
Contributor

The work there has hardly been tested at all yet. There is basically only a simple test for @Getter and one for @Builder. Adding the @Builder test revealed a few missing pieces, so I suspect there could be more of that.

I have checked that I can run ./gradlew pTML, so if anyone wants to give this a try (only implemented for rewrite-java-17 for now), that would be very appreciated.

@traceyyoshima
Copy link
Contributor

Or Sam will classload the annotation processor and call the relevant methods, instead, haha. I won't have time this week, but I can throw most of the parsing together over the weekend.

@nmck257
Copy link
Collaborator Author

nmck257 commented Oct 22, 2024

I have checked that I can run ./gradlew pTML, so if anyone wants to give this a try (only implemented for rewrite-java-17 for now), that would be very appreciated.

Getting this exception:

[ERROR] Failed to execute goal org.openrewrite.maven:rewrite-maven-plugin:5.43.0-SNAPSHOT:run (default-cli) on project grt-gimfm-security-utils: Execution default-cli of goal org.openrewrite.maven:rewrite-maven-plugin:5.43.0-SNAPSHOT:run failed: Unable to find runtime dependencies beginning with: 'lombok', classpath: [...]

swapping lombok to be an implementation dependency sorted it, but, dunno if we'd prefer a classpathFromResources pattern

@knutwannheden
Copy link
Contributor

Thanks for taking a look! @sambsnyd might have some idea or insights.

@nmck257
Copy link
Collaborator Author

nmck257 commented Oct 22, 2024

edit: I've pushed a change to support @Slf4j to the branch, and no longer see this error on the relevant project

if it's useful with minimal context, here's an error I got when parsing a file which uses @Slfj but doesn't have any other obvious lombok interaction (with the lombok-spike branch).

The field it was parsing is the first occurrence of Map in the file (outside imports).

[DEBUG]
org.openrewrite.java.JavaParsingException: Failed to convert for the following cursor stack:--- BEGIN PATH ---
JCCompilationUnit(sourceFile = /redacted/Foobar.java)
JCClassDecl(name = Foobar, line = 19)
JCVariableDecl(name = foobarVariable, line = 30)
JCTypeApply(line = 31)
--- END PATH ---

    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convert (ReloadableJava17ParserVisitor.java:1705)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convert (ReloadableJava17ParserVisitor.java:1711)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convertAll (ReloadableJava17ParserVisitor.java:1749)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convertTypeParameters (ReloadableJava17ParserVisitor.java:1766)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitParameterizedType (ReloadableJava17ParserVisitor.java:1134)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitParameterizedType (ReloadableJava17ParserVisitor.java:73)
    at com.sun.tools.javac.tree.JCTree$JCTypeApply.accept (JCTree.java:2728)
    at com.sun.source.util.TreePathScanner.scan (TreePathScanner.java:86)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convert (ReloadableJava17ParserVisitor.java:1675)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitVariables (ReloadableJava17ParserVisitor.java:1568)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitVariable (ReloadableJava17ParserVisitor.java:1528)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitVariable (ReloadableJava17ParserVisitor.java:73)
    at com.sun.tools.javac.tree.JCTree$JCVariableDecl.accept (JCTree.java:1045)
    at com.sun.source.util.TreePathScanner.scan (TreePathScanner.java:86)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convert (ReloadableJava17ParserVisitor.java:1675)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convert (ReloadableJava17ParserVisitor.java:1711)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convertStatements (ReloadableJava17ParserVisitor.java:1822)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convertStatements (ReloadableJava17ParserVisitor.java:1799)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitClass (ReloadableJava17ParserVisitor.java:507)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitClass (ReloadableJava17ParserVisitor.java:73)
    at com.sun.tools.javac.tree.JCTree$JCClassDecl.accept (JCTree.java:860)
    at com.sun.source.util.TreePathScanner.scan (TreePathScanner.java:86)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convert (ReloadableJava17ParserVisitor.java:1675)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convertAll (ReloadableJava17ParserVisitor.java:1735)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitCompilationUnit (ReloadableJava17ParserVisitor.java:552)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitCompilationUnit (ReloadableJava17ParserVisitor.java:73)
    at com.sun.tools.javac.tree.JCTree$JCCompilationUnit.accept (JCTree.java:614)
    at com.sun.source.util.TreePathScanner.scan (TreePathScanner.java:86)
    at org.openrewrite.java.isolated.ReloadableJava17Parser.lambda$parseInputs$1 (ReloadableJava17Parser.java:189)
[..]
Caused by: java.lang.StringIndexOutOfBoundsException: begin 2485, end 2485, length 2480
    at java.lang.String.checkBoundsBeginEnd (String.java:4608)
    at java.lang.String.substring (String.java:2711)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convert (ReloadableJava17ParserVisitor.java:1673)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convert (ReloadableJava17ParserVisitor.java:1711)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convertAll (ReloadableJava17ParserVisitor.java:1749)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convertTypeParameters (ReloadableJava17ParserVisitor.java:1766)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitParameterizedType (ReloadableJava17ParserVisitor.java:1134)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitParameterizedType (ReloadableJava17ParserVisitor.java:73)
    at com.sun.tools.javac.tree.JCTree$JCTypeApply.accept (JCTree.java:2728)
    at com.sun.source.util.TreePathScanner.scan (TreePathScanner.java:86)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convert (ReloadableJava17ParserVisitor.java:1675)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitVariables (ReloadableJava17ParserVisitor.java:1568)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitVariable (ReloadableJava17ParserVisitor.java:1528)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitVariable (ReloadableJava17ParserVisitor.java:73)
    at com.sun.tools.javac.tree.JCTree$JCVariableDecl.accept (JCTree.java:1045)
    at com.sun.source.util.TreePathScanner.scan (TreePathScanner.java:86)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convert (ReloadableJava17ParserVisitor.java:1675)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convert (ReloadableJava17ParserVisitor.java:1711)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convertStatements (ReloadableJava17ParserVisitor.java:1822)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convertStatements (ReloadableJava17ParserVisitor.java:1799)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitClass (ReloadableJava17ParserVisitor.java:507)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitClass (ReloadableJava17ParserVisitor.java:73)
    at com.sun.tools.javac.tree.JCTree$JCClassDecl.accept (JCTree.java:860)
    at com.sun.source.util.TreePathScanner.scan (TreePathScanner.java:86)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convert (ReloadableJava17ParserVisitor.java:1675)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convertAll (ReloadableJava17ParserVisitor.java:1735)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitCompilationUnit (ReloadableJava17ParserVisitor.java:552)
    at org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitCompilationUnit (ReloadableJava17ParserVisitor.java:73)
    at com.sun.tools.javac.tree.JCTree$JCCompilationUnit.accept (JCTree.java:614)
    at com.sun.source.util.TreePathScanner.scan (TreePathScanner.java:86)
    at org.openrewrite.java.isolated.ReloadableJava17Parser.lambda$parseInputs$1 (ReloadableJava17Parser.java:189)
[...]

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

Successfully merging this pull request may close these issues.

4 participants