Annotation + a linter requiring using a return value

[RobertSzymacha]

Describe the rule you'd like to see implemented

In Java there is an annotation for a linter https://errorprone.info/bugpattern/CheckReturnValue which warns engineers that the specific call is not useful if the value is ignored. This is used in cases where:

• operation does not have side effects (e.g. creates a new instance of an immutable object (e.g. when using built_value), or just returns object's state)
• not using return value is a clear bug due to above, especially in context of .. operator (see examples below)

Examples

It is useful for instance when:

• one calls list.length w/o using the value
• one calls builtValue.rebuild((b) => b..someField = 'value') w/o using the value

In Dart it could be especially useful to catch following bug:

var x = SomeBuiltValue();
var y = x..rebuild((b) => b.field = 'new value')

In that example, value of rebuild() is being ignored due to using .. operator (which is a bug), and one should use instead

var y = x.rebuild((b) => b.field = 'new value')

If the rebuild() method was annotated with CheckReturnValue, linter should display a warning/error, that 'Value returned by rebuild() is not used'.

Similarly, following code snipped is a clear bug as well, that would be easily prevented with that linter:

var x = list..length;

Typing .. is very close to typing just ., Dart would benefit from that linter even more than Java

[eernstg]

We do have unawaited_futures, which is aimed at flagging exactly the situation where an object (in this case: a future) is obtained from some evaluation, and then it's discarded. That's frequently an error-prone situation with a future, but business logic could justify a similar discipline for any type of object, or for any function or instance member.

Maybe a generalization of unawaited_futures could be a @dontDiscard annotation on classes and functions (including getters, and including instance members), and possibly on function types, and the effect would be that it is linted whenever we discard the value of an expression of such a type, and whenever we discard the value returned by such a function.

@dontDiscard
class C {}

@dontDiscard
int foo() => 42;

@dontDiscard
typedef F = int Function();

main() {
  C(); // Lint: Don't discard an instance of `C`.
  var c = C(); // OK, and backed up by hint: unused local variable.
  foo(); // Lint: Don't discard the returned value from `foo`.
  F f = foo;
  f(); // Lint: Don't discard the returned value from a function of type `F`.

  // And here's an interesting case:
  void ignored;
  ignored.toString(); // Error, you shouldn't use a value of type `void`.
  ignored = 42; // OK; we could also execute `42;` as a statement.
  ignored = f(); // Lint: Don't discard..
}

[pq]

Thanks for the suggestion @RobertSzymacha. I think something along the lines of errorprone's @CheckReturnValue annotation or, as @eernstg suggests @dontDiscard is a great idea.

[srawlins]

I thin you're describing the unnecessary_statements rule. See the tests for examples.

[eernstg]

Right, I can see a certain amount of overlap, but the unnecessary_statements lint won't cover exactly the same cases as a @dontDiscard based lint.

The former needs to characterize certain elements (like operator +) as "likely pure", and recognize constructs like b ? e1 : e2 as "result oriented" (if you don't need the result, use an if statement). With that, it's fair to flag the statement a + b; because it is likely to compute a result which is discarded, and otherwise have no effect. So that looks like a bug.

But the @dontDiscard based lint would flag constructs where a distinguished kind of object is discarded. For instance, if we add @dontDiscard to the Future class then every location where a Future<T> for any T is discarded would be flagged. (That's the reason why I mentioned the overlap with unawaited_futures). Similarly, any other class MyClass could have the annotation @dontDiscard, and then we'd get a lint whenever the result from an expression of type MyClass or a subtype thereof is discarded. Or we could put @dontDiscard on specific functions, and then we'd get a lint whenever it's returned value is discarded (which is useful if it returns int or other common things, and we don't want to have lints on every location where an int is discarded).

[camsteffen]

Android has @CheckResult. I like the positive name instead of "don't"

Would this belong in the meta package?

[bwilkerson]

Yes, I would expect the annotation to be added to the meta package.

[pq]

WRT naming, +1 to @CheckResult.

[pq]

Annotation WIP: https://dart-review.googlesource.com/c/sdk/+/200301

[pq]

/// Used to annotate a method, field, or getter within a class, mixin, or
/// extension, or a or top-level getter, variable or function to indicate that
/// the value obtained by invoking it should be use. A value is considered used
/// if it is assigned to a variable, passed to a function, or used as the target
/// of an invocation, or invoked (if the result is itself a function).
///
/// Tools, such as the analyzer, can provide feedback if
///
/// * the annotation is associated with anything other than a method, field or
///   getter, top-level variable, getter or function or
/// * the value obtained by a method, field, getter or top-level getter,
///   variable or function annotated with `@useResult` is not used.
const UseResult useResult = UseResult();

[pq]

Implicit Ignored Contexts

@RobertSzymacha (and others), as I work on this implementation, I'm curious where you weigh in on the various implicit "ignore contexts" described in the Java @CheckReturnValue docs. In particular it seems like we should consider some cases to allow for unused values in some test contexts.

How do people feel about the try/execute/fail/catch pattern described in the doc?

Explicit Ignored Contexts

What about the explicit pattern of suppression that involves assigning to a variable unused (or maybe _)?

[pq]

@rofrankel: would "ignored contexts" play into how you'd expect to use this annotation to support fluent programming w/ flute? Are there patterns we should be thinking about?

[fzyzcjy]

Hi, is there any updates?

[pq]

Hi @fzyzcjy. @useResult is available in package:meta and the analyzer should perform the expected verification. If you hit any snags, feel free to let us know!

Sorry btw for leaving this open. Let's close it in favor of targeted issues when they come up.

Thanks!