Nullness annotations to gson [review 1] #1
Conversation
There was a problem hiding this comment.
@PRITI1999 This is an initial review. Please go through my comments and address them. When you are ready, request my review again. Good job so far!
| @@ -146,7 +147,7 @@ public boolean hasModifier(int modifier) { | |||
| * @throws IllegalAccessException | |||
| * @throws IllegalArgumentException | |||
| */ | |||
| Object get(Object instance) throws IllegalAccessException { | |||
| @Nullable Object get(Object instance) throws IllegalAccessException { | |||
There was a problem hiding this comment.
In the documentation it is not specified that the return result can be null. This annotation is forced here because the method get(Object) in java.lang.reflect has its return type annotated with @Nullable. I am not sure if this annotation is correct, since get from jdk.internal.reflect.FieldAccessor.java is not annotated as @Nullable.
| @@ -39,7 +41,6 @@ | |||
| public JsonArray() { | |||
| elements = new ArrayList<JsonElement>(); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Avoid this type of changes.
gson/src/main/java/com/google/gson/internal/$Gson$Preconditions.java
Outdated
Show resolved
Hide resolved
| @@ -389,8 +392,10 @@ static void checkValidFloatingPoint(double value) { | |||
| @Override public void write(JsonWriter out, AtomicLong value) throws IOException { | |||
| longAdapter.write(out, value.get()); | |||
| } | |||
| /*Possible defect in Gson code as read(in) #3 may return null*/ | |||
| @SuppressWarnings("dereference.of.nullable") | |||
There was a problem hiding this comment.
The method is private. Still, can you verify that it is not possible to throw a NullPointerException internally?
There was a problem hiding this comment.
No NullPointerException is thrown internally but as the method is private developers ensure the concerned read function does not return null so the code is safe
| @@ -192,11 +193,13 @@ public Gson() { | |||
| Collections.<TypeAdapterFactory>emptyList()); | |||
| } | |||
|
|
|||
| /*dangerous to call an instance method inside a constructor, framework could identify #1 and #2 out of 4 such errors*/ | |||
| @SuppressWarnings("method.invocation.invalid") | |||
There was a problem hiding this comment.
The errors you suppressed are InitializationChecker errors. You can use this when doing nullness case studies. Can you make sure adding such annotations still produce method.invocation.invalid errors?
There was a problem hiding this comment.
@Maxi17 I could not completely understand this comment. Adding which annotations still produce this error?
There was a problem hiding this comment.
I am saying that the errors you are suppressing are because of InitializationChecker. What I'm asking is if you add InitializationChecker annotations (link @Initialized) works here.
There was a problem hiding this comment.
Annotation needs to be added to the current object or this which is not possible, therefore the suppression
| @@ -192,11 +193,13 @@ public Gson() { | |||
| Collections.<TypeAdapterFactory>emptyList()); | |||
| } | |||
|
|
|||
| /*dangerous to call an instance method inside a constructor, framework could identify #1 and #2 out of 4 such errors*/ | |||
| @SuppressWarnings("method.invocation.invalid") | |||
There was a problem hiding this comment.
I am saying that the errors you are suppressing are because of InitializationChecker. What I'm asking is if you add InitializationChecker annotations (link @Initialized) works here.
gson/src/main/java/com/google/gson/internal/$Gson$Preconditions.java
Outdated
Show resolved
Hide resolved
|
@Maxi17 nullness annotations are added according to week 2 of plan. |
gson/src/main/java/com/google/gson/internal/reflect/UnsafeReflectionAccessor.java
Outdated
Show resolved
Hide resolved
| try { | ||
| final Constructor<? super T> constructor = rawType.getDeclaredConstructor(); | ||
| if (!constructor.isAccessible()) { | ||
| accessor.makeAccessible(constructor); | ||
| } | ||
| return new ObjectConstructor<T>() { | ||
| @SuppressWarnings("unchecked") // T is the same raw type as is requested | ||
| /*Probable missing annotation in java.lang.reflect.Constructor.java | ||
| #1 sends null to a variable length argument*/ |
There was a problem hiding this comment.
After you add the annotation in the jdk, make sure you re-run the case study and fix all @SuppressWarnings caused by missing jdk annotations.
| @@ -31,6 +32,9 @@ | |||
| public abstract class UnsafeAllocator { | |||
| public abstract <T> T newInstance(Class<T> c) throws Exception; | |||
|
|
|||
| @CFComment({"possible missing annotation in jdk for class Field, method get as the doc says it can take null argument #1", | |||
| "class Method, function invoke is annotated to receive non-null args but CFComment says it might permit null #2"}) | |||
| @SuppressWarnings({"nullness:argument.type.incompatible","unboxing.of.nullable"}) | |||
There was a problem hiding this comment.
Seems like more annotations need to be added in the jdk. Remove the comment after you do so.
|
|
||
|
|
| @@ -157,7 +158,9 @@ public Adapter(Gson context, Type keyType, TypeAdapter<K> keyTypeAdapter, | |||
| this.constructor = constructor; | |||
| } | |||
|
|
|||
| @Override public Map<K, V> read(JsonReader in) throws IOException { | |||
| //INSTANCE in #3 may be null as JsonReaderInternalAccess do not initialize it | |||
There was a problem hiding this comment.
This looks like a bug in Gson. Can you produce a test case in which INSTANCE is null and the program crashes?
There was a problem hiding this comment.
The method is defined inside a private class and is never called so can this method be ruled out as safe?
| @@ -65,18 +69,20 @@ boolean makeAccessibleWithUnsafe(AccessibleObject ao) { | |||
| return false; | |||
| } | |||
|
|
|||
| private static Object getUnsafeInstance() { | |||
| //null should not be sent to get method #1 | |||
There was a problem hiding this comment.
If null should not be sent to this smethod, then why is the code safe?
@Maxi17, annotations are added for nullness checker according to week 1 of plan.