Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add @CanIgnoreReturnValue as appropriate to Gson methods. #2369

Merged
merged 1 commit into from
Apr 10, 2023

Conversation

eamonnmcmanus
Copy link
Member

@eamonnmcmanus eamonnmcmanus commented Apr 10, 2023

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 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.
@@ -1664,7 +1664,7 @@
*/
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

Variable 'int unused' is never read.
@@ -1223,7 +1223,7 @@
boolean oldLenient = reader.isLenient();
reader.setLenient(true);
try {
reader.peek();
JsonToken unused = reader.peek();

Check notice

Code scanning / CodeQL

Unread local variable

Variable 'JsonToken unused' is never read.
@@ -44,7 +45,7 @@
public static JsonElement parse(JsonReader reader) throws JsonParseException {
boolean isEmpty = true;
try {
reader.peek();
JsonToken unused = reader.peek();

Check notice

Code scanning / CodeQL

Unread local variable

Variable 'JsonToken unused' is never read.
@@ -1119,7 +1119,7 @@
return;
} else if (c == '\\') {
pos = p;
readEscapeCharacter();
char unused = readEscapeCharacter();

Check notice

Code scanning / CodeQL

Unread local variable

Variable 'char unused' is never read.
@eamonnmcmanus eamonnmcmanus requested a review from kluever April 10, 2023 17:32
Copy link
Member

@kluever kluever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eamonnmcmanus eamonnmcmanus merged commit c1da2d7 into google:master Apr 10, 2023
@eamonnmcmanus eamonnmcmanus deleted the cirv branch April 10, 2023 17:50
Copy link
Collaborator

@Marcono1234 Marcono1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope it is ok that I added a few review comments.

However, we are not enabling CheckReturnValue by 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)?

@@ -15,6 +15,7 @@
*/
package com.google.gson;

import com.google.errorprone.annotations.CanIgnoreReturnValue;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed.

Comment on lines +147 to 148
@CanIgnoreReturnValue
public JsonNull getAsJsonNull() {
Copy link
Collaborator

@Marcono1234 Marcono1234 Apr 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there should be a comment explaining this? E.g.

@CanIgnoreReturnValue // For when this method is used only to verify that the value is JsonNull

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +130 to 131
URL unused = gson.fromJson('"' + urlValue + '"', URL.class);
assertThat(target.toExternalForm()).isEqualTo(urlValue);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Suggested change
URL unused = gson.fromJson('"' + urlValue + '"', URL.class);
assertThat(target.toExternalForm()).isEqualTo(urlValue);
target = gson.fromJson('"' + urlValue + '"', URL.class);
assertThat(target.toExternalForm()).isEqualTo(urlValue);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think you're right. Well spotted!

Comment on lines +132 to +133
Employee unused1 = new Employee("Jesse", google);
Employee unused2 = new Employee("Joel", google);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to add a comment here explaining this?

Suggested change
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);

@eamonnmcmanus
Copy link
Member Author

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

Yes. People can enable this checking by compiling with -XepOpt:CheckReturnValue:Packages=com.google.gson. We could indeed add @CheckReturnValue to the various package-info.java files, and we might yet do that, but it could potentially be a bit intrusive. I've updated all of Google's internal source code to add the appropriate var unused = ... but I'm not sure I want to impose that on external users.

eamonnmcmanus added a commit that referenced this pull request May 31, 2023
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.
tibor-universe pushed a commit to getuniverse/gson that referenced this pull request Sep 14, 2024
)

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants