Update comments referencing Refaster rule limitation

[rickie]

While applying error-prone-support on repositories within Picnic we found that for some templates we lose generic type information. Therefore, we opened a PR upstream google/error-prone#2706 to fix this. The problem is that for method invocations the type arguments were simply ignored by Refaster in the @AfterTemplates.

Our fork cherry-picked the commit from the upstream repository. In addition, we also use the fork for Picnic internally.
However, currently we are not on the latest version due to some issues.

This should prepare the code for when we upgrade EPS and PSM to v2.11.0-picnic-1 or 2.11.1 (which doesn't exist yet).

[Badbond]

Not completely comfortable with reviewing this as I was not in the loop of these problems (type arguments, wiremock, other blockers) and their fixes. Nor do I have a clear overview of which state currently the fork is in and what is currently blocking us. Brain is a bit depleted atm to really get a sense of this.

Do have some feedback that we can discuss already though.

[Badbond]

I would still keep an XXX here. Now it looks like just class Javadoc; not something to take action upon.

Also, see suggestion: Right? I was so confused. Copied this from suggested commit message in upstream.

Suggested change

	* <p>Picnic's Error Prone fork supports inlining of method arguments in the `@AfterTemplate`.
	* <p>Picnic's Error Prone fork supports method invocation type argument inlining in the `@AfterTemplate`.

These two suggestions apply to all changes.

[rickie]

Now it looks like just class Javadoc; not something to take action upon.

That was exactly what was my intention 😄.

Both you and @Stephan202 mention that it should be an XXX so I updated the Javadoc and added the feedback 😉.

Btw, let me give some extra context on this tomorrow :).

[Stephan202]

Updated the milestone. As discussed offline, we'll merge this PR once error-prone-support depends on a version of Error Prone (or our fork) that includes google/error-prone#2706.

[Stephan202]

Dropped this ticket from the upcoming milestone for now. Not because we don't want this change, but because we first need to formulate a solution to the "serialization compatibility question".

[github-actions]

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[rickie]

I looked into this again. Rebased it and tried some things. However, I think we need to have a HasTypeArgumentsMatcher or something like that. Now it unconditionally introduces the type arguments, that is not the goal I would say. So we need to have two Refaster rules per case and use the @{Not,}Matches to make sure we don't introduce it everytime.

WDYT @Stephan202?

[rickie]

Well, you are right (discussed here) and indeed we cannot use a Matcher for this. In that case I'm not sure how we can best approach this. I think we need a BugChecker for this that only introduces the type arguments if we need it. If we would proceed with this PR we would introduce this while it's not necessary. Other option would be to create a check that detects we don't need the type arguments....

[Stephan202]

Other option would be to create a check that detects we don't need the type arguments....

That sounds more complex, but on the plus side would enable also other cleanup. I haven't looked into it deeply, but it would amount to identifying method invocations that (a) specify explicit type parameters and (b) invoke a specific method overload that, absent such type parameters, would also be selected by the algorithm defined by JLS 18.5.2. For (b) we'd of course rely on a few heuristics; just enough to avoid false positives while still covering the cases discussed here.

[Stephan202]

I synced this branch and added two commits:

• One to drop HasTypeArgumentsMatcher, as we don't have uses for it.
• One to revert other changes, and to instead update comments that referenced Refaster: support method invocation type argument inlining google/error-prone#2706.

Rationale is that google/error-prone#2706 is no longer a blocker, and that instead we should make up our mind about whether to propagate the type parameters.

I suggest merging this PR with the following commit message:

Update comments referencing Refaster rule limitation (#46)

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

[github-actions]

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[mohamedsamehsalah]

✏️

[github-actions]

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[rickie]

Thanks a lot @Stephan202 for picking up this old one. I think this is the right approach for now. It's an interesting case.

I encountered this case a lot when trying to implement the Guava integration tests.

I'd say lets merge this one :), updated the branch (might be offline in a bit when its ready to merge)