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

@MayIgnoreResult / @MustUseResult: mark whether a non-void function is okay to call as if void #200

Open
kevinb9n opened this issue Jun 30, 2021 · 9 comments
Labels
design An issue that is resolved by making a decision, about whether and how something should work. needs-decision-post-1.0 Issues needing a finalized decision that they are post 1.0 new-feature Adding something entirely new for users, not improving what's there
Milestone

Comments

@kevinb9n
Copy link
Collaborator

kevinb9n commented Jun 30, 2021

(The below applies to constructors as well as methods, and includes usages of either via member references.)

Background

Each call to a non-void method produces a result value, which the call either uses or ignores. It's considered to use the result if and only if changing the method to void would break the call site.

Rare methods like Set.add are legitimately useful when called in either fashion. These are "may-ignore-result" methods. For want of a pithy adjective we might call them "voidable" methods, short for "okay-to-treat-as-void".

The problem

May-ignore-result methods are a comparatively rare special case, yet Java treats all methods this way. This allows many bugs to happen. If we can annotate the methods appropriately we can catch those bugs.

Numerous tools support this check, using annotations like @CheckReturnValue (from Error Prone or SpotBugs) or @CheckResult (from Android). Since may-ignore-result has been Java's default treatment, they have you mark all other methods with the annotation. Like we've found with nullness, a better approach is to let you "flip the default" for a class/package/module, then annotate only the may-ignore-result special cases.

(Kudos to Swift for getting this right, with its @discardableResult annotation; surely others too.)

Proposal (sketch)

This calls for:

  • a context-annotation to express "methods in this context expect callers to use their result (unless marked otherwise)".
  • a method annotation to express "okay to ignore the result".
  • ternary semantics (must use, may ignore, unknown), like with nullness

Note that tools would be at liberty to use this information as they see fit. For example, if a tool recognizes that it's analyzing test code it might exempt the code from this check.

One key semantic question is whether may-ignore-resultness should automatically be inherited from a supertype, or whether it must be reasserted on overrides. The latter is obviously convenient but there are a few points to consider on each side.

Kotlin status

Kotlin is looking at doing something like this, but of course wants it to work seamlessly back and forth with Java, so they are happy to work with us on standardizing these semantics.

Diagram

I don't know why I made this but I did so here it is. It uses the awkward term "voidable" for "may ignore result".

voidable (optionally treat as void) methods

@kevinb9n
Copy link
Collaborator Author

(Prioritization: I'd feel great about this being the next thing tackled beyond nullness, partly because Kotlin's already talking about it, but also because we have a mountain of evidence that it catches a ton of real bugs.)

@kevinb9n
Copy link
Collaborator Author

kevinb9n commented Jul 1, 2021

(Tangent not to follow: for a few may-ignore-result methods like List.add, callers should probably ignore the result! could say this is the third of four partitions if we wanted. But I think the three are enough because this is an orthogonal and smaller concern to the rest of what we're talking about, and is actually just a special case of "expression with statically known result can be simplified".)

@jspecify jspecify deleted a comment from cpovirk Jul 1, 2021
@kevinb9n kevinb9n changed the title Specify whether a non-void function may be called *as if* it is void Specify whether a non-void function can be meaningfully called *as if* void Jul 1, 2021
@kevinb9n kevinb9n added this to the Post-1.0 milestone Aug 18, 2021
@yogurtearl
Copy link

Detekt has a related rule for this: https://detekt.github.io/detekt/potential-bugs.html#ignoredreturnvalue

@kevinb9n kevinb9n changed the title Specify whether a non-void function can be meaningfully called *as if* void Specify whether a non-void function can be meaningfully called *as if* void ("can ignore return value" vs "check return value") Dec 17, 2021
@kevinb9n kevinb9n changed the title Specify whether a non-void function can be meaningfully called *as if* void ("can ignore return value" vs "check return value") Annotation for a non-void function that can be meaningfully called *as if* void ("may ignore return value") Jan 27, 2022
@kevinb9n
Copy link
Collaborator Author

kevinb9n commented Jan 27, 2022

Detekt has a related rule for this: https://detekt.github.io/detekt/potential-bugs.html#ignoredreturnvalue

@yogurtearl sorry for the slow reaction, but that's awesome. I hope you'll be interested in an ongoing conversation about bringing these annotations into JSpecify and then getting them supported natively by kotlinc.

@kevinb9n kevinb9n changed the title Annotation for a non-void function that can be meaningfully called *as if* void ("may ignore return value") A non-void function that's okay to call *as if* void (@MayIgnoreValue vs. @MustUseValue) Mar 25, 2022
@kevinb9n
Copy link
Collaborator Author

kevinb9n commented Apr 21, 2022

Some nomenclature alternatives

  • must use value / may ignore value
  • caller must use value / caller may ignore value
  • value must be used / value may be ignored
  • mandatory value / ignorable value
  • just "ignorable" (recommend place before the return type)
  • (change "value" to "result" in any of the above)
  • (change "ignore" to "discard" in any of the above)
  • result use expected / result use optional
  • may not call as void / may call as void
  • not voidable / voidable

(I resisted using the term "result" for this, because I thought "value" would serve. I'm now convinced "result" is better. JLS uses "result" only for a variety of other different purposes, but it doesn't tie our hands.)

I don't consider the verb "check" to be a viable option. What does it mean to "check" a value? It doesn't sound like merely assigning it to a variable or handing it off to another method would be enough.

We should also consider how we would want to name a "caller is (not) responsible for closing the returned AutoCloseable instance" annotation and have appropriately consistent answers.

@kevinb9n kevinb9n modified the milestones: Post-1.0, 1.0 Sep 20, 2022
@kevinb9n kevinb9n changed the title A non-void function that's okay to call *as if* void (@MayIgnoreValue vs. @MustUseValue) @MayIgnoreResult / @MustUseResult: mark whether a non-void function is okay to call as if void Nov 22, 2022
@kevinb9n
Copy link
Collaborator Author

@cortinico @BraisGabin @schalkms @chao2zhang We want to standardize how this information is captured for APIs in any JVM language. Any interest in being involved? Kotlin hopes to support it natively, which would eventually make it an unnecessary check for detekt. But the annotations will still matter for language interop, and your involvement could help us get this right.

@cortinico
Copy link

But the annotations will still matter for language interop, and your involvement could help us get this right.

I believe we can extend the IgnoredReturnValue rule to account for the extra logic (may/must). Beyond this, were you looking for any specific support?

@kevinb9n
Copy link
Collaborator Author

Oh, I suppose mainly that when we come out with a specific proposal we'd love to get your feedback on it -- so I suppose the question is a bit premature. We welcome any thoughts you might have whenever you have them.

@kevinb9n kevinb9n added design An issue that is resolved by making a decision, about whether and how something should work. new-feature Adding something entirely new for users, not improving what's there and removed design An issue that is resolved by making a decision, about whether and how something should work. labels Nov 30, 2022
@kevinb9n
Copy link
Collaborator Author

kevinb9n commented Sep 20, 2023

As mentioned above, the Kotlin team want to transition to having must-use-result as the default, and may-ignore-result as a language feature. Annotations meaning the same things would seem unnecessary, and in the very long run they should be, but they would be very helpful for a while.

For one thing, when depending on classes compiled from other languages it can recognize these annotations. And when compiling classes to be called by other languages it can embed them.

And even squarely inside "Kotlin land" this should help. Suppose the language feature drops in Kotlin version N. Some published library wishes to support both Kotlin N-1 and Kotlin N (a highly normal thing for a library to want to do). That means it wants a way to provide the information, but it can't use the normal approach for providing it. Annotations to the rescue? Though perhaps Kotlin has a different strategy for dealing with this problem generally (Java doesn't).

If Kotlin moves forward, then either

  • JSpecify had already gotten busy providing these annotations and Kotlin only has to recognize them, or
  • Kotlin will recognize any of a long list of annotations, with or without respecting the minor variations in what they are supposed to mean, and may likely enough provide another one of its own

@kevinb9n kevinb9n added the design An issue that is resolved by making a decision, about whether and how something should work. label Mar 13, 2024
@kevinb9n kevinb9n modified the milestones: 1.0, Post-1.0 Mar 27, 2024
@netdpb netdpb added the needs-decision-post-1.0 Issues needing a finalized decision that they are post 1.0 label Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design An issue that is resolved by making a decision, about whether and how something should work. needs-decision-post-1.0 Issues needing a finalized decision that they are post 1.0 new-feature Adding something entirely new for users, not improving what's there
Projects
None yet
Development

No branches or pull requests

4 participants