Introduce `ErrorProneRuntimeClasspath` check #882

Stephan202 · 2023-11-13T05:19:29Z

~~❗ This PR is on top of #881. ❗~~

Suggested commit message:

Introduce `ErrorProneRuntimeClasspath` check (#882)

Prefer "type-safe" type references were possible, but use string
literals if the references type may not be available at runtime.

The PR is split into three commits that can best be reviewed one-by-one: the check, its automated suggestions, and manual tweaks.

github-actions · 2023-11-13T05:22:56Z

Surviving mutants in this change: 1
Killed mutants in this change: 43

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.ErrorProneRuntimeClasspath`	1	42
🎉`tech.picnic.errorprone.bugpatterns.CollectorMutability`	0	1

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-11-13T05:36:44Z

Looks good. All 44 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.CollectorMutability`	0	1
🎉`tech.picnic.errorprone.bugpatterns.ErrorProneRuntimeClasspath`	0	43

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

sonarcloud · 2023-11-13T07:33:58Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

98.7% Coverage
0.0% Duplication

github-actions · 2023-11-13T07:35:10Z

Looks good. All 44 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.CollectorMutability`	0	1
🎉`tech.picnic.errorprone.bugpatterns.ErrorProneRuntimeClasspath`	0	43

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2023-12-18T12:01:49Z

I've started to resolve the conflicts here. Almost done; will finalize after some meetings.

Stephan202 · 2023-12-18T14:25:24Z

Rebased; the PR now targets master and is ready for review.

github-actions · 2023-12-18T14:26:34Z

Looks good. All 43 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.ErrorProneRuntimeClasspath`	0	43

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-12-18T14:27:10Z

Looks good. All 43 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.ErrorProneRuntimeClasspath`	0	43

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2023-12-18T17:09:09Z

Once again rebased and resolved a conflict. @rickie and @mohamedsamehsalah I suspect this'll start happening more ;)

github-actions · 2023-12-18T17:10:58Z

Looks good. All 43 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.ErrorProneRuntimeClasspath`	0	43

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

mohamedsamehsalah

Nothing stands out for me

Is there a general rule to follow when determining if a class is available on run-time or not ?

Stephan202

I added one more commit, in which I resorted a few things. The only place I didn't reorder is the listing in JUnitValueSource, as there the existing order seems more intuitive.

Is there a general rule to follow when determining if a class is available on run-time or not ?

Thanks for reviewing @mohamedsamehsalah! I'm not sure about a "general rule", but we use this code to check whether a given class is "currently" on the classpath. That of course doesn't mean that a class will also be on the classpath when the code is executed at a later point in time. For example, we have a bunch of provided dependencies that may not actually be "provided" by downstream users. That's why the ErrorProneRuntimeClasspath check restricts itself to types that match the CLASSPATH_TYPES regex: those are known to be dependencies of Error Prone, and thus ~guaranteed to be present. I've also added a comment making this more explicit :)

github-actions · 2023-12-18T21:08:35Z

Looks good. All 43 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.ErrorProneRuntimeClasspath`	0	43

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspath.java

rickie

Cool PR, sorry for delay in review :). 🚀

rickie · 2023-12-19T22:13:16Z

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspath.java

+  }
+
+  @Override
+  public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {


Interesting, AFAIKT we always start with the two overrides, right?

Indeed. Here I think the idea was to have two "groups" methods, one for each "direction", but I see now that this is not how it turned out. Will move 👍

rickie · 2023-12-20T10:46:40Z

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspath.java

+   */
+  private static final Pattern CLASSPATH_TYPES =
+      Pattern.compile(
+          "com\\.google\\.common\\..*|com\\.google\\.errorprone\\.([^\\.]+(?<!TestHelper)(\\..*)?)|java\\..*");


Suggested change

"com\\.google\\.common\\..*|com\\.google\\.errorprone\\.([^\\.]+(?<!TestHelper)(\\..*)?)|java\\..*");

"com\\.google\\.common\\..*|com\\.google\\.errorprone\\.([^.]+(?<!TestHelper)(\\..*)?)|java\\..*");

Test passes without these 👀. Seems redundant.

rickie

Added the commit as it can probably be improved. Other issue is not a blocker for me so already approving.

github-actions · 2023-12-20T11:58:14Z

Looks good. All 43 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.ErrorProneRuntimeClasspath`	0	43

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Prefer "type-safe" type references were possible, but use string literals if the references type may not be available at runtime.

Stephan202

Rebased and added a commit.

Ideally we move this check, and some like it, to a separate Maven module, so that downstream users won't have to disable it. But that's something we can/should pick up separately.

Stephan202 · 2023-12-20T13:44:01Z

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspath.java

+  }
+
+  @Override
+  public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {


Indeed. Here I think the idea was to have two "groups" methods, one for each "direction", but I see now that this is not how it turned out. Will move 👍

sonarcloud · 2023-12-20T13:48:46Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
98.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2023-12-20T13:49:48Z

Looks good. All 43 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.ErrorProneRuntimeClasspath`	0	43

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 added this to the 0.15.0 milestone Nov 13, 2023

Stephan202 requested a review from rickie November 13, 2023 05:19

Stephan202 mentioned this pull request Nov 13, 2023

Introduce UnqualifiedSuggestedFixImport check #880

Merged

rickie force-pushed the sschroevers/canonical-class-name-usage branch from 2e67804 to 1fa3951 Compare December 5, 2023 12:55

rickie requested a review from mohamedsamehsalah December 13, 2023 07:57

Stephan202 force-pushed the sschroevers/canonical-class-name-usage branch from 1fa3951 to 2695c4d Compare December 16, 2023 16:21

rickie force-pushed the sschroevers/canonical-class-name-usage branch from 2695c4d to 9307a2e Compare December 18, 2023 08:06

Base automatically changed from sschroevers/canonical-class-name-usage to master December 18, 2023 08:19

Stephan202 force-pushed the sschroevers/error-prone-runtile-classpath branch 2 times, most recently from 90f58c2 to ec1ce7e Compare December 18, 2023 14:25

Stephan202 force-pushed the sschroevers/error-prone-runtile-classpath branch from ec1ce7e to 748b70f Compare December 18, 2023 17:08

mohamedsamehsalah approved these changes Dec 18, 2023

View reviewed changes

Stephan202 commented Dec 18, 2023

View reviewed changes

mohamedsamehsalah reviewed Dec 19, 2023

View reviewed changes

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneRuntimeClasspath.java Show resolved Hide resolved

rickie added the new feature label Dec 19, 2023

rickie reviewed Dec 20, 2023

View reviewed changes

rickie approved these changes Dec 20, 2023

View reviewed changes

Introduce ErrorProneRuntimeClasspath check

29dc33c

Prefer "type-safe" type references were possible, but use string literals if the references type may not be available at runtime.

Stephan202 and others added 7 commits December 20, 2023 14:36

Apply automated changes

17088d7

Manual tweaks

7ffa6ac

Kill final mutant

492ac0e

Resolve some SonarCloud warnings

f7acb3c

Tweaks

a8e16ee

Suggestion

a1846e4

Move the method

1ce6800

Stephan202 force-pushed the sschroevers/error-prone-runtile-classpath branch from 59bb665 to 1ce6800 Compare December 20, 2023 13:47

Stephan202 commented Dec 20, 2023

View reviewed changes

rickie merged commit d569156 into master Dec 20, 2023
16 checks passed

rickie deleted the sschroevers/error-prone-runtile-classpath branch December 20, 2023 21:14

Stephan202 changed the title ~~Introduce ErrorProneRuntimeClasspath check~~ Introduce ErrorProneRuntimeClasspath check Feb 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `ErrorProneRuntimeClasspath` check #882

Introduce `ErrorProneRuntimeClasspath` check #882

Stephan202 commented Nov 13, 2023 •

edited

Loading

github-actions bot commented Nov 13, 2023

github-actions bot commented Nov 13, 2023

sonarcloud bot commented Nov 13, 2023

github-actions bot commented Nov 13, 2023

Stephan202 commented Dec 18, 2023

Stephan202 commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

Stephan202 commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

mohamedsamehsalah left a comment

Stephan202 left a comment

github-actions bot commented Dec 18, 2023

rickie left a comment

rickie Dec 19, 2023

Stephan202 Dec 20, 2023

rickie Dec 20, 2023

rickie left a comment

github-actions bot commented Dec 20, 2023

Stephan202 left a comment

Stephan202 Dec 20, 2023

sonarcloud bot commented Dec 20, 2023

github-actions bot commented Dec 20, 2023

	"com\\.google\\.common\\..\|com\\.google\\.errorprone\\.([^\\.]+(?<!TestHelper)(\\..)?)\|java\\..*");
	"com\\.google\\.common\\..\|com\\.google\\.errorprone\\.([^.]+(?<!TestHelper)(\\..)?)\|java\\..*");

Introduce ErrorProneRuntimeClasspath check #882

Introduce ErrorProneRuntimeClasspath check #882

Conversation

Stephan202 commented Nov 13, 2023 • edited Loading

github-actions bot commented Nov 13, 2023

github-actions bot commented Nov 13, 2023

sonarcloud bot commented Nov 13, 2023

github-actions bot commented Nov 13, 2023

Stephan202 commented Dec 18, 2023

Stephan202 commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

Stephan202 commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

mohamedsamehsalah left a comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 18, 2023

rickie left a comment

Choose a reason for hiding this comment

rickie Dec 19, 2023

Choose a reason for hiding this comment

Stephan202 Dec 20, 2023

Choose a reason for hiding this comment

rickie Dec 20, 2023

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 20, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Dec 20, 2023

Choose a reason for hiding this comment

sonarcloud bot commented Dec 20, 2023

Quality Gate passed

github-actions bot commented Dec 20, 2023

Introduce `ErrorProneRuntimeClasspath` check #882

Introduce `ErrorProneRuntimeClasspath` check #882

Stephan202 commented Nov 13, 2023 •

edited

Loading