Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UnusedMethod flags @JsonValue methods as unused #3144

Closed
hisener opened this issue Apr 22, 2022 · 2 comments
Closed

UnusedMethod flags @JsonValue methods as unused #3144

hisener opened this issue Apr 22, 2022 · 2 comments

Comments

@hisener
Copy link
Contributor

hisener commented Apr 22, 2022

We have a couple of private static nested classes and enums with @JsonValue methods, mostly in tests. The UnusedMethod bug pattern flags those as unused, unlike the @JsonCreator methods.

Steps to reproduce

JsonValueExample.java

import java.util.Locale;
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonValue;

public class JsonValueExample {
  private JsonValueExample() {}

  private enum FooBar {
    FOO,
    BAR;

    @JsonValue
    String value() {
      return name().toLowerCase(Locale.ROOT);
    }

    @JsonCreator
    static FooBar of(String value) {
      return valueOf(value.toUpperCase(Locale.ROOT));
    }
  }
}
curl -LO https://repo1.maven.org/maven2/com/google/errorprone/error_prone_core/2.13.1/error_prone_core-2.13.1-with-dependencies.jar
curl -LO https://repo1.maven.org/maven2/com/fasterxml/jackson/core/jackson-annotations/2.13.1/jackson-annotations-2.13.1.jar

javac \
  -J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED \
  -J--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED \
  -J--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED \
  -J--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED \
  -J--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED \
  -J--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED \
  -J--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED \
  -J--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED \
  -J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED \
  -J--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED \
  -cp jackson-annotations-2.13.1.jar \
  -XDcompilePolicy=simple \
  -processorpath error_prone_core-2.13.1-with-dependencies.jar \
  '-Xplugin:ErrorProne -XepDisableAllChecks -Xep:UnusedMethod:ERROR' \
  JsonValueExample.java
JsonValueExample.java:13: error: [UnusedMethod] Method 'value' is never used.
    String value() {
           ^
    (see https://errorprone.info/bugpattern/UnusedMethod)
  Did you mean 'BAR'?
1 error

A potential solution would be adding com.fasterxml.jackson.annotation.JsonValue to exempted annotations similar to JsonCreator.

"com.fasterxml.jackson.annotation.JsonCreator",

Let me know if that makes sense; I can create a PR, as well.

@rickie
Copy link
Contributor

rickie commented Apr 23, 2022

We have the same situation in some of our tests for the following annotations:

  • org.junit.jupiter.params.ParamaterizedTest
  • org.junit.jupiter.api.Test
  • org.junit.jupiter.api.RepeatedTest

For completeness we can also add the following JUnit 5 variants (the JUnit 4 equivalents are already listed):

  • org.junit.jupiter.api.BeforeAll
  • org.junit.jupiter.api.AfterAll
  • org.junit.jupiter.api.AfterEach
  • org.junit.jupiter.api.BeforeEach

And while we are at it, we could also make the JUnit 4 list complete by adding:

  • org.junit.Before
  • org.junit.After
  • org.junit.Test

One last thing to consider is that instead of the org.junit.jupiter.api.Test annotation we could add support for the meta-annotation org.junit.platform.commons.annotation.Testable. That annotation is used on both org.junit.jupiter.api.Test and org.junit.jupiter.api.TestTemplate. TestTemplate is in turn also used in the org.junit.jupiter.api.RepeatedTest annotation.
Supporting this meta annotation requires some more work, but we are also willing to help out.

What do you think of adding these as well?

@retrodaredevil
Copy link

I think there's an endless number of annotations that should be allow a method to be except from UnusedMethod, so I think the better solution would be to allow the configuration of these annotations.

rickie added a commit to PicnicSupermarket/error-prone that referenced this issue Sep 2, 2022
rickie added a commit to PicnicSupermarket/error-prone that referenced this issue Sep 26, 2022
rickie added a commit to PicnicSupermarket/error-prone that referenced this issue Dec 15, 2022
copybara-service bot pushed a commit that referenced this issue Dec 19, 2022
Fixes #3144 & #3297.

Fixes #3428

FUTURE_COPYBARA_INTEGRATE_REVIEW=#3428 from PicnicSupermarket:rossendrijver/unusedmethod_improvements 99f8878
PiperOrigin-RevId: 495640502
benkard pushed a commit to benkard/jgvariant that referenced this issue Jan 14, 2023
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: [#&#8203;1650](google/error-prone#1650), [#&#8203;2706](google/error-prone#2706), [#&#8203;3404](google/error-prone#3404), [#&#8203;3493](google/error-prone#3493), [#&#8203;3504](google/error-prone#3504), [#&#8203;3519](google/error-prone#3519), [#&#8203;3579](google/error-prone#3579), [#&#8203;3610](google/error-prone#3610), [#&#8203;3632](google/error-prone#3632), [#&#8203;3638](google/error-prone#3638), [#&#8203;3645](google/error-prone#3645), [#&#8203;3646](google/error-prone#3646), [#&#8203;3652](google/error-prone#3652), [#&#8203;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: [#&#8203;2321](google/error-prone#2321), [#&#8203;3144](google/error-prone#3144), [#&#8203;3297](google/error-prone#3297), [#&#8203;3428](google/error-prone#3428), [#&#8203;3437](google/error-prone#3437), [#&#8203;3462](google/error-prone#3462), [#&#8203;3482](google/error-prone#3482), [#&#8203;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.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants