Add @CanIgnoreReturnValue as appropriate to Gson methods. - #2369
Conversation
This annotation indicates that return value of the annotated method does not need to be used. If it is _not_ present on a non-void method, and if Error Prone's `CheckReturnValue` is active, then calling the method without using the result is an error. However, we are not enabling `CheckReturnValue` by default here. Also update some code that does ignore return values, so that the returned value is used, if only by assigning it to an unused variable.
| private void consumeNonExecutePrefix() throws IOException { | ||
| // fast forward through the leading whitespace | ||
| nextNonWhitespace(true); | ||
| int unused = nextNonWhitespace(true); |
Check notice
Code scanning / CodeQL
Unread local variable
| reader.setLenient(true); | ||
| try { | ||
| reader.peek(); | ||
| JsonToken unused = reader.peek(); |
Check notice
Code scanning / CodeQL
Unread local variable
| boolean isEmpty = true; | ||
| try { | ||
| reader.peek(); | ||
| JsonToken unused = reader.peek(); |
Check notice
Code scanning / CodeQL
Unread local variable
| } else if (c == '\\') { | ||
| pos = p; | ||
| readEscapeCharacter(); | ||
| char unused = readEscapeCharacter(); |
Check notice
Code scanning / CodeQL
Unread local variable
Marcono1234
left a comment
There was a problem hiding this comment.
I hope it is ok that I added a few review comments.
However, we are not enabling
CheckReturnValueby default here.
Do you mean with this, that if users were using Error Prone they would not see any warnings? Would it be necessary to add @CheckReturnValue to the package-info.java files of Gson to achieve that (as hinted by the @CanIgnoreReturnValue documentation)?
| */ | ||
| package com.google.gson; | ||
|
|
||
| import com.google.errorprone.annotations.CanIgnoreReturnValue; |
| @CanIgnoreReturnValue | ||
| public JsonNull getAsJsonNull() { |
There was a problem hiding this comment.
Maybe there should be a comment explaining this? E.g.
@CanIgnoreReturnValue // For when this method is used only to verify that the value is JsonNullThere was a problem hiding this comment.
That's fair. I had to do the var unused = ... in a few places in Google's source code that were for example calling getAsBoolean() to check that it was indeed a Boolean and not something else. Here there's basically no other reason to call it, since it can only ever return JsonNull.INSTANCE.
| URL unused = gson.fromJson('"' + urlValue + '"', URL.class); | ||
| assertThat(target.toExternalForm()).isEqualTo(urlValue); |
There was a problem hiding this comment.
This might actually be a bug in the test; that assertion is exactly the same as in line 128, without any of the values having changed. Should maybe be the following?
| URL unused = gson.fromJson('"' + urlValue + '"', URL.class); | |
| assertThat(target.toExternalForm()).isEqualTo(urlValue); | |
| target = gson.fromJson('"' + urlValue + '"', URL.class); | |
| assertThat(target.toExternalForm()).isEqualTo(urlValue); |
There was a problem hiding this comment.
Yes, I think you're right. Well spotted!
| Employee unused1 = new Employee("Jesse", google); | ||
| Employee unused2 = new Employee("Joel", google); |
There was a problem hiding this comment.
Might be good to add a comment here explaining this?
| Employee unused1 = new Employee("Jesse", google); | |
| Employee unused2 = new Employee("Joel", google); | |
| // Employee constructor adds `this` to the given Company object | |
| Employee unused1 = new Employee("Jesse", google); | |
| Employee unused2 = new Employee("Joel", google); |
Yes. People can enable this checking by compiling with |
This annotation indicates that return value of the annotated method does not need to be used. If it is _not_ present on a non-void method, and if Error Prone's `CheckReturnValue` is active, then calling the method without using the result is an error. However, we are not enabling `CheckReturnValue` by default here. Also update some code that does ignore return values, so that the returned value is used, if only by assigning it to an unused variable.
) This annotation indicates that return value of the annotated method does not need to be used. If it is _not_ present on a non-void method, and if Error Prone's `CheckReturnValue` is active, then calling the method without using the result is an error. However, we are not enabling `CheckReturnValue` by default here. Also update some code that does ignore return values, so that the returned value is used, if only by assigning it to an unused variable.
This annotation indicates that the return value of the annotated method does not need to be used. If it is not present on a non-void method, and if Error Prone's
CheckReturnValueis active, then calling the method without using the result is an error. However, we are not enablingCheckReturnValueby default here.Also update some code that does ignore return values, so that the returned value is used, if only by assigning it to an unused variable.