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

Add support for jakarta annotations to some checks #4782

Closed
wants to merge 3 commits into from

Conversation

gavlyukovskiy
Copy link
Contributor

This MR adds jakarta annotations to the exempt lists.
We're currently migrating to Jakarta EE and we have to temporarily disable the rule or add exceptions in the code.

I didn't add new tests since it's just additional exempt annotations, but let me know if you want explicit tests.

@@ -168,6 +173,7 @@ public final class UnusedVariable extends BugChecker implements CompilationUnitT
"com.google.inject.multibindings.ProvidesIntoMap",
"com.google.inject.multibindings.ProvidesIntoSet",
"dagger.Provides",
"jakarta.inject.Inject",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also add this to UnnecessarilyVisible? (as hinted in the TODO comment above the constant)

Copy link
Contributor Author

@gavlyukovskiy gavlyukovskiy Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added it there as well as to few other places I found. Thanks!
I've seen it also in InjectMatchers and other places that seem to relate to dependency injection / Guice. I didn't change that, because the logic is a bit more complicated, and since we're not using Guice, I'm not that confident in changing those. I'm not sure whether it even supports Jakarta EE already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we're not using Guice, I'm not that confident in changing those. I'm not sure whether it even supports Jakarta EE already.

FYI, it does!
https://github.com/google/guice/wiki/Guice600
https://github.com/google/guice/wiki/Guice700

Copy link
Contributor Author

@gavlyukovskiy gavlyukovskiy Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tbroyer good to know :) But do you think we can have these checks merged separately from Guice or are they somehow intertwined?
I also noticed that sometimes there are suggested fixes to add javax.inject.Inject annotation, but it doesn't make sense if the project is already using jakarta / Guice 7 🤷 I think there could be some heuristic, but I'm not the best person to define that as we're using spring instead 😅

@gavlyukovskiy gavlyukovskiy changed the title Add jakarta annotations to UnusedVariable Add support for jakarta annotations to some checks Jan 20, 2025
@gavlyukovskiy
Copy link
Contributor Author

Hi @tbroyer, did you have a chance to take a look at this PR? I've updated all the places that do not touch guice, let me know if you need anything else from me.

@tbroyer
Copy link
Contributor

tbroyer commented Feb 17, 2025

Ha, I'm just a contributor like you here, not a project member 🤷‍♂️

Anyway, I think (in that I can't see a reason why not) anywhere a javax has a jakarta equivalent, then that one should be added. UnusedMethod seems to have a lot more of javax.persistence, javax.annotation, and javax.validation (as well as a javax.inject), but I would have no problem improving things step by step (also, dependency injection related checks look quite convoluted and I wouldn't want to touch them myself either 😅 so fully understand not modifying them here)

@gavlyukovskiy
Copy link
Contributor Author

gavlyukovskiy commented Feb 18, 2025

@tbroyer thanks for taking a look :) UnusedMethod already has jakarta equivalents for everything, but I found two that are missing - jakarta.validation.constraints.AssertFalse and jakarta.validation.constraints.AssertTrue so I've added those as well.
You definitely have more experience contributing to this project, so would you suggest reaching out somewhere or just wait until a maintainer takes a look? In our project we are already using patched UnusedVaraible, so for me it's not urgent to be merged, but it would be nice to have this fix upstream in general.

…ssarilyVisible, UnnecessaryAssignment
@gavlyukovskiy
Copy link
Contributor Author

@cushon thank you for reviewing this PR, I've just rebased it on the latest master to resolve some conflicts.

copybara-service bot pushed a commit that referenced this pull request Feb 27, 2025
This MR adds jakarta annotations to the exempt lists.
We're currently migrating to Jakarta EE and we have to temporarily disable the rule or add exceptions in the code.

I didn't add new tests since it's just additional exempt annotations, but let me know if you want explicit tests.

Fixes #4782

FUTURE_COPYBARA_INTEGRATE_REVIEW=#4782 from gavlyukovskiy:patch-1 398cd4a
PiperOrigin-RevId: 731687227
@gavlyukovskiy gavlyukovskiy deleted the patch-1 branch February 27, 2025 16:53
svc-squareup-copybara pushed a commit to cashapp/misk that referenced this pull request Mar 21, 2025
| Package | Type | Package file | Manager | Update | Change |
|---|---|---|---|---|---|
| [com.google.guava:guava-bom](https://github.com/google/guava)
([source](http://svn.sonatype.org/spice/trunk/oss/oss-parent-9)) |
dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`33.4.0-jre` -> `33.4.5-jre` |
|
[com.google.apis:google-api-services-storage](http://nexus.sonatype.org/oss-repository-hosting.html)
([source](http://svn.sonatype.org/spice/tags/oss-parent-7)) |
dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`v1-rev20250224-2.0.0` -> `v1-rev20250312-2.0.0` |
|
[com.google.errorprone:error_prone_annotations](https://errorprone.info)
([source](https://github.com/google/error-prone)) | dependencies |
misk/gradle/libs.versions.toml | gradle | minor | `2.36.0` -> `2.37.0` |
|
[com.autonomousapps.dependency-analysis](https://github.com/autonomousapps/dependency-analysis-android-gradle-plugin)
| plugin | misk/gradle/libs.versions.toml | gradle | minor | `2.12.0` ->
`2.13.0` |
| [software.amazon.awssdk:sdk-core](https://aws.amazon.com/sdkforjava) |
dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`2.31.3` -> `2.31.4` |
| [software.amazon.awssdk:sqs](https://aws.amazon.com/sdkforjava) |
dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`2.31.3` -> `2.31.4` |
|
[software.amazon.awssdk:dynamodb-enhanced](https://aws.amazon.com/sdkforjava)
| dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`2.31.3` -> `2.31.4` |
| [software.amazon.awssdk:dynamodb](https://aws.amazon.com/sdkforjava) |
dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`2.31.3` -> `2.31.4` |
| [software.amazon.awssdk:aws-core](https://aws.amazon.com/sdkforjava) |
dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`2.31.3` -> `2.31.4` |
| [software.amazon.awssdk:bom](https://aws.amazon.com/sdkforjava) |
dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`2.31.3` -> `2.31.4` |
| [software.amazon.awssdk:auth](https://aws.amazon.com/sdkforjava) |
dependencies | misk/gradle/libs.versions.toml | gradle | patch |
`2.31.3` -> `2.31.4` |

---

### Release Notes

<details>
<summary>google/error-prone
(com.google.errorprone:error_prone_annotations)</summary>

###
[`v2.37.0`](https://github.com/google/error-prone/releases/tag/v2.37.0):
Error Prone 2.37.0

Changes:

- The annotations that were previously in `error_prone_type_annotations`
have been been merged into `error_prone_annotations`.
`error_prone_type_annotations` is now deprecated, and will be removed in
a future release.

New checks:

-
[`AssignmentExpression`](https://errorprone.info/bugpattern/AssignmentExpression)
- The use of an assignment expression can be surprising and hard to
read; consider factoring out the assignment to a separate statement.
-
[`IntFloatConversion`](https://errorprone.info/bugpattern/IntFloatConversion)
- Detect calls to `scalb` that should be using the double overload
instead
- [`InvalidSnippet`](https://errorprone.info/bugpattern/InvalidSnippet)
- Detects snippets which omit the `:` required for inline code.
-
[`JUnit4EmptyMethods`](https://errorprone.info/bugpattern/JUnit4EmptyMethods)
- Detects empty JUnit4 `@Before`, `@After`, `@BeforeClass`, and
`@AfterClass` methods.
-
[`MockIllegalThrows`](https://errorprone.info/bugpattern/MockIllegalThrows)
- Detects cases where Mockito is configured to throw checked exception
types which are impossible.
-
[`NegativeBoolean`](https://errorprone.info/bugpattern/NegativeBoolean)
- Prefer positive boolean names.
- [`RuleNotRun`](https://errorprone.info/bugpattern/RuleNotRun) -
Detects `TestRule`s not annotated with `@Rule`, that won't be run.
-
[`StringConcatToTextBlock`](https://errorprone.info/bugpattern/StringConcatToTextBlock)
- Replaces concatenated multiline strings with text blocks.
-
[`TimeInStaticInitializer`](https://errorprone.info/bugpattern/TimeInStaticInitializer)
- Detects accesses of the system time in static contexts.

Closed issues:

- Propagate check flags in patch mode
([#&#8203;4699](google/error-prone#4699))
- Fixes a crash in ComputeIfAbsentAmbiguousReference
([#&#8203;4736](google/error-prone#4736))
- Show the field name in HidingField diagnostics
([#&#8203;4775](google/error-prone#4775))
- Add support for jakarta annotations to some checks
([#&#8203;4782](google/error-prone#4782))
- FloatingPointAssertionWithinEpsilonTest depends on default locale
([#&#8203;4815](google/error-prone#4815))
- `@InlineMe` patching of `Strings.repeat` produces broken code
([#&#8203;4819](google/error-prone#4819))
- Fix a crash in IdentifierName on unnamed (`_`) variables
([#&#8203;4847](google/error-prone#4847))
- Fix a crash in ArgumentParameterSwap
([#&#8203;490](google/error-prone#490))

Full changelog:
google/error-prone@v2.36.0...v2.37.0

</details>

<details>
<summary>autonomousapps/dependency-analysis-android-gradle-plugin
(com.autonomousapps.dependency-analysis)</summary>

###
[`v2.13.0`](https://github.com/autonomousapps/dependency-analysis-android-gradle-plugin/blob/HEAD/CHANGELOG.md#Version-2130)

- \[Feat]: `computeResolvedDependencies` to also generate a version
catalog file
- \[Feat]: experimenting with compressing intermediates, starting with
`exploded-jars.json`.
-   \[Chore]: remove unused moshi functions.

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 6pm every weekday,before 2am
every weekday" in timezone Australia/Melbourne, Automerge - At any time
(no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Never, or you tick the rebase/retry checkbox.

👻 **Immortal**: This PR 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 PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://github.com/renovatebot/renovate).

GitOrigin-RevId: 9a8acb12117486aa61b1d54c73d77d84629e5e79
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants