feat: create new page for the unused return value checker #5169
Conversation
docs/topics/compiler-reference.md
Outdated
|
|
||
| * `disable`: disables the unused return value checker. (Default) | ||
| * `check`: reports warnings for ignored results from marked functions. | ||
| * `full`: reports warnings for ignored results from all functions in your project. |
There was a problem hiding this comment.
| * `full`: reports warnings for ignored results from all functions in your project. | |
| * `full`: marks your code as non-ignorable by default. Warnings for your code will be reported in your project and any projects that depend on it with the checker enabled. |
There was a problem hiding this comment.
Nice — good point here 🤔 I'm thinking do you think we could rephrase it slightly like so?
I suggest removing future tense to avoid confusion — future tense is always a bit difficult to work with, because it can trigger questions like "when?" if the warning doesn't appear for some reason — the user might think they have to wait for it.
I want to avoid non-ignorable term here if possible. I feel like it makes it a bit harder to follow compared to the previous one that way — if you think it's a must have let's use it
full: treats all functions in your project as marked and reports warnings for ignored results in your project and in dependent projects with the checker enabled.
Or perhaps:
full: treats all functions in your project as marked by default. Reports warnings for ignored results in your project and in dependent projects with the checker enabled.
There was a problem hiding this comment.
I have a question here 🤔 Wouldn't the same apply for the check mode? So should we highlight this specific behavior for only the full mode or for check as well?
and any projects that depend on it with the checker enabled.
There was a problem hiding this comment.
full: treats all functions in your project as marked by default. Reports warnings for ignored results in your project and in dependent projects with the checker enabled.
This one is better
Wouldn't the same apply for the check mode?
Hm, that is true. In check mode only explicitly marked with @MustUseReturnValues declarations will have warnings in dependent projects, and in full mode all functions are treated as marked.
I don't know how to better communicate this, maybe you have some ideas?
There was a problem hiding this comment.
hmm 🤔
we could add a clarifying note below (again to make it pop a bit)
It has the following modes:
disabledisables the unused return value checker. (Default)checkreports warnings for ignored results from marked functions.fulltreats all functions in your project as marked and reports warnings for ignored results.
When you set
-Xreturn-value-checkertocheckorfull, the checker also reports ignored results from marked functions in library dependencies.
{style="note"}
There was a problem hiding this comment.
Maybe make some accent on distribution in the note?
All marked functions will be distributed as such and will be reported if checker is enabled in your dependent projects
Otherwise, it is not clear what in library dependencies. refers to. My dependencies or my dependants?
| * Checked in a condition such as `if`, `when`, or `while`. | ||
| * Used as the last statement of a lambda. | ||
|
|
||
| > The checker doesn't report ignored results from increment operations such as `++` and `--`. |
There was a problem hiding this comment.
This note seems weirdly specific. We have a bunch of other exclusions as well; maybe it is better to just give the link to Ignorable Expressions section of KEEP with the text like "See full definitions on what is reported and what is not here"?
There was a problem hiding this comment.
Re-reading that part — what do you think about making it exhaustive instead? 🤔
I think adding a link to the KEEP is a good idea regardless, but perhaps we can structure it in a way to remove the need for the user to look at two separate pages for this information.
So what do you think about adding this to it?
The checker doesn't report ignored results for increment operations like
++and--,
or for boolean shortcuts where the right-hand side exits the current function, for examplecondition || return.
{style="note"}
We already covered Unit or Nothing before the list (I also added Nothing? to it)
The only other exception seems to be functions marked with @IgnorableReturnValue but that's covered in a separate section.
There was a problem hiding this comment.
The checker doesn't report ignored results for increment operations like ++ and --,
or for boolean shortcuts where the right-hand side exits the current function, for example condition || return.
I'm OK with that, but maybe let's remove {style="note"} then? With it, it feels like it is a vital piece of information, while in reality, this is a pair of corner cases.
| fun someFunction(): Int = ... | ||
| ``` | ||
|
|
||
| To apply the checker to your entire project, set the `-Xreturn-value-checker=check` compiler option to `full`. |
There was a problem hiding this comment.
| To apply the checker to your entire project, set the `-Xreturn-value-checker=check` compiler option to `full`. | |
| To apply the checker to your entire project, set the `-Xreturn-value-checker` compiler option to `full`. |
Can we highlight this paragraph somehow? Using full mode should be the preferrable choice for users instead of @MustUseReturnValues
There was a problem hiding this comment.
🤔 We could put it as a note at the beginning with some reasoning that might give the idea why this is a good idea:
You can apply the checker to your entire project by setting the
-Xreturn-value-checkercompiler option tofull.
With this option, you don't have to annotate your code with@MustUseReturnValues, and the checker also reports warnings in dependent projects that have the checker enabled.
Alternatively at the bottom as a note/ as a paragraph — but with more text it would pop more.
What do you think?
(we can drop this part "and the checker also reports warnings in dependent projects that have the checker enabled" if it applies to check as well
There was a problem hiding this comment.
ou can apply the checker to your entire project by setting the -Xreturn-value-checker compiler option to full.
With this option, you don't have to annotate your code with @MustUseReturnValues.
I like this version. Can we also add style=note to it so it will stand out?
Maybe it's better to place it right after ##Mark functions to check ignored results, but I'm not sure
There was a problem hiding this comment.
Can we also add style=note to it so it will stand out?
👍 of course
Maybe it's better to place it right after ##Mark functions to check ignored results, but I'm not sure
Yea 🤔 I'm also leaning toward leaving it at the bottom. 👍
daniCsorbaJB
force-pushed
the
unused-return-value-checker-doc
branch
from
290d5d2 to
6716c47
Compare
|
|
||
| Configure how the compiler [reports ignored results](unused-return-value-checker.md): | ||
|
|
||
| * `disable`: disables the unused return value checker. (Default) |
There was a problem hiding this comment.
| * `disable`: disables the unused return value checker. (Default) | |
| * `disable`: disables the unused return value checker (default). |
|
|
||
| The unused return value checker allows you to detect _ignored results_. | ||
| These are values returned from expressions that produce something other than | ||
| `Unit`, `Nothing`, or `Nothing?` and aren't: |
There was a problem hiding this comment.
| `Unit`, `Nothing`, or `Nothing?` and aren't: | |
| `Unit`, `Nothing`, or `Nothing?`. On the other hand, ignored results aren't: |
There was a problem hiding this comment.
Why 'On the other hand'? Does it have same semantics?
| } | ||
| ``` | ||
|
|
||
| In this example, a string is created but never used, so the checker reports it as an ignored result. |
There was a problem hiding this comment.
What about bringing this explanation to the leading line, so that it has context from the beginning.
Here's an example of a string that is created but never used...
| if (name.isBlank()) return "Hello, anonymous user!" | ||
| if (!name.contains(' ')) { | ||
| // The checker reports a warning that this result is ignored: | ||
| // Unused return value of 'plus'. |
There was a problem hiding this comment.
Should we use string to clarify it is the report?
// "Unused return value of 'plus'."
|
|
||
| It has the following modes: | ||
|
|
||
| * `disable` disables the unused return value checker. (Default) |
There was a problem hiding this comment.
| * `disable` disables the unused return value checker. (Default) | |
| * `disable` disables the unused return value checker (default). |
| ``` | ||
|
|
||
| You can suppress a warning without annotating the function itself. | ||
| To do this, assign the result to a special unnamed variable with an underscore syntax (`_`): |
There was a problem hiding this comment.
What about adding in the example the full (val _)
| When you override a function, the override inherits the reporting rules defined by the annotations on the base declaration. | ||
| This also applies when the base declaration is part of the Kotlin standard library or of other library dependencies, so the checker reports ignored results for overrides of functions like `Any.hashCode()`. | ||
|
|
||
| Additionally, you can't override a function marked with `@IgnorableReturnValue` with one that [requires its return value to be used](#mark-functions-to-check-ignored-results). |
There was a problem hiding this comment.
by "one" do we refer to "another function". What about explicitly mention it to be clearer?
|
|
||
| * [`com.google.errorprone.annotations.CheckReturnValue`](https://errorprone.info/api/latest/com/google/errorprone/annotations/CheckReturnValue.html) | ||
| * [`edu.umd.cs.findbugs.annotations.CheckReturnValue`](https://findbugs.sourceforge.net/api/edu/umd/cs/findbugs/annotations/CheckReturnValue.html) | ||
| * [`org.jetbrains.annotations.CheckReturnValue`](https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/lang/CheckReturnValue.html) |
There was a problem hiding this comment.
This link is just the same as the next one, and the page refers to org.springframework.lang instead of jetbrains. Is it correct?
AlejandraPedroza
left a comment
AlejandraPedroza
left a comment
There was a problem hiding this comment.
Great docs! Left some suggestions for you to check.
This PR adds a new document for the unused return value checker feature.
Related ticket: KT-71188