By default, prevent `BugChecker`s from introducing new dependencies #308

Stephan202 · 2022-10-23T17:35:50Z

As-is, this is especially relevant for users who do not use Guava or the New Relic Agent API, as mentioned here. Note that most BugCheckers don't need to be modified, since any third-party references in their suggestions must already be present if the check is to trigger at all.

Suggested commit message:

By default, prevent `BugChecker`s from introducing new dependencies (#308)

We should implement something similar for Refaster, but that will look quite different (and is out of scope for this PR).

rickie

Added a commit.

First round of review 👀:smile:.

rickie · 2022-10-24T14:35:20Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java

+   * the given context.
+   *
+   * @param state The context under consideration.
+   * @return {@code true} iff if it is okay to assume or create a dependency on this library.


Suggested change

* @return {@code true} iff if it is okay to assume or create a dependency on this library.

* @return {@code true} iff it is okay to assume or create a dependency on this library.

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java

rickie · 2022-10-24T16:23:07Z

...rone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTrace.java

@@ -48,7 +49,8 @@ public final class ScheduledTransactionTrace extends BugChecker implements Metho

  @Override
  public Description matchMethod(MethodTree tree, VisitorState state) {
-    if (!IS_SCHEDULED.matches(tree, state)) {
+    if (!ThirdPartyLibrary.NEW_RELIC_AGENT_API.canUse(state)


Do we want to add ThirdPartyLibary to StaticImport#STATIC_IMPORT_EXEMPTED_MEMBERS?

No strong opinion. 😄

I see a +1, so will do.

Stephan202

Tnx for the typo fixes :)

Stephan202 · 2022-10-25T17:00:19Z

...rone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTrace.java

@@ -48,7 +49,8 @@ public final class ScheduledTransactionTrace extends BugChecker implements Metho

  @Override
  public Description matchMethod(MethodTree tree, VisitorState state) {
-    if (!IS_SCHEDULED.matches(tree, state)) {
+    if (!ThirdPartyLibrary.NEW_RELIC_AGENT_API.canUse(state)


No strong opinion. 😄

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java

Stephan202 · 2022-10-25T20:33:12Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java

+   * @param state The context under consideration.
+   * @return {@code true} iff it is okay to assume or create a dependency on this library.
+   */
+  public boolean canUse(VisitorState state) {


Note to self: ponder a better name. I didn't select isSupported because it doesn't seem to jive with IGNORE_CLASSPATH_COMPAT_FLAG, but perhaps we should use that name anyway. Or perhaps something else?

I think canUse is fine, especially if we plan to add more fine-grained control.
In this case I think canUse makes more sense than isSupported (or isAvailable) as the library might be available on the classpath, but like the XXX mentions new usages could be undesirable.

Yeah I was also thinking about this.

isAllowedToUse (is allowed might a bit more "accurate" as shouldIgnoreClasspath is part of that check, although it is very similar to canUse)?

I like "allowed". How about isIntroductionAllowed? (Will push, but feel free to challenge.)

Read the Javadocs and goal of the class again.

Have three other options: isUsageAllowed, isReferencingAllowed, isReferenceAllowed.

To me, the ThirdPartyLibrary#isIntroductionAllowed sounds a bit like the question: Is it okay if we introduce this third party library (at all)? The name of the method does not 100% match with the Javadoc of the class, as it mentions "introducing references". So I'd say that isReferencingAllowed could make the whole intent of the class even clearer (and is for that reason the one I like most). WDYT?

We discussed this offline for a while.

@Stephan202 explained his reasoning behind isIntroductionAllowed. I see where you are coming from and it is a good reasoning. He gave the example that one might use Guava but wishes to migrate away from it and therefore using / introducing new usages is not allowed. I'm fine with following the proposed change but, to be honest, I'm not 100% convinced yet and feel isUsingAllowed or isIntroducingUsageAllowed would be the clearest option 🤔. Don't have additional arguments besides the ones already mentioned, sorry.

oxkitsune

Cool stuff! Left some questions and opinions 😄

Would be interesting to see whether we could make a bug checker that enforces ThirdPartyLibrary usage in bug checkers 🤯

oxkitsune · 2022-10-26T07:44:39Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java

+        .errorProneOptions()
+        .getFlags()
+        .getBoolean(IGNORE_CLASSPATH_COMPAT_FLAG)
+        .orElse(Boolean.FALSE);


Suggested change

.orElse(Boolean.FALSE);

.orElse(false);

What's the reason for using Boolean.FALSE over just false 🤔

Because the getBoolean expects a Boolean and otherwise there would be an implicit cast from the boxed false variant.

oxkitsune · 2022-10-26T07:55:22Z

error-prone-contrib/pom.xml

+        <dependency>
+            <groupId>org.springframework</groupId>
+            <artifactId>spring-context</artifactId>
+            <scope>provided</scope>


Suggested change

<scope>provided</scope>

<scope>test</scope>

Considering org.springframework.scheduling.annotation.Scheduled is only explicitly used in ScheduledTransactionTraceTest shouldn't using <scope>test</scope> be enough?

rickie

Some final notes, PR looks nice!

rickie · 2022-10-26T07:03:29Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java

+  @SuppressWarnings("ImmutableEnumChecker" /* Supplier is deterministic. */)
+  private final Supplier<Boolean> canUse;
+
+  ThirdPartyLibrary(String witnessFqcn) {


IMO it would be nice to add some Javadoc here. I understand what witnessFqcn is in this context, but might be nice to add some explanation here as well.

rickie · 2022-10-26T08:01:10Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java

+   * @param state The context under consideration.
+   * @return {@code true} iff it is okay to assume or create a dependency on this library.
+   */
+  public boolean canUse(VisitorState state) {


Yeah I was also thinking about this.

isAllowedToUse (is allowed might a bit more "accurate" as shouldIgnoreClasspath is part of that check, although it is very similar to canUse)?

rickie · 2022-10-26T08:04:20Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java

+        .errorProneOptions()
+        .getFlags()
+        .getBoolean(IGNORE_CLASSPATH_COMPAT_FLAG)
+        .orElse(Boolean.FALSE);


Because the getBoolean expects a Boolean and otherwise there would be an implicit cast from the boxed false variant.

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java

Stephan202 · 2022-10-26T09:50:33Z

Thanks for the review and fix @oxkitsune and @rickie! I'll circle back to this PR ~later today (many meetings; let's see :D).

Stephan202

Rebased and added a commit. We could also move the StaticImport changes and investigation to a separate PR. (If we do want to keep it here, then eventually the suggested commit message should be updated, too.)

Stephan202 · 2022-10-26T09:59:43Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java

+   * @param state The context under consideration.
+   * @return {@code true} iff it is okay to assume or create a dependency on this library.
+   */
+  public boolean canUse(VisitorState state) {


I like "allowed". How about isIntroductionAllowed? (Will push, but feel free to challenge.)

Stephan202 · 2022-10-26T10:10:22Z

...rone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ScheduledTransactionTrace.java

@@ -48,7 +49,8 @@ public final class ScheduledTransactionTrace extends BugChecker implements Metho

  @Override
  public Description matchMethod(MethodTree tree, VisitorState state) {
-    if (!IS_SCHEDULED.matches(tree, state)) {
+    if (!ThirdPartyLibrary.NEW_RELIC_AGENT_API.canUse(state)


I see a +1, so will do.

Stephan202 · 2022-10-26T10:14:28Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java

@@ -102,7 +102,8 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa
          "org.springframework.http.MediaType",
          "org.testng.Assert",
          "reactor.function.TupleUtils",
-          "tech.picnic.errorprone.bugpatterns.util.MoreTypes");
+          "tech.picnic.errorprone.bugpatterns.util.MoreTypes",
+          "tech.picnic.errorprone.bugpatterns.util.ThirdPartyLibrary");


This doesn't cause the enum values to be statically imported. Need to investigate before merging this PR.

Stephan202 · 2022-10-26T10:14:52Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java

@@ -189,7 +190,8 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa
          "INSTANCE",
          "newBuilder",
          "of",
-          "valueOf");
+          "valueOf",
+          "values");


... but ThirdPartyLibrary.values(), would be statically imported, which is not something we want.

This should also be reverted.

rickie · 2022-10-26T10:35:52Z

Note that I mention in this comment that I want to add it to the STATIC_IMPORT_EXEMPTED_MEMBERS because I don't like statically importing this one. Primarily as it would mean loosing some context.

... but ThirdPartyLibrary.values(), would be statically imported, which is not something we want.

This is not true right? It would be prevented in the isCandidate method because it sees that values is in STATIC_IMPORT_EXEMPTED_IDENTIFIERS.

So I want to propose that I add a commit to move it. Which means that we defer looking into the issue with not importing 🤔. WDYT?

rickie · 2022-10-26T10:39:19Z

I see that we currently only allow to exempt specific members using STATIC_IMPORT_EXEMPTED_MEMBERS, which we could do as there are only 4 types in the enum. Nevertheless, let's defer it all together and investigate the issue + introduce support for easily adding a class to disallow all members thereof.

Stephan202

Rebased and added a commit. As discussed offline: will merge once built.

Note that I mention in this comment that I want to add it to the STATIC_IMPORT_EXEMPTED_MEMBERS because I don't like statically importing this one. Primarily as it would mean loosing some context.

Okay, I misread that (but find it rather surprising; I don't see any issue with statically importing these, as long as it'd be done consistently 🤷).

... but ThirdPartyLibrary.values(), would be statically imported, which is not something we want.

This is not true right? It would be prevented in the isCandidate method because it sees that values is in STATIC_IMPORT_EXEMPTED_IDENTIFIERS.

Yes, because I added values in the context of this PR after making this observation...

Stephan202 · 2022-10-29T11:24:59Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java

@@ -189,7 +190,8 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa
          "INSTANCE",
          "newBuilder",
          "of",
-          "valueOf");
+          "valueOf",
+          "values");


This should also be reverted.

rickie · 2022-10-29T15:57:25Z

I don't see any issue with statically importing these

I think it is a bit less clear 🤷🏻.

Yes, because I added values in the context of this PR after making this observation...

Ahh I missed that change 😂, that's why I didn't revert it haha. Thanks for fixing that 😉.

Stephan202 added this to the 0.5.0 milestone Oct 23, 2022

Stephan202 requested a review from rickie October 23, 2022 17:35

rickie mentioned this pull request Oct 25, 2022

False positive against Sentry-Trace #315

Closed

2 tasks

rickie linked an issue Oct 25, 2022 that may be closed by this pull request

False positive against Sentry-Trace #315

Closed

2 tasks

rickie added the improvement label Oct 25, 2022

rickie reviewed Oct 25, 2022

View reviewed changes

Stephan202 mentioned this pull request Oct 25, 2022

Make it easy to disable Immutable***Rules #313

Closed

Stephan202 commented Oct 25, 2022

View reviewed changes

oxkitsune self-requested a review October 26, 2022 07:28

oxkitsune approved these changes Oct 26, 2022

View reviewed changes

rickie approved these changes Oct 26, 2022

View reviewed changes

Stephan202 force-pushed the sschroevers/better-classpath-awareness branch from f64bf4d to 13c862b Compare October 26, 2022 10:13

Stephan202 commented Oct 26, 2022

View reviewed changes

Stephan202 and others added 6 commits October 29, 2022 13:27

By default, prevent BugCheckers from introducing new dependencies

973a791

Suggestions

b7bcd28

Suggestion Gijs

d9b8b5e

Apply suggestions

dfeb574

Drop ThirdPartyLibrary from StaticImport candidate list

3415f92

Further revert StaticImport changes

fec8d52

Stephan202 force-pushed the sschroevers/better-classpath-awareness branch from b8fd499 to fec8d52 Compare October 29, 2022 11:29

Stephan202 commented Oct 29, 2022

View reviewed changes

Stephan202 merged commit 8fa3ff3 into master Oct 29, 2022

Stephan202 deleted the sschroevers/better-classpath-awareness branch October 29, 2022 11:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

By default, prevent `BugChecker`s from introducing new dependencies #308

By default, prevent `BugChecker`s from introducing new dependencies #308

Stephan202 commented Oct 23, 2022 •

edited

Loading

rickie left a comment

rickie Oct 24, 2022

rickie Oct 24, 2022

Stephan202 Oct 25, 2022

Stephan202 Oct 26, 2022

Stephan202 left a comment

Stephan202 Oct 25, 2022

Stephan202 Oct 25, 2022

oxkitsune Oct 26, 2022

rickie Oct 26, 2022

Stephan202 Oct 26, 2022

rickie Oct 26, 2022

rickie Oct 26, 2022 •

edited

Loading

oxkitsune left a comment

oxkitsune Oct 26, 2022

rickie Oct 26, 2022

oxkitsune Oct 26, 2022

rickie left a comment

rickie Oct 26, 2022

rickie Oct 26, 2022

rickie Oct 26, 2022

Stephan202 commented Oct 26, 2022

Stephan202 left a comment

Stephan202 Oct 26, 2022

Stephan202 Oct 26, 2022

Stephan202 Oct 26, 2022

Stephan202 Oct 26, 2022

Stephan202 Oct 29, 2022

rickie commented Oct 26, 2022 •

edited

Loading

rickie commented Oct 26, 2022

Stephan202 left a comment

Stephan202 Oct 29, 2022

rickie commented Oct 29, 2022

	* @return {@code true} iff if it is okay to assume or create a dependency on this library.
	* @return {@code true} iff it is okay to assume or create a dependency on this library.

By default, prevent BugCheckers from introducing new dependencies #308

By default, prevent BugCheckers from introducing new dependencies #308

Conversation

Stephan202 commented Oct 23, 2022 • edited Loading

rickie left a 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

Stephan202 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

oxkitsune left a 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

rickie left a 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

Stephan202 commented Oct 26, 2022

Stephan202 left a 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

Choose a reason for hiding this comment

rickie commented Oct 26, 2022 • edited Loading

rickie commented Oct 26, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Oct 29, 2022

By default, prevent `BugChecker`s from introducing new dependencies #308

By default, prevent `BugChecker`s from introducing new dependencies #308

Stephan202 commented Oct 23, 2022 •

edited

Loading

rickie Oct 26, 2022 •

edited

Loading

rickie commented Oct 26, 2022 •

edited

Loading