Prefer Guava version of VisibleForTesting annotation #2505

ash211 · 2023-03-06T22:27:50Z

Before this PR

There are multiple versions of the @VisibleForTesting annotation, and if a developer isn't careful they can inadvertently import a version of that annotation besides the main one from Guava.

After this PR

==COMMIT_MSG==
Prefer Guava version of VisibleForTesting annotation
==COMMIT_MSG==

This PR makes importing

Possible downsides?

Repos with -Werror that trip this warning will now fail to compile. However, they have opted into this behavior by enabling -Werror and their CI checks should prevent gradle-baseline auto-upgrades from breaking their main branches. However, they would then be stuck on the previous version of gradle-baseline, and not receive future updates to gradle-baseline automatically via excavator auto-bumps.

changelog-app · 2023-03-06T22:27:53Z

Generate changelog in `changelog/@unreleased`

Type

Description

Prefer Guava version of VisibleForTesting annotation

Check the box to generate changelog(s)

Generate changelog entry

...error-prone/src/main/java/com/palantir/baseline/errorprone/PreferGuavaVisibleForTesting.java

pkoenig10 · 2023-03-06T22:33:31Z

...error-prone/src/main/java/com/palantir/baseline/errorprone/PreferGuavaVisibleForTesting.java

+    private static final Set<String> OTHER_VFTS = Set.of(
+            "org.apache.hadoop.thirdparty.com.google.common.annotations.VisibleForTesting",
+            "org.jetbrains.annotations.VisibleForTesting",
+            "com.facebook.react.common.annotations.VisibleForTesting",
+            "org.apache.arrow.util.VisibleForTesting",
+            "org.assertj.core.util.VisibleForTesting",
+            "org.apache.flink.annotation.VisibleForTesting",
+            "com.cronutils.utils.VisibleForTesting",
+            "com.google.api.client.repackaged.com.google.common.annotations.VisibleForTesting",
+            "org.sparkproject.guava.annotations.VisibleForTesting",
+            "org.apache.hadoop.classification.VisibleForTesting",
+            "org.jheaps.annotations.VisibleForTesting",
+            "io.debezium.annotation.VisibleForTesting",
+            "graphql.VisibleForTesting",
+            "avro.shaded.com.google.common.annotations.VisibleForTesting");


I'd prefer to avoid explicitly listing these and just forbid any annotation named "VisibleForTesting" that isn't the error-prone one.

Agree with Patrick, I think we may also want to consider blocking some of these repackaged imports in the baseline checkstyle as well by expanding IllegalImports:

gradle-baseline/.baseline/checkstyle/checkstyle.xml

Lines 127 to 132 in f797c67

<module name="IllegalImport"> 

<property name="id" value="BanShadedClasses"/>

<property name="illegalPkgs" value=".*\.(repackaged|shaded|thirdparty)"/>

<property name="regexp" value="true"/>

<message key="import.illegal" value="Must not import repackaged classes."/>

</module>

Removed the deny list of imports and instead now look for all imports with the same class name.

Is it a requirement to change Checkstyle at the same time as ErrorProne, or can they be independent? The EP change seems better to me, because it can suggest and apply fixes.

pkoenig10 · 2023-03-06T22:35:05Z

...error-prone/src/main/java/com/palantir/baseline/errorprone/PreferGuavaVisibleForTesting.java

+ */
+@AutoService(BugChecker.class)
+@BugPattern(summary = "Prefer Guava's @VisibleForTesting over other versions.", severity = SeverityLevel.WARNING)
+public final class PreferGuavaVisibleForTesting extends BugChecker implements ImportTreeMatcher {


These are Error-Prone annotations, not Guava

Suggested change

public final class PreferGuavaVisibleForTesting extends BugChecker implements ImportTreeMatcher {

public final class PreferErrorProneAnnotations extends BugChecker implements ImportTreeMatcher {

WDYT about reframing/restructuring this as a general PreferErrorProneAnnotations check so we can add other annotations here in the future?

I'm pretty sure that VisibleForTesting exists in standard guava without error-prone annotation apis.

@VisibleForTesting has been in standard Guava since was renamed from Google-collections in 2010.

We may want to consider something like PreferCommonAnnotations to steer folks to preferred annotations like cases like this, nullness, etc.

We'll likely be deconflicting between checker-framework,
jspecify, guava, legacy jsr305 & findbugs/spotbugs, etc. and ideally consolidating on the smallest combination of these.

Ah sorry, I didn't look closely enough here. I thought this was for an EP annotation.

Changed to PreferCommonAnnotations. If there are other annotations like VisibleForTesting we'd like to add now, please lmk. I don't know of others, and am happy to get just this one in.

...error-prone/src/main/java/com/palantir/baseline/errorprone/PreferGuavaVisibleForTesting.java

schlosna · 2023-03-08T01:42:33Z

Minor perf suggestion

...line-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCommonAnnotations.java

schlosna

seems reasonable to me

schlosna · 2023-03-08T06:26:06Z

...-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCommonAnnotationsTest.java

+                        "import org.jetbrains.annotations.*;",
+                        "public final class Client {",
+                        // NOTE: wildcard-imported annotations are not currently re-written
+                        "  @VisibleForTesting",


this seems ok for now given we block wildcard imports

schlosna · 2023-03-08T06:27:17Z

...-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferCommonAnnotationsTest.java

+                        // NOTE: fully-qualified annotations are not currently re-written
+                        "  @org.jetbrains.annotations.VisibleForTesting",


seems reasonable to allow fully qualified as method to explicitly use different type

###### _excavator_ is a bot for automating changes across repositories. Changes produced by the roomba/latest-baseline-oss check. # Release Notes ## 4.189.0 | Type | Description | Link | | ---- | ----------- | ---- | | Improvement | Upgrade error_prone to 2.18.0 (from 2.16) | palantir/gradle-baseline#2472 | ## 4.190.0 | Type | Description | Link | | ---- | ----------- | ---- | | Feature | Added `DangerousCollapseKeysUsage` error prone check to disallow usage of `collapseKeys()` API of `EntryStream`. | palantir/gradle-baseline#2291 | | Feature | Prefer common versions of annotations over other copies | palantir/gradle-baseline#2505 | To enable or disable this check, please contact the maintainers of Excavator.

Prefer Guava version of VisibleForTesting annotation

0d442b6

ash211 requested a review from pkoenig10 March 6, 2023 22:27

Add generated changelog entries

dc7485d

pkoenig10 reviewed Mar 6, 2023

View reviewed changes

carterkozak reviewed Mar 7, 2023

View reviewed changes

...error-prone/src/main/java/com/palantir/baseline/errorprone/PreferGuavaVisibleForTesting.java Outdated Show resolved Hide resolved

CR responses

5dfd0c5

ash211 requested review from pkoenig10, carterkozak and schlosna March 7, 2023 21:31

schlosna reviewed Mar 8, 2023

View reviewed changes

...line-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferCommonAnnotations.java Outdated Show resolved Hide resolved

Fix perf nits

be1dd6e

ash211 requested a review from schlosna March 8, 2023 05:09

schlosna approved these changes Mar 8, 2023

View reviewed changes

ash211 added the merge when ready label Mar 8, 2023

bulldozer-bot bot merged commit 11a0a3c into develop Mar 8, 2023

bulldozer-bot bot deleted the aash/prefer-guava-VisibleForTesting branch March 8, 2023 06:29

svc-excavator-bot mentioned this pull request Mar 23, 2023

Excavator: Upgrades Baseline to the latest version palantir/docker-compose-rule#685

Open

rhuffy mentioned this pull request Nov 22, 2024

promote PreferCommonAnnotations to WARNING #2962

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefer Guava version of VisibleForTesting annotation #2505

Prefer Guava version of VisibleForTesting annotation #2505

ash211 commented Mar 6, 2023

changelog-app bot commented Mar 6, 2023 •

edited by ash211

Loading

pkoenig10 Mar 6, 2023

schlosna Mar 7, 2023

ash211 Mar 7, 2023

pkoenig10 Mar 6, 2023

carterkozak Mar 7, 2023

schlosna Mar 7, 2023

pkoenig10 Mar 7, 2023 •

edited

Loading

ash211 Mar 7, 2023

schlosna commented Mar 8, 2023

schlosna left a comment

schlosna Mar 8, 2023

schlosna Mar 8, 2023

	<module name="IllegalImport"> <!-- Java Coding Guidelines: Import the canonical package -->
	<property name="id" value="BanShadedClasses"/>
	<property name="illegalPkgs" value=".*\.(repackaged\|shaded\|thirdparty)"/>
	<property name="regexp" value="true"/>
	<message key="import.illegal" value="Must not import repackaged classes."/>
	</module>

	public final class PreferGuavaVisibleForTesting extends BugChecker implements ImportTreeMatcher {
	public final class PreferErrorProneAnnotations extends BugChecker implements ImportTreeMatcher {

		// NOTE: fully-qualified annotations are not currently re-written
		" @org.jetbrains.annotations.VisibleForTesting",

Prefer Guava version of VisibleForTesting annotation #2505

Prefer Guava version of VisibleForTesting annotation #2505

Conversation

ash211 commented Mar 6, 2023

Before this PR

After this PR

Possible downsides?

changelog-app bot commented Mar 6, 2023 • edited by ash211 Loading

Generate changelog in changelog/@unreleased

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkoenig10 Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schlosna commented Mar 8, 2023

schlosna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changelog-app bot commented Mar 6, 2023 •

edited by ash211

Loading

Generate changelog in `changelog/@unreleased`

pkoenig10 Mar 7, 2023 •

edited

Loading