Refaster: support method invocation type argument inlining #2706

rickie · 2021-11-23T20:05:32Z

No description provided.

rickie · 2021-11-23T20:06:52Z

core/src/test/java/com/google/errorprone/refaster/UMethodInvocationTest.java

@@ -41,7 +41,8 @@ public void match() {
    ULiteral oneLit = ULiteral.intLit(1);
    ULiteral barLit = ULiteral.stringLit("bar");
    UMethodInvocation invocation =
-        UMethodInvocation.create(fooIdent, ImmutableList.<UExpression>of(oneLit, barLit));
+        UMethodInvocation.create(


Should we extend these tests or leave it as is?

The unification logic does not consider the type parameters (should it?), so I guess right now there's nothing to test. Maybe we can use the varargs overload instead:

UMethodInvocation.create(fooIdent, oneLit, barLit);

(But not sure that makes the test easier to understand 🤔.)

That said, perhaps we could add an extra inline test below, e.g. one that handles two type arguments (since the Refaster test further down already covers the one-type argument case).

I tried adding the typeArguments to the unification in UMethodInvocation. However, the ImplicitTypesInlinedTemplate fails after doing so, specifically on this example.
The unification fails simply because the expression in the @BeforeTemplate does not have type arguments, but the matched code does.

We could try to define the semantics in such a way that the unification process also accounts for implicit type arguments, but it feels like a bridge too far for this PR. So I would suggest to leave it as is.

Let me know what you think.

Stephan202

Maybe we can provide a motivating example for this feature. It really helped during your thesis work to ensure that Refaster-patched code still compiled. An example where this was useful, was the wrapping of expressions that implemented a functional interface. In some cases these expressions were method references or lambda expressions whose exact signature was determined by their context. Wrapping such expression without specifying explicit type parameters (inferred from context) would lead to non-compilable code.

Stephan202 · 2021-11-23T20:20:27Z

.../com/google/errorprone/refaster/testdata/template/TypeArgumentsMethodInvocationTemplate.java

+
+  @AfterTemplate
+  boolean after(String string) {
+    return string.<String>isEmpty();


Discussed offline: surprisingly this is valid code (TIL!), but let's use a realistic example, one with a method that actually accepts a generic type parameter. (For example, ExecutorService#submit.)

Yes, just pushed the update, now it uses a realistic example.

Stephan202 · 2021-11-23T20:25:21Z

core/src/test/java/com/google/errorprone/refaster/UMethodInvocationTest.java

@@ -41,7 +41,8 @@ public void match() {
    ULiteral oneLit = ULiteral.intLit(1);
    ULiteral barLit = ULiteral.stringLit("bar");
    UMethodInvocation invocation =
-        UMethodInvocation.create(fooIdent, ImmutableList.<UExpression>of(oneLit, barLit));
+        UMethodInvocation.create(


The unification logic does not consider the type parameters (should it?), so I guess right now there's nothing to test. Maybe we can use the varargs overload instead:

UMethodInvocation.create(fooIdent, oneLit, barLit);

(But not sure that makes the test easier to understand 🤔.)

That said, perhaps we could add an extra inline test below, e.g. one that handles two type arguments (since the Refaster test further down already covers the one-type argument case).

Stephan202

Rebased. The test isn't "exciting", but it works 😄

Suggested commit message:

Refaster: support method invocation type argument inlining

I think this PR is ready for review by the Error Prone team; it'd be nice to bring support for method invocations on par with constructor invocations when it comes to type arguments.

rickie · 2022-01-13T12:04:06Z

Within de Picnic codebase we found more situations where "losing" type arguments during a rewrite can lead to non-compilable code. In the example below the explicit type arguments in ImmutableMap.<K, V>builder() currently don't have any effect, as Refaster currently does not support inlining such type arguments for method invocations.

static final class ImmutableMapBuilder<K, V> {
  @BeforeTemplate
  ImmutableMap.Builder<K, V> before() {
    return new ImmutableMap.Builder<>();
  }

  @AfterTemplate
  ImmutableMap.Builder<K, V> after() {
    return ImmutableMap.<K, V>builder();
  }
}

The changes in this PR resolve this issue.
If desired we can update the test in this PR to use the above example instead of the existing ExecutorService#submit example. @cushon if any other changes are required let us know :).

Stephan202 · 2022-01-26T17:24:48Z

Rebased and squashed the two commits, as I'll cherry-pick the changes of this PR to our fork. That said: still interested in seeing this included in mainline Error Prone 🙃.

See google#2706.

rickie · 2022-04-15T07:58:07Z

Rebased. Still curious, could this change still get into mainline Error Prone 😄?

See google#2706.

rickie · 2022-04-26T13:13:21Z

Rebased and added a commit.

See google#2706.

rickie · 2022-07-07T07:20:36Z

Rebased. Is it possible to get this change into mainline Error Prone 😄?

See google#2706.

rickie · 2022-09-26T10:00:49Z

Rebased.

See google#2706.

rickie · 2023-01-03T08:26:08Z

In Error Prone Support we have quite some Refaster rules. Some of them (examples 1 and 2, ideally would be written like this) can lead to non-compilable code because of this. Therefore it would still help a lot if this PR would be merged.

Could you take a look at this @cushon? We're curious to hear what you think.

cushon

Sorry for not getting to this sooner. I did some testing with our internal refactor templates, and it looks good. Thanks for the PR!

See google#2706.

rickie · 2023-01-05T21:30:09Z

Oh nice, good to hear! 😄

I added a commit to update the year numbers in the license headers and rebased the PR.

Fixes #2706 FUTURE_COPYBARA_INTEGRATE_REVIEW=#2706 from PicnicSupermarket:rossendrijver/type_arguments_umethodinvocation f44c1e6 PiperOrigin-RevId: 499956986

This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [com.google.errorprone:error_prone_core](https://errorprone.info) ([source](https://github.com/google/error-prone)) | | minor | `2.16` -> `2.18.0` | | [com.google.errorprone:error_prone_annotations](https://errorprone.info) ([source](https://github.com/google/error-prone)) | compile | minor | `2.16` -> `2.18.0` | | [org.apache.maven.plugins:maven-failsafe-plugin](https://maven.apache.org/surefire/) | build | patch | `3.0.0-M7` -> `3.0.0-M8` | | [org.apache.maven.plugins:maven-surefire-plugin](https://maven.apache.org/surefire/) | build | patch | `3.0.0-M7` -> `3.0.0-M8` | --- ### Release Notes <details> <summary>google/error-prone</summary> ### [`v2.18.0`](https://github.com/google/error-prone/releases/tag/v2.18.0): Error Prone 2.18.0 [Compare Source](google/error-prone@v2.17.0...v2.18.0) New Checkers: - [`InjectOnBugCheckers`](https://errorprone.info/bugpattern/InjectOnBugCheckers) - [`LabelledBreakTarget`](https://errorprone.info/bugpattern/LabelledBreakTarget) - [`UnusedLabel`](https://errorprone.info/bugpattern/UnusedLabel) - [`YodaCondition`](https://errorprone.info/bugpattern/YodaCondition) Fixes issues: [#1650](google/error-prone#1650), [#2706](google/error-prone#2706), [#3404](google/error-prone#3404), [#3493](google/error-prone#3493), [#3504](google/error-prone#3504), [#3519](google/error-prone#3519), [#3579](google/error-prone#3579), [#3610](google/error-prone#3610), [#3632](google/error-prone#3632), [#3638](google/error-prone#3638), [#3645](google/error-prone#3645), [#3646](google/error-prone#3646), [#3652](google/error-prone#3652), [#3690](google/error-prone#3690) **Full Changelog**: google/error-prone@v2.17.0...v2.18.0 ### [`v2.17.0`](https://github.com/google/error-prone/releases/tag/v2.17.0): Error Prone 2.17.0 [Compare Source](google/error-prone@v2.16...v2.17.0) New Checkers: - [`AvoidObjectArrays`](https://errorprone.info/bugpattern/AvoidObjectArrays) - [`Finalize`](https://errorprone.info/bugpattern/Finalize) - [`IgnoredPureGetter`](https://errorprone.info/bugpattern/IgnoredPureGetter) - [`ImpossibleNullComparison`](https://errorprone.info/bugpattern/ProtoFieldNullComparison) - [`MathAbsoluteNegative`](https://errorprone.info/bugpattern/MathAbsoluteNegative) - [`NewFileSystem`](https://errorprone.info/bugpattern/NewFileSystem) - [`StatementSwitchToExpressionSwitch`](https://errorprone.info/bugpattern/StatementSwitchToExpressionSwitch) - [`UnqualifiedYield`](https://errorprone.info/bugpattern/UnqualifiedYield) Fixed issues: [#2321](google/error-prone#2321), [#3144](google/error-prone#3144), [#3297](google/error-prone#3297), [#3428](google/error-prone#3428), [#3437](google/error-prone#3437), [#3462](google/error-prone#3462), [#3482](google/error-prone#3482), [#3494](google/error-prone#3494) **Full Changelog**: google/error-prone@v2.16...v2.17.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Issue google/error-prone#2706 has been resolved; we have yet to decide how to proceed.

google-cla bot added the cla: yes label Nov 23, 2021

rickie commented Nov 23, 2021

View reviewed changes

Stephan202 reviewed Nov 23, 2021

View reviewed changes

Stephan202 force-pushed the rossendrijver/type_arguments_umethodinvocation branch from 5538a86 to fa69dcf Compare December 10, 2021 07:01

Stephan202 approved these changes Dec 10, 2021

View reviewed changes

rickie changed the title ~~Support inlining TypeArguments of UMethodInvocations in @AfterTemplates~~ Refaster: support method invocation type argument inlining Jan 13, 2022

Stephan202 force-pushed the rossendrijver/type_arguments_umethodinvocation branch from fa69dcf to e69ce80 Compare January 26, 2022 17:23

Stephan202 pushed a commit to PicnicSupermarket/error-prone that referenced this pull request Jan 26, 2022

Refaster: support method invocation type argument inlining

edf8ae8

See google#2706.

rickie mentioned this pull request Mar 31, 2022

Update comments referencing Refaster rule limitation PicnicSupermarket/error-prone-support#46

Merged

Stephan202 pushed a commit to PicnicSupermarket/error-prone that referenced this pull request Apr 13, 2022

Refaster: support method invocation type argument inlining

f4bc725

See google#2706.

Stephan202 pushed a commit to PicnicSupermarket/error-prone that referenced this pull request Apr 13, 2022

Refaster: support method invocation type argument inlining

93a9c51

See google#2706.

Stephan202 pushed a commit to PicnicSupermarket/error-prone that referenced this pull request Apr 15, 2022

Refaster: support method invocation type argument inlining

669c544

See google#2706.

rickie force-pushed the rossendrijver/type_arguments_umethodinvocation branch from e69ce80 to c460df0 Compare April 15, 2022 07:54

Stephan202 pushed a commit to PicnicSupermarket/error-prone that referenced this pull request Apr 16, 2022

Refaster: support method invocation type argument inlining

614b292

See google#2706.

rickie force-pushed the rossendrijver/type_arguments_umethodinvocation branch from c460df0 to fcb9e09 Compare April 26, 2022 11:30

rickie added a commit to PicnicSupermarket/error-prone that referenced this pull request May 5, 2022

Refaster: support method invocation type argument inlining

d8c41ef

See google#2706.

rickie force-pushed the rossendrijver/type_arguments_umethodinvocation branch from d7d8e1b to d8c41ef Compare May 5, 2022 16:34

Stephan202 pushed a commit to PicnicSupermarket/error-prone that referenced this pull request Jun 5, 2022

Refaster: support method invocation type argument inlining

c71c609

See google#2706.

Stephan202 mentioned this pull request Jun 5, 2022

Upgrade Error Prone 2.10.0 -> 2.14.0 and errorprone-slf4j 0.1.4 -> 0.1.12 PicnicSupermarket/error-prone-support#76

Merged

1 task

rickie added a commit to PicnicSupermarket/error-prone that referenced this pull request Jul 7, 2022

Refaster: support method invocation type argument inlining

2853def

See google#2706.

rickie force-pushed the rossendrijver/type_arguments_umethodinvocation branch from d8c41ef to 2853def Compare July 7, 2022 06:59

rickie added a commit to PicnicSupermarket/error-prone that referenced this pull request Aug 8, 2022

Refaster: support method invocation type argument inlining

b88e074

See google#2706.

rickie force-pushed the rossendrijver/type_arguments_umethodinvocation branch from 2853def to b88e074 Compare August 8, 2022 07:02

rickie added a commit to PicnicSupermarket/error-prone that referenced this pull request Sep 2, 2022

Refaster: support method invocation type argument inlining

c51221f

See google#2706.

rickie force-pushed the rossendrijver/type_arguments_umethodinvocation branch from b88e074 to c51221f Compare September 2, 2022 08:30

rickie added a commit to PicnicSupermarket/error-prone-support that referenced this pull request Sep 21, 2022

Update XXX comments to reference google/error-prone#2706 (#249)

43bcbea

rickie added a commit to PicnicSupermarket/error-prone that referenced this pull request Sep 26, 2022

Refaster: support method invocation type argument inlining

e0ee9a1

See google#2706.

rickie force-pushed the rossendrijver/type_arguments_umethodinvocation branch from c51221f to e0ee9a1 Compare September 26, 2022 10:00

rickie added a commit to PicnicSupermarket/error-prone that referenced this pull request Jan 3, 2023

Refaster: support method invocation type argument inlining

f44c1e6

See google#2706.

rickie force-pushed the rossendrijver/type_arguments_umethodinvocation branch from e0ee9a1 to f44c1e6 Compare January 3, 2023 07:57

cushon approved these changes Jan 5, 2023

View reviewed changes

rickie and others added 2 commits January 5, 2023 22:27

Refaster: support method invocation type argument inlining

3d6e2c6

See google#2706.

Update the year to 2023 in license header

da1413a

rickie force-pushed the rossendrijver/type_arguments_umethodinvocation branch from f44c1e6 to da1413a Compare January 5, 2023 21:28

copybara-service bot mentioned this pull request Jan 7, 2023

Refaster: support method invocation type argument inlining #3702

Closed

copybara-service bot closed this in 7f459e1 Jan 7, 2023

Stephan202 deleted the rossendrijver/type_arguments_umethodinvocation branch January 7, 2023 11:20

Stephan202 pushed a commit to PicnicSupermarket/error-prone-support that referenced this pull request Dec 27, 2024

Update comments referencing Refaster rule limitation (#46)

7daa39a

Issue google/error-prone#2706 has been resolved; we have yet to decide how to proceed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refaster: support method invocation type argument inlining #2706

Refaster: support method invocation type argument inlining #2706

rickie commented Nov 23, 2021 •

edited

Loading

rickie Nov 23, 2021

Stephan202 Nov 23, 2021

rickie Dec 2, 2021 •

edited

Loading

Stephan202 left a comment

Stephan202 Nov 23, 2021

rickie Dec 2, 2021

Stephan202 Nov 23, 2021

Stephan202 left a comment

rickie commented Jan 13, 2022

Stephan202 commented Jan 26, 2022

rickie commented Apr 15, 2022

rickie commented Apr 26, 2022

rickie commented Jul 7, 2022

rickie commented Sep 26, 2022

rickie commented Jan 3, 2023

cushon left a comment

rickie commented Jan 5, 2023

Refaster: support method invocation type argument inlining #2706

Refaster: support method invocation type argument inlining #2706

Conversation

rickie commented Nov 23, 2021 • edited Loading

rickie Nov 23, 2021

Choose a reason for hiding this comment

Stephan202 Nov 23, 2021

Choose a reason for hiding this comment

rickie Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Nov 23, 2021

Choose a reason for hiding this comment

rickie Dec 2, 2021

Choose a reason for hiding this comment

Stephan202 Nov 23, 2021

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

rickie commented Jan 13, 2022

Stephan202 commented Jan 26, 2022

rickie commented Apr 15, 2022

rickie commented Apr 26, 2022

rickie commented Jul 7, 2022

rickie commented Sep 26, 2022

rickie commented Jan 3, 2023

cushon left a comment

Choose a reason for hiding this comment

rickie commented Jan 5, 2023

rickie commented Nov 23, 2021 •

edited

Loading

rickie Dec 2, 2021 •

edited

Loading