Skip to content

Remove redundant casts #18128

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

Merged

Conversation

findepi
Copy link
Member

@findepi findepi commented Jul 4, 2023

defaultRewrite is generic, returns same type as provided.

findepi added 3 commits July 4, 2023 16:34
`defaultRewrite` is generic, returns same type as provided.
Checkstyle does not detect it because it mis-interprets switch case
labels.
@findepi findepi added the no-release-notes This pull request does not require release notes entry label Jul 4, 2023
@findepi findepi requested a review from ebyhr July 4, 2023 14:49
@cla-bot cla-bot bot added the cla-signed label Jul 4, 2023
@@ -79,7 +79,6 @@
import static io.trino.sql.tree.ComparisonExpression.Operator.EQUAL;
import static io.trino.sql.tree.ComparisonExpression.Operator.GREATER_THAN;
import static io.trino.sql.tree.ComparisonExpression.Operator.GREATER_THAN_OR_EQUAL;
import static io.trino.sql.tree.ComparisonExpression.Operator.IS_DISTINCT_FROM;
Copy link
Member Author

Choose a reason for hiding this comment

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

error-prone's RemoveUnusedImports doesn't seem to catch this either

cc @ksobolew

Copy link
Contributor

Choose a reason for hiding this comment

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

I reported a similar bug in Checkstyle: checkstyle/checkstyle#12923. It concerns statically-imported methods, but maybe it also misses constants as well when they are used as targets in switch.

Copy link
Member Author

Choose a reason for hiding this comment

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

it looks like error-prone would be in a better position to correctly analyze unused imports, because it also understands class inheritance. would you want to try to report this problem with RemoveUnusedImports to error-prone?

Copy link
Contributor

Choose a reason for hiding this comment

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

First, I reported it to Checkstyle: checkstyle/checkstyle#13361, because there is a very similar issue reported there already (checkstyle/checkstyle#12923).

Copy link
Contributor

Choose a reason for hiding this comment

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

And here's a mostly copy-pasted one in Error Prone: google/error-prone#4000 (Error Prone 4000!)

Copy link
Contributor

Choose a reason for hiding this comment

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

And I have a weird feeling of deja vu, as if I already reported something like this somewhere, but I can't find any previous issues...

Copy link
Member Author

Choose a reason for hiding this comment

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

thank you @ksobolew !

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: The checkstyle issue was closed because Checkstyle does not have the necessary type information to reliably detect this. This leaves us with ErrorProne (but no movement on that front).

@findepi findepi merged commit 1c8c83d into trinodb:master Jul 5, 2023
@findepi findepi deleted the findepi/remove-redundant-casts-00f58d branch July 5, 2023 05:37
@github-actions github-actions bot added this to the 421 milestone Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

4 participants