Issue #14084: fix Checker violation for nonnegative integer argument #14673

Lmh-java · 2024-03-17T06:47:49Z

Part of Issue #14084
Solve suppression for DetailAstImpl by introducing NonNegative annotation.

romani

items:

romani · 2024-03-17T14:54:08Z

src/main/java/com/puppycrawl/tools/checkstyle/api/DetailAST.java

@@ -19,6 +19,8 @@

 package com.puppycrawl.tools.checkstyle.api;

+import org.checkerframework.checker.index.qual.NonNegative;


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 ?

@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.

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.

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

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.

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.

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

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.

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,

romani

Items

romani

Items

romani · 2024-03-17T23:24:31Z

config/checker-framework-suppressions/checker-index-suppressions.xml

-    <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>


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.

Done. I use 1 as the boundary since the smallest flag value of tokenType is 1, and also to pass Pitest mutation.

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.

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.

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.

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.

@rnveach , please share your opinion here on exception or just return of false

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.

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.

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.

… argument

Lmh-java · 2024-03-18T16:11:53Z

@romani, the checker framework may add the @Nonnegative from javax as an alias to their annotation in the future update. I will keep track of that so we can bump the version of the checker framework in the future to better solve other suppressed violations.
The issue I opened: typetools/checker-framework#6507

MANISH-K-07 · 2024-03-18T16:21:59Z

@romani, the checker framework may add the @Nonnegative from javax as an alias to their annotation in the future update. I will keep track of that so we can bump the version of the checker framework in the future to better solve other suppressed violations. The issue I opened: typetools/checker-framework#6507

They had majorly introduced the nullness annotations in release 2.1.14. Unfortunately, this is outdated by 8 solid years :)
It would be great if they add the update coz we don't have any other option to jsr 305, but I'm not sure if this will be happening anytime soon, given their update history in regard of this😅

romani

ok to merge

romani · 2024-03-23T13:14:43Z

src/main/java/com/puppycrawl/tools/checkstyle/DetailAstImpl.java

@@ -384,7 +384,7 @@ private BitSet getBranchTokenTypes() {

    @Override
    public boolean branchContains(int tokenType) {
-        return getBranchTokenTypes().get(tokenType);
+        return tokenType >= 1 && getBranchTokenTypes().get(tokenType);


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,

nrmancuso · 2024-03-26T15:17:11Z

@romani, the checker framework may add the @Nonnegative from javax as an alias to their annotation in the future update. I will keep track of that so we can bump the version of the checker framework in the future to better solve other suppressed violations. The issue I opened: typetools/checker-framework#6507

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.

romani · 2024-03-26T16:04:46Z

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 branchContains . API method should not throw exceptions if that is possible. Bitset is details of implementation, we should not leak fact that we are limited to Bitset.
Non of search like API throws exceptions if parameters are not good, they just return nothing.

romani · 2024-03-26T16:05:12Z

@rnveach , please share your opinion.

nrmancuso · 2024-03-26T16:17:45Z

Bitset is details of implementation, we should not leak fact that we are limited to Bitset.

I agree with this statement, we should create an issue to discuss not using bitset.

Sure, one day we will do summary. But if we pause, we will never find next problems for us.

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

rnveach · 2024-03-26T17:41:04Z

please share your opinion.

Which am I sharing on?

romani · 2024-03-26T21:27:39Z

create an issue to discuss not using bitset.

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 contains, we do not throw exception on not expect content, we just return false.

romani · 2024-03-30T00:56:24Z

@rnveach , here is a discussion #14673 (comment)

here is my answer - #14673 (comment)

rnveach · 2024-05-12T21:43:30Z

Here is my answer - #14673 (comment)

Let me know when we have decided on a group decision.

Lmh-java marked this pull request as draft March 17, 2024 07:34

Lmh-java force-pushed the minghao/resolve-checkerframework-violation branch from 3301fe0 to 7fa2268 Compare March 17, 2024 07:47

Lmh-java marked this pull request as ready for review March 17, 2024 08:12

romani requested changes Mar 17, 2024

View reviewed changes

romani self-assigned this Mar 17, 2024

romani requested changes Mar 17, 2024

View reviewed changes

Lmh-java force-pushed the minghao/resolve-checkerframework-violation branch from 7fa2268 to 3729e24 Compare March 18, 2024 02:02

Issue checkstyle#14084: fix Checker violation for nonnegative integer…

7daa526

… argument

Lmh-java force-pushed the minghao/resolve-checkerframework-violation branch from 3729e24 to 7daa526 Compare March 18, 2024 04:48

Lmh-java requested a review from romani March 18, 2024 06:14

romani approved these changes Mar 23, 2024

View reviewed changes

romani assigned nrmancuso and unassigned romani Mar 23, 2024

romani requested a review from nrmancuso March 23, 2024 13:15

nrmancuso assigned romani and unassigned nrmancuso Mar 26, 2024

romani assigned nrmancuso and unassigned romani Mar 26, 2024

romani assigned rnveach and unassigned nrmancuso Mar 26, 2024

rnveach assigned romani Mar 29, 2024

rnveach removed their assignment Mar 29, 2024

romani assigned rnveach and unassigned romani Mar 30, 2024

rnveach assigned romani and unassigned rnveach Mar 30, 2024

romani assigned rnveach and unassigned romani May 11, 2024

rnveach added the awaiting response label May 20, 2024

rnveach removed their assignment May 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #14084: fix Checker violation for nonnegative integer argument #14673

Issue #14084: fix Checker violation for nonnegative integer argument #14673

Lmh-java commented Mar 17, 2024 •

edited

romani left a comment

romani Mar 17, 2024

MANISH-K-07 Mar 17, 2024

Lmh-java Mar 17, 2024 •

edited

MANISH-K-07 Mar 17, 2024

Lmh-java Mar 17, 2024 •

edited

romani Mar 17, 2024

MANISH-K-07 Mar 17, 2024

Lmh-java Mar 17, 2024

Lmh-java Mar 17, 2024

romani left a comment

romani left a comment

romani Mar 17, 2024

Lmh-java Mar 18, 2024 •

edited

nrmancuso Mar 26, 2024 •

edited

romani Mar 26, 2024 •

edited

nrmancuso Mar 26, 2024

nrmancuso Mar 26, 2024 •

edited

romani Mar 30, 2024

rnveach Mar 30, 2024 •

edited

romani Apr 1, 2024

romani May 13, 2024

Lmh-java commented Mar 18, 2024 •

edited

MANISH-K-07 commented Mar 18, 2024 •

edited

romani left a comment

romani Mar 23, 2024

nrmancuso commented Mar 26, 2024

romani commented Mar 26, 2024

romani commented Mar 26, 2024

nrmancuso commented Mar 26, 2024 •

edited

rnveach commented Mar 26, 2024

romani commented Mar 26, 2024

romani commented Mar 30, 2024 •

edited

rnveach commented May 12, 2024

		@@ -19,6 +19,8 @@

		package com.puppycrawl.tools.checkstyle.api;

		import org.checkerframework.checker.index.qual.NonNegative;

Issue #14084: fix Checker violation for nonnegative integer argument #14673

Are you sure you want to change the base?

Issue #14084: fix Checker violation for nonnegative integer argument #14673

Conversation

Lmh-java commented Mar 17, 2024 • edited

romani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lmh-java Mar 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lmh-java Mar 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lmh-java Mar 18, 2024 • edited

Choose a reason for hiding this comment

nrmancuso Mar 26, 2024 • edited

Choose a reason for hiding this comment

romani Mar 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrmancuso Mar 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach Mar 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lmh-java commented Mar 18, 2024 • edited

MANISH-K-07 commented Mar 18, 2024 • edited

romani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrmancuso commented Mar 26, 2024

romani commented Mar 26, 2024

romani commented Mar 26, 2024

nrmancuso commented Mar 26, 2024 • edited

rnveach commented Mar 26, 2024

romani commented Mar 26, 2024

romani commented Mar 30, 2024 • edited

rnveach commented May 12, 2024

Lmh-java commented Mar 17, 2024 •

edited

Lmh-java Mar 17, 2024 •

edited

Lmh-java Mar 17, 2024 •

edited

Lmh-java Mar 18, 2024 •

edited

nrmancuso Mar 26, 2024 •

edited

romani Mar 26, 2024 •

edited

nrmancuso Mar 26, 2024 •

edited

rnveach Mar 30, 2024 •

edited

Lmh-java commented Mar 18, 2024 •

edited

MANISH-K-07 commented Mar 18, 2024 •

edited

nrmancuso commented Mar 26, 2024 •

edited

romani commented Mar 30, 2024 •

edited