Skip to content

Conversation

Jenson3210
Copy link
Contributor

@Jenson3210 Jenson3210 commented Jul 30, 2025

What's changed?

Updated the antlr code to allow for wildcard parameters similarly to dot_dot.

What's your motivation?

Before, the existing test used to work due to the following:

  • * gets regexed by the StringUtils.aspectjNameToPattern("*") to [^.]+.
  • Foo.bar() was a non existing method => type information <unknown> does not contain a . so the matcher works.
  • The moment you add type information by returning a string for a static method in the test, the test fails.
Screenshot 2025-07-30 at 11 50 51

The current code behavior: see the <unknown> and [^.]+

Screenshot 2025-07-30 at 11 58 40

Changing the test adding type information

Screenshot 2025-07-30 at 12 00 00

See how that regex would no longer match?

Anyone you would like to review specifically?

@greg-at-moderne

Have you considered any alternatives or workarounds?

We could alter the getRegex on FormalType to avoid changes to anltr code but that's not correct according the semantics of the code.

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

@Jenson3210 Jenson3210 self-assigned this Jul 30, 2025
@Jenson3210 Jenson3210 added the bug Something isn't working label Jul 30, 2025
@github-project-automation github-project-automation bot moved this to In Progress in OpenRewrite Jul 30, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • rewrite-java/src/main/java/org/openrewrite/java/internal/grammar/MethodSignatureParser.java
    • lines 1059-1059

@Jenson3210 Jenson3210 marked this pull request as ready for review July 30, 2025 14:10
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • rewrite-java/src/main/java/org/openrewrite/java/internal/grammar/MethodSignatureParser.java
    • lines 1059-1059

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • rewrite-java/src/main/java/org/openrewrite/java/internal/grammar/MethodSignatureParser.java
    • lines 1059-1059

@openrewrite openrewrite deleted a comment from github-actions bot Jul 30, 2025
@openrewrite openrewrite deleted a comment from jevanlingen Jul 30, 2025
@openrewrite openrewrite deleted a comment from github-actions bot Jul 30, 2025
@openrewrite openrewrite deleted a comment from github-actions bot Jul 30, 2025
@openrewrite openrewrite deleted a comment from github-actions bot Jul 30, 2025
@openrewrite openrewrite deleted a comment from github-actions bot Jul 30, 2025
@openrewrite openrewrite deleted a comment from github-actions bot Jul 30, 2025
@openrewrite openrewrite deleted a comment from github-actions bot Jul 30, 2025
@openrewrite openrewrite deleted a comment from github-actions bot Jul 30, 2025
@openrewrite openrewrite deleted a comment from github-actions bot Jul 30, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Ready to Review in OpenRewrite Jul 30, 2025
@Jenson3210
Copy link
Contributor Author

@timtebeek Do I add this one to the list of core changes? As MethodMatcher is quite broadly used. The tests are quite good though, so not sure.

@timtebeek
Copy link
Member

Yes please add to the list of core issues to review! 🙏🏻

Copy link
Member

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Approved from my side already; added another test to show the difference pointed out in PR comments when type information for Foo.bar() is present, but for Assert.assertTrue likely still absent.

Let's wait for that final review before we merge.

@sambsnyd sambsnyd merged commit 8d3b360 into main Aug 4, 2025
2 checks passed
@sambsnyd sambsnyd deleted the methodmatcher-support-wildcard branch August 4, 2025 23:11
@github-project-automation github-project-automation bot moved this from Ready to Review to Done in OpenRewrite Aug 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants