Add Index Checker annotations #1

panacekcz · 2018-02-22T17:29:11Z

Adds Index Checker annotations.

Only the packages com.google.common.primitives and com.google.common.base are ~~fully~~ annotated, ~~so to typecheck, run the Index Checker with -AonlyDefs=^com\.google\.common\.primitives\.|^com\.google\.common\.base\..~~

In order to get implicit annotations in class files, the Index Checker is run on all files, but index warnings are suppressed. To typecheck the packages that are annotated by index annotations, the Index checker is run in another phase only on these packages with warnings enabled.
See the properties in guava/pom.xml.

The used version of the Checker Framework is increased so that new annotations can be used.

…er-pr

Conflicts: .travis.yml README-typetools.md guava/pom.xml

panacekcz · 2018-05-10T15:03:08Z

@kelloggm, I have addressed the comments.
@mernst, I have merged the master branch.

mernst

Here are some small things that will improve the quality, and reduce the size, of the pull request.

mernst · 2018-05-16T04:55:39Z

guava/pom.xml

    <annotatedJdk>${org.checkerframework:jdk8:jar}</annotatedJdk>
+    <checkerframework.extraargs></checkerframework.extraargs>


Can you document what these two variables (extraargs and extraargs2) are and how they should be used?
The same goes for index.only.arg.

mernst · 2018-05-16T05:01:42Z

guava/src/com/google/common/base/Ascii.java

@@ -17,6 +17,8 @@
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;

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


As a matter of style, Google puts imports in alphabetical order, and with no blank lines (except a blank line separating import static from regular import). So this pull request should conform to that style, here and elsewhere.

mernst · 2018-05-16T05:03:28Z

guava/src/com/google/common/base/Ascii.java

    checkNotNull(seq);

    // length to truncate the sequence to, not including the truncation indicator
-    int truncationLength = maxLength - truncationIndicator.length();
+    @NonNegative int truncationLength = maxLength - truncationIndicator.length();


Is this local variable annotation needed?
Usually we only need one if there is a @Suppresswarnings or if the variable is re-assigned after being set, and neither seems to be true here.
(I don't have a particular objection to it, since it is useful documentation, but having smaller diffs and fewer annotations would be desirable.)
There are a number of other local variables that also have annotations but no @SuppressWarnings, so I wonder if they should also be removed.

There is @SuppressWarnings on this method.

That makes sense, thanks.

mernst · 2018-05-16T05:13:01Z

guava/src/com/google/common/base/Preconditions.java

-  public static int checkElementIndex(int index, int size) {
-    return checkElementIndex(index, size, "index");
+  public static @NonNegative int checkElementIndex(@NonNegative int index, @NonNegative int size) {
+    return checkElementIndex(index, size, "index"); // This comment contains @AnnotatedFor so that this line is ignored by count-suppressions


Please remove all comments like this, to keep the diffs small.

We use the count-suppressions script only to estimate which bugs/features to add to the Index Checker, but not for any statistics in a paper. (Or, that should be true if it is not currently!)
I could try to improve the regex in count-suppressions if this is too big an annoyance during development.

It is used by the script print-fps.sh in the reproduction package.

Although the to comment improves the numbers slightly, I don't think this should be merged into master because it would pollute the source code.

How about changing count-suppressions.sh to filter out lines that end with ;? Can you try that and see if it changes the numbers? If not, we could remove these comments and make that change to count-suppressions.sh instead. (If necessary, leave this pull request open for the ISSTA reviewers, and make a new one that eliminates the extraneous comments.)

Done: typetools/checker-framework#1989. The usage of count-suppressions seems to allow these comments to be remvoed here.

mernst · 2018-05-16T05:19:15Z

guava/src/com/google/common/base/Throwables.java

@@ -34,6 +34,8 @@
 import java.util.List;
 import org.checkerframework.checker.nullness.compatqual.NullableDecl;

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


This should go before the nullness import, in alphabetical order.

Conflicts: guava/src/com/google/common/base/CaseFormat.java guava/src/com/google/common/base/Function.java guava/src/com/google/common/base/Functions.java guava/src/com/google/common/base/Joiner.java guava/src/com/google/common/base/Objects.java guava/src/com/google/common/base/Platform.java guava/src/com/google/common/base/Preconditions.java guava/src/com/google/common/base/Predicate.java guava/src/com/google/common/base/Predicates.java guava/src/com/google/common/base/Strings.java guava/src/com/google/common/base/Suppliers.java guava/src/com/google/common/base/Throwables.java guava/src/com/google/common/base/internal/Finalizer.java guava/src/com/google/common/collect/AbstractBiMap.java guava/src/com/google/common/collect/AbstractIterator.java guava/src/com/google/common/collect/AbstractListMultimap.java guava/src/com/google/common/collect/AbstractMapBasedMultiset.java guava/src/com/google/common/collect/AbstractMapEntry.java guava/src/com/google/common/collect/AbstractMultimap.java guava/src/com/google/common/collect/AbstractMultiset.java guava/src/com/google/common/collect/AbstractSetMultimap.java guava/src/com/google/common/collect/AbstractSortedSetMultimap.java guava/src/com/google/common/collect/ArrayListMultimap.java guava/src/com/google/common/collect/BiMap.java guava/src/com/google/common/collect/ByFunctionOrdering.java guava/src/com/google/common/collect/ClassToInstanceMap.java guava/src/com/google/common/collect/Collections2.java guava/src/com/google/common/collect/ComparatorOrdering.java guava/src/com/google/common/collect/CompoundOrdering.java guava/src/com/google/common/collect/ComputationException.java guava/src/com/google/common/collect/ConcurrentHashMultiset.java guava/src/com/google/common/collect/EnumBiMap.java guava/src/com/google/common/collect/EnumHashBiMap.java guava/src/com/google/common/collect/EnumMultiset.java guava/src/com/google/common/collect/ExplicitOrdering.java guava/src/com/google/common/collect/ForwardingCollection.java guava/src/com/google/common/collect/ForwardingConcurrentMap.java guava/src/com/google/common/collect/ForwardingList.java guava/src/com/google/common/collect/ForwardingMap.java guava/src/com/google/common/collect/ForwardingMapEntry.java guava/src/com/google/common/collect/ForwardingMultimap.java guava/src/com/google/common/collect/ForwardingMultiset.java guava/src/com/google/common/collect/ForwardingSet.java guava/src/com/google/common/collect/ForwardingSortedMap.java guava/src/com/google/common/collect/ForwardingSortedSet.java guava/src/com/google/common/collect/HashBiMap.java guava/src/com/google/common/collect/HashMultimap.java guava/src/com/google/common/collect/HashMultiset.java guava/src/com/google/common/collect/Hashing.java guava/src/com/google/common/collect/ImmutableClassToInstanceMap.java guava/src/com/google/common/collect/ImmutableCollection.java guava/src/com/google/common/collect/ImmutableEntry.java guava/src/com/google/common/collect/ImmutableEnumSet.java guava/src/com/google/common/collect/ImmutableList.java guava/src/com/google/common/collect/ImmutableListMultimap.java guava/src/com/google/common/collect/ImmutableMap.java guava/src/com/google/common/collect/ImmutableMultimap.java guava/src/com/google/common/collect/ImmutableMultiset.java guava/src/com/google/common/collect/ImmutableSet.java guava/src/com/google/common/collect/ImmutableSetMultimap.java guava/src/com/google/common/collect/ImmutableSortedMap.java guava/src/com/google/common/collect/ImmutableSortedSet.java guava/src/com/google/common/collect/Iterables.java guava/src/com/google/common/collect/Iterators.java guava/src/com/google/common/collect/LinkedHashMultimap.java guava/src/com/google/common/collect/LinkedHashMultiset.java guava/src/com/google/common/collect/LinkedListMultimap.java guava/src/com/google/common/collect/ListMultimap.java guava/src/com/google/common/collect/Lists.java guava/src/com/google/common/collect/MapDifference.java guava/src/com/google/common/collect/Maps.java guava/src/com/google/common/collect/Multimap.java guava/src/com/google/common/collect/Multimaps.java guava/src/com/google/common/collect/Multiset.java guava/src/com/google/common/collect/Multisets.java guava/src/com/google/common/collect/MutableClassToInstanceMap.java guava/src/com/google/common/collect/NullsFirstOrdering.java guava/src/com/google/common/collect/NullsLastOrdering.java guava/src/com/google/common/collect/ObjectArrays.java guava/src/com/google/common/collect/Ordering.java guava/src/com/google/common/collect/RegularImmutableBiMap.java guava/src/com/google/common/collect/RegularImmutableList.java guava/src/com/google/common/collect/RegularImmutableMap.java guava/src/com/google/common/collect/RegularImmutableSet.java guava/src/com/google/common/collect/RegularImmutableSortedSet.java guava/src/com/google/common/collect/ReverseOrdering.java guava/src/com/google/common/collect/SetMultimap.java guava/src/com/google/common/collect/Sets.java guava/src/com/google/common/collect/SingletonImmutableSet.java guava/src/com/google/common/collect/SortedSetMultimap.java guava/src/com/google/common/collect/Synchronized.java guava/src/com/google/common/collect/TreeMultimap.java guava/src/com/google/common/collect/TreeMultiset.java guava/src/com/google/common/primitives/Booleans.java guava/src/com/google/common/primitives/Bytes.java guava/src/com/google/common/primitives/Chars.java guava/src/com/google/common/primitives/Doubles.java guava/src/com/google/common/primitives/Floats.java guava/src/com/google/common/primitives/ImmutableDoubleArray.java guava/src/com/google/common/primitives/ImmutableIntArray.java guava/src/com/google/common/primitives/ImmutableLongArray.java guava/src/com/google/common/primitives/Ints.java guava/src/com/google/common/primitives/Longs.java guava/src/com/google/common/primitives/Shorts.java guava/src/com/google/common/primitives/UnsignedInteger.java guava/src/com/google/common/primitives/UnsignedLong.java

Putting the variable at the end caused no errors to be reported.

panacekcz · 2018-05-22T16:01:28Z

@mernst, I have adressed the comments

mernst · 2018-05-22T17:25:00Z

Thanks. This looks good to me now -- I think it can be merged.

mernst · 2018-05-24T14:41:42Z

Don't delete the branch yet -- leave it for the ISSTA AEC.

The old implementation had two bugs: 1. Tasks that threw RejectedExecutionException on submission would still execute later 2. Two threads submitting to an idle SequentialExecutor might see one submission succeed, even if the delegate threw REE. The submitted task wouldn't run until someone else successfully submitted another task. This costs a possible extra lock acquisition (to solve typetools#2) and an allocation (to solve typetools#1) when submitting to a SequentialExecutor that doesn't already have worker scheduled or running. RELNOTES=n/a ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=181810127

To work with local Checker Framework installation.

panacekcz added 30 commits November 8, 2017 20:42

Add links to suppressed warnings

4a58874

Unlimit warnings

08a2123

Update checker output

2f0e43b

Add index annotations (mainly @nonnegative size())

7abbb74

Update checker output

d69c878

Add lower bound annotations

3e2884e

Fix imports

34b7720

Update checker output

8fdafdb

Add index annotations

730d409

Annotate AbstractIndexedListIterator

55e90a4

Annotate UnsignedBytes

f6c00fc

Annotate SignedBytes

5b3a069

Annotate UnsignedInts

2efca21

Annotate UnsignedLongs

f4f0a11

Annotate Booleans

3290da1

Annotate Bytes

584e856

Annotate com.google.common.primitives

72b188c

Annotate immutable primitive arrays

9bcc93b

Annotate hashing streams

d561798

Annotate Strings

be07d88

Annotate Splitter

bf672b1

Range annotations

f87cc1e

Index annotations

af6dd28

Annotate math

49298d3

Annnotate TopKSelector

c011ca7

Add index annotations

2f411e1

Merge branch 'index-checker' into index-checker-suppress

51691df

Add SuppressWarnings for kelloggm/checker-framework#149

fd097e3

Limit warnigns to com.google.common.primitives

ba9c40a

Update checker output

dbd3311

panacekcz added 5 commits May 10, 2018 00:47

Fix capitalization

5cfa557

Remove references to kelloggm/checker-framework#176

530addc

Merge remote-tracking branch 'fork/index-checker-pr' into index-check…

4d92b4a

…er-pr

Merge remote-tracking branch 'origin/master' into index-checker-pr

30fb31c

Conflicts: .travis.yml README-typetools.md guava/pom.xml

Replace tabs by spaces

5168188

Comment tweaks

4b07df2

mernst reviewed May 16, 2018

View reviewed changes

mernst and others added 13 commits May 15, 2018 22:39

Remove trailing whitespace

e23dc56

Format imports

3a17bc6

Documentation of additional arguments

5181f28

Fix import

dc6e159

Put additional arguments earlier in the check-index phase.

d4a2083

Putting the variable at the end caused no errors to be reported.

Make suppression key more specific

2bd722c

Add local variable annotation

b5b4581

Add back changes lost when merging

afad2ec

Remove workarounds for count-suppressions

24a244e

Remove unnecessary local variable annotation

ff3d2e4

Fix whitespace

3480d53

Remove local variable annotation

fffa690

mernst merged commit 76189c3 into typetools:master May 23, 2018

NhatDinh referenced this pull request in NhatDinh/guava Jun 8, 2018

Replace @LTLengthOf("#1") with @LTEqLengthOf("#1")

1a85875

kelloggm pushed a commit that referenced this pull request Jul 17, 2018

Add @IndexOrHigh("#1") for nextEscapeIndex()

961632d

smillst pushed a commit that referenced this pull request Aug 23, 2019

Move errorprone to global dependency (#1)

85372bb

To work with local Checker Framework installation.

markro49 mentioned this pull request May 14, 2023

More @SuppressionWarnings cleanup #71

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Index Checker annotations #1

Add Index Checker annotations #1

panacekcz commented Feb 22, 2018 •

edited

Loading

panacekcz commented May 10, 2018

mernst left a comment

mernst May 16, 2018

mernst May 16, 2018

mernst May 16, 2018

panacekcz May 22, 2018

mernst May 22, 2018

mernst May 16, 2018

panacekcz May 22, 2018

mernst May 22, 2018

panacekcz May 22, 2018

mernst May 16, 2018

panacekcz commented May 22, 2018

mernst commented May 22, 2018

mernst commented May 24, 2018

		<annotatedJdk>${org.checkerframework:jdk8:jar}</annotatedJdk>
		<checkerframework.extraargs></checkerframework.extraargs>

Add Index Checker annotations #1

Add Index Checker annotations #1

Conversation

panacekcz commented Feb 22, 2018 • edited Loading

panacekcz commented May 10, 2018

mernst left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panacekcz commented May 22, 2018

mernst commented May 22, 2018

mernst commented May 24, 2018

panacekcz commented Feb 22, 2018 •

edited

Loading