Added annotations to org.apache.dubbo.common.io #1
Conversation
| <dependency> | ||
| <groupId>org.checkerframework</groupId> | ||
| <artifactId>checker</artifactId> | ||
| <version>2.8.1</version> |
There was a problem hiding this comment.
Should be latest release when this is merged (so 2.8.2 right now)
| @@ -434,7 +440,10 @@ public static String bytes2hex(byte[] bs, int off, int len) { | |||
| * @param len length. | |||
| * @return byte array. | |||
| */ | |||
| public static byte[] hex2bytes(final String str, final int off, int len) { | |||
| @SuppressWarnings({"argument.type.incompatible", "array.access.unsafe.high"}) /* | |||
There was a problem hiding this comment.
Here and elsewhere: can you preface these suppress warning annotations with index: (i.e. @SuppressWarnings({"index:argument.type.incompatible", "index:array.access.unsafe.high"})). That way if someone else typechecks this library with a different checker, their warnings won't be suppressed. It's also just good style.
| } | ||
| DECODE_TABLE_MAP.put(hash, ret); | ||
| } | ||
| return ret; | ||
| } | ||
|
|
||
| @SuppressWarnings({"array.length.negative", "argument.type.incompatible"}) // Preconditions are enforced by the only caller of this private method. |
There was a problem hiding this comment.
Why can't the preconditions be written as annotations?
| @@ -873,6 +901,10 @@ private static int indexOf(char[] cs, char c) { | |||
| return -1; | |||
| } | |||
|
|
|||
| @SuppressWarnings({"array.access.unsafe", "argument.type.incompatible"}) /* | |||
| #1. ret has length of 128, the loop stops at position 127 | |||
| #2. It was verified that code has at least 64 characters. The ascii value of a chr is at most 127, which is a valid index for ret | |||
There was a problem hiding this comment.
The checker ought to know that the maximum value of a char is 127. If that doesn't work, I would file a bug with a test case like:
void test(int @MinLen(128) [] array, @NonNegative char c) {
array[c];
}| /** | ||
| * UnsafeByteArrayInputStream. | ||
| */ | ||
| public class UnsafeByteArrayInputStream extends InputStream { | ||
| protected byte[] mData; | ||
|
|
||
| protected int mPosition, mLimit, mMark = 0; | ||
| protected @IndexOrHigh("this.mData") int mPosition, mMark, mLimit = 0; |
There was a problem hiding this comment.
Why not annotated mPosition as @LessThan("this.mLimit")? You use that fact to justify several warning suppressions later in the file.
| /** | ||
| * Thread-unsafe StringReader. | ||
| */ | ||
| public class UnsafeStringReader extends Reader { | ||
| private String mString; | ||
|
|
||
| private int mPosition, mLimit, mMark; | ||
| private @IndexOrHigh("this.mString") int mPosition,mLimit, mMark; |
There was a problem hiding this comment.
Missing space in this line
| @@ -34,7 +40,9 @@ public UnsafeStringReader(String str) { | |||
| } | |||
|
|
|||
| @Override | |||
| public int read() throws IOException { | |||
| @SuppressWarnings({"return.type.incompatible", "argument.type.incompatible", "compound.assignment.type.incompatible"}) | |||
| // A char is always greater than 0 and it has been previously verified that mPosition is less than the length of mString | |||
There was a problem hiding this comment.
Where/how was that verified? If it's a precondition of the method, you should express it as an annotation instead.
| } | ||
|
|
||
| @Override | ||
| public long skip(long ns) throws IOException { | ||
| @SuppressWarnings("compound.assignment.type.incompatible") // n is valid because it was previously verified | ||
| public @NonNegative long skip(long ns) throws IOException { |
There was a problem hiding this comment.
I'm not convinced that this is the right specification for this method: I would expect that the argument ns should be @NonNegative. Why isn't that the case here?
| @@ -80,7 +84,8 @@ public Writer append(CharSequence csq) { | |||
| } | |||
|
|
|||
| @Override | |||
| public Writer append(CharSequence csq, int start, int end) { | |||
| @SuppressWarnings("argument.type.incompatible") // The documentation is inherited from the overridden method. | |||
There was a problem hiding this comment.
I don't understand what you mean by this? Are you saying that the spec on the inherited method is wrong?
There was a problem hiding this comment.
I tried to say that the implementation of this method is similar to the overridden one, which is part of the JDK. That one has a documentation and specifies that the method can throw an IndexOutOfBoundsException
There was a problem hiding this comment.
Can you clarify in the comment exactly where the warning is being suppressed, and how the exception can be thrown? Alternatively, you could add a link to the JDK documentation on the web so that a reader can easily read it themselves. As it is now, it's very confusing.
Apache Dubbo io case study