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

Introduce MissingRefasterAnnotation checker #2232

Conversation

rickie
Copy link
Contributor

@rickie rickie commented Mar 11, 2021

See the MissingRefasterAnnotation.md for more details.

Feedback is welcome!

@google-cla google-cla bot added the cla: yes label Mar 11, 2021
.distinct()
.count();

return methodTypes < 2 ? Description.NO_MATCH : buildDescription(tree).build();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why only match if there are at least two methods with refaster annotations? I know we'd usually expect to see at least two (e.g. both a @BeforeTemplate and @AfterTemplate) but would only matching on one result in false positives?

Maybe there are cases where there's an abstract class that's used to share a placeholder or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to partition the methods into two groups: those with a Refaster annotation and those without.
The checker should only flag classes that have methods of both types.
So far, we have not seen a scenario in which a Refaster template class contains an unannotated method.
If those do exist, can you share a concrete example, perhaps from Google's vast internal collection of Refaster templates?


@Override
public Description matchClass(ClassTree tree, VisitorState state) {
long methodTypes =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd consider implementing this is a MethodTreeMatcher instead of a ClassTreeMatcher that scans its members for methods, but that depends on whether the check for < 2 methods below is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps a better name would be methodTypeCount.
Since we want to know about all the methods in the class, we use a ClassTreeMatcher.

Copy link
Collaborator

@cushon cushon left a comment

Choose a reason for hiding this comment

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

Thanks for the check!

@rickie rickie requested a review from cushon August 28, 2021 07:01
@rickie rickie force-pushed the rossendrijver/bugpattern_refaster_annotations branch from 0976cd2 to 965fb2f Compare November 23, 2021 19:25
@rickie
Copy link
Contributor Author

rickie commented Nov 23, 2021

Rebased and added a commit. What do you think of the check?

@rickie rickie force-pushed the rossendrijver/bugpattern_refaster_annotations branch from 965fb2f to 3e0411f Compare April 15, 2022 09:03
@rickie
Copy link
Contributor Author

rickie commented Apr 15, 2022

Rebased and added a small commit. Still think this check can be useful :).

@rickie rickie force-pushed the rossendrijver/bugpattern_refaster_annotations branch from 3e0411f to d6a7be7 Compare April 15, 2022 09:16
@rickie rickie force-pushed the rossendrijver/bugpattern_refaster_annotations branch from d6a7be7 to 981dcbe Compare June 8, 2022 09:14
@rickie
Copy link
Contributor Author

rickie commented Jun 8, 2022

Rebased.

@rickie rickie force-pushed the rossendrijver/bugpattern_refaster_annotations branch from 981dcbe to ce982d8 Compare July 4, 2022 12:17
@rickie
Copy link
Contributor Author

rickie commented Jul 4, 2022

Rebased.

@rickie rickie force-pushed the rossendrijver/bugpattern_refaster_annotations branch from ce982d8 to 4eb1c41 Compare August 8, 2022 07:06
@rickie rickie force-pushed the rossendrijver/bugpattern_refaster_annotations branch from 4eb1c41 to 211966e Compare September 2, 2022 08:30
copybara-service bot pushed a commit that referenced this pull request Jun 7, 2023
See the `MissingRefasterAnnotation.md` for more details.

Feedback is welcome!

Fixes #2232

FUTURE_COPYBARA_INTEGRATE_REVIEW=#2232 from PicnicSupermarket:rossendrijver/bugpattern_refaster_annotations 211966e
PiperOrigin-RevId: 538605421
copybara-service bot pushed a commit that referenced this pull request Jun 7, 2023
See the `MissingRefasterAnnotation.md` for more details.

Feedback is welcome!

Fixes #2232

FUTURE_COPYBARA_INTEGRATE_REVIEW=#2232 from PicnicSupermarket:rossendrijver/bugpattern_refaster_annotations 211966e
PiperOrigin-RevId: 538605421
@copybara-service copybara-service bot closed this in 0604d38 Jun 7, 2023
benkard pushed a commit to benkard/jgvariant that referenced this pull request Jun 18, 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.19.1` -> `2.20.0` |
| [com.google.errorprone:error_prone_annotations](https://errorprone.info) ([source](https://github.com/google/error-prone)) | compile | minor | `2.19.1` -> `2.20.0` |

---

### Release Notes

<details>
<summary>google/error-prone</summary>

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

[Compare Source](google/error-prone@v2.19.1...v2.20.0)

Changes:

-   This release is compatible with early-access builds of JDK 21.

New Checkers:

-   [`InlineTrivialConstant`](https://errorprone.info/bugpattern/InlineTrivialConstant)
-   [`UnnecessaryStringBuilder`](https://errorprone.info/bugpattern/UnnecessaryStringBuilder)
-   [`BanClassLoader`](https://errorprone.info/bugpattern/BanClassLoader)
-   [`DereferenceWithNullBranch`](https://errorprone.info/bugpattern/DereferenceWithNullBranch)
-   [`DoNotUseRuleChain`](https://errorprone.info/bugpattern/DoNotUseRuleChain)
-   [`LockOnNonEnclosingClassLiteral`](https://errorprone.info/bugpattern/LockOnNonEnclosingClassLiteral)
-   [`MissingRefasterAnnotation`](https://errorprone.info/bugpattern/MissingRefasterAnnotation)
-   [`NamedLikeContextualKeyword`](https://errorprone.info/bugpattern/NamedLikeContextualKeyword)
-   [`NonApiType`](https://errorprone.info/bugpattern/NonApiType)

Fixes issues: [#&#8203;2232](google/error-prone#2232), [#&#8203;2243](google/error-prone#2243), [#&#8203;2997](google/error-prone#2997), [#&#8203;3301](google/error-prone#3301), [#&#8203;3843](google/error-prone#3843), [#&#8203;3903](google/error-prone#3903), [#&#8203;3918](google/error-prone#3918), [#&#8203;3923](google/error-prone#3923), [#&#8203;3931](google/error-prone#3931), [#&#8203;3945](google/error-prone#3945), [#&#8203;3946](google/error-prone#3946)

**Full Changelog**: google/error-prone@v2.19.1...v2.20.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.

🔕 **Ignore**: Close this MR and you won't be reminded about these updates again.

---

 - [ ] <!-- 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-->
@Stephan202 Stephan202 deleted the rossendrijver/bugpattern_refaster_annotations branch August 6, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants