-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Issue #14084: fix Checker violation for nonnegative integer argument #14673
base: master
Are you sure you want to change the base?
Issue #14084: fix Checker violation for nonnegative integer argument #14673
Conversation
3301fe0
to
7fa2268
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
items:
@@ -19,6 +19,8 @@ | |||
|
|||
package com.puppycrawl.tools.checkstyle.api; | |||
|
|||
import org.checkerframework.checker.index.qual.NonNegative; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is apart I am not sure about Checker: we looks like have to change our api and force all our user to depend on Checker, that is not good.
We recently resolved big pain of dependency to guava, that is not api based dependency.
Do we have any other way resolve this ?
if not I would rather disable validation of Checker for cases that require NonNegative
.
Can we remove it from api
class only ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@romani ,
Sorry to involve.. But why are we not using jsr-305 annotations here ?
I think we have a Nonnegative annotation in javax.
https://www.javadoc.io/doc/com.google.code.findbugs/jsr305/latest/javax/annotation/Nonnegative.html
@Lmh-java , could you please explain why you have introduced checker's annotations here?
I understand why there are no ci troubles here. Checker framework treats annotations from other tools as if they were extracted from corresponding annotation in Checker itself.
But by doing this, we will only be increasing our JAR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems checkerframework does not support Nonnegative
from javax. I was trying to use it first until I figured out it does not recognize that annotation. Therefore, I turned to use NonNegative
from the checker framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, looks that way , I was just browsing through checker release docs : https://checkerframework.org/releases/2.3.1/api/index.html?javax/annotation/package-summary.html
Do we have any other way resolve this ?
@romani , I'm afraid we don't have any alternative to migrate jsr 305 other than checker framework.
Most of the projects are removing their javax annotations rather than using checker imports.
Jenkins for example : https://groups.google.com/g/jenkinsci-dev/c/uE1wwtVi1W0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, same here. I've just looked at the same Jenkin's discussion posts. But, I will try to solve this problem by throwing an UncheckException
. Not very sure if that works, but I will test it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems checkerframework does not support Nonnegative from javax.
Can we check if there is issue registed for it on checker ? If not, let's create.
If there is defect in checker, I would rather postpone dealing with such violations. There are tons of others, no need to be stuck with problematic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Nonnegative annotation is just one of many..
These are all the annotations in package javax.annotations (jsr 305) :
https://www.javadoc.io/doc/com.google.code.findbugs/jsr305/latest/javax/annotation/package-summary.html
And checker is only supporting three of them :
https://checkerframework.org/releases/2.3.1/api/index.html?javax/annotation/package-summary.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And checker is only supporting three of them
Yea, I have seen that as well.
Can we check if there is issue registed for it on checker ? If not, let's create.
I have opened an issue to their repo already and I am waiting for their response. Meanwhile, I will be dealing with other violations. We can postpone this violation for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can solve this problem by introducing a throw of IllegalArgumentException
as an unchecked exception. But I still feel it's not elegant and not necessary,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Items
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Items
<fileName>src/main/java/com/puppycrawl/tools/checkstyle/DetailAstImpl.java</fileName> | ||
<specifier>argument</specifier> | ||
<message>incompatible argument for parameter bitIndex of BitSet.get.</message> | ||
<lineContent>return getBranchTokenTypes().get(tokenType);</lineContent> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is details of implementation that we use bitset. But we are changing API method. It is not good.
API is generic, and if somebody wants to put -1 in our method, they have full right in this, we should not throw exception, and just return false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I use 1
as the boundary since the smallest flag value of tokenType
is 1
, and also to pass Pitest mutation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API is generic, and if somebody wants to put -1 in our method, they have full right in this, we should not throw exception, and just return false.
But we are changing API method. It is not good.
@romani we are still changing the behavior of our API for this update (from IndexOutOfBoundsException
-> silent fail). Can we just annotate the method's parameter to specify that it should be nonnegative? Dancing around the dependency on Checker at compile time is getting old; if we are going to use this tool, let's use it correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid to use checker as compile time dependency for our API .
For now we just remove Checker violation, by workaround. No change in behavior, or may be minor change to better, less exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid to use checker as compile time dependency for our API .
Then I don’t see any point in using it at all. Changing a part of our API to silently fail to be able to remove some suppression from a file is not an improvement to the project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should absolutely throw an exception here. If we cannot come to an agreement on this particular checker (index) then we should avoid sending PRs for this one until we agree on how we are handling it. Changing production code in our core API to silently fail is not a positive change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rnveach , please share your opinion here on exception or just return of false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I question the whole point of the new code check. We are reliant on ANTLR generating these numbers. I am not sure we have any contract with them which they should not be negative. Even if they did specify it somewhere in some document, I would think this is something we shouldn't enforce since its such low level and behinds the scene. As long as the method doesn't have a problem with positive/zero/negative, then I don't think we should check this.
I am not for an exception and not for this new condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would not be a concern if we have tokens as enum , so it will be type safe. But we have int
so user can put anything to such methods that is comply by type, for example index of some loop in custom Check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as the method doesn't have a problem with positive/zero/negative
It does. It throws exception. After fix it will return false. I like it more as "search" should not throw exceptions and just return true/false.
7fa2268
to
3729e24
Compare
3729e24
to
7daa526
Compare
@romani, the checker framework may add the |
They had majorly introduced the nullness annotations in release 2.1.14. Unfortunately, this is outdated by 8 solid years :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok to merge
@@ -384,7 +384,7 @@ private BitSet getBranchTokenTypes() { | |||
|
|||
@Override | |||
public boolean branchContains(int tokenType) { | |||
return getBranchTokenTypes().get(tokenType); | |||
return tokenType >= 1 && getBranchTokenTypes().get(tokenType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is good.
✔ ~/java/github/checkstyle/checkstyle
$ head -n 25 ./target/generated-sources/antlr/com/puppycrawl/tools/checkstyle/grammar/java/JavaLanguageLexer.java
// Generated from com/puppycrawl/tools/checkstyle/grammar/java/JavaLanguageLexer.g4 by ANTLR 4.13.1
package com.puppycrawl.tools.checkstyle.grammar.java;
import com.puppycrawl.tools.checkstyle.grammar.CommentListener;
import com.puppycrawl.tools.checkstyle.grammar.CompositeLexerContextCache;
import com.puppycrawl.tools.checkstyle.grammar.CrAwareLexerSimulator;
import org.antlr.v4.runtime.Lexer;
import org.antlr.v4.runtime.CharStream;
import org.antlr.v4.runtime.Token;
import org.antlr.v4.runtime.TokenStream;
import org.antlr.v4.runtime.*;
import org.antlr.v4.runtime.atn.*;
import org.antlr.v4.runtime.dfa.DFA;
import org.antlr.v4.runtime.misc.*;
@SuppressWarnings({"all", "warnings", "unchecked", "unused", "cast", "CheckReturnValue", "this-escape"})
public class JavaLanguageLexer extends Lexer {
static { RuntimeMetaData.checkVersion("4.13.1", RuntimeMetaData.VERSION); }
protected static final DFA[] _decisionToDFA;
protected static final PredictionContextCache _sharedContextCache =
new PredictionContextCache();
public static final int
COMPILATION_UNIT=1, PLACEHOLDER1=2, NULL_TREE_LOOKAHEAD=3, BLOCK=4, MODIFIERS=5,
I propose that we gather all annotations that we will require to complete index checker integration and share this with checker maintainers. Once they support this, we can resume integration with index checker. |
Sure, one day we will do summary. But if we pause, we will never find next problems for us. Our problem is that part of our code is API. But as slowly start thinking of Checker, on single and simple case we will eventually find more items that we can not do, and we will have better summary. Problem here is that we have API |
@rnveach , please share your opinion. |
I agree with this statement, we should create an issue to discuss not using bitset.
I am not proposing we pause on all checker, just ones that force us to change production code to dance around depending on checker at compile time |
Which am I sharing on? |
It is ok to use it, such a performance optimization is far future, and I am not sure about it. For now all we say is, if use do |
@rnveach , here is a discussion #14673 (comment) here is my answer - #14673 (comment) |
Here is my answer - #14673 (comment) Let me know when we have decided on a group decision. |
Part of Issue #14084
Solve suppression for
DetailAstImpl
by introducingNonNegative
annotation.