Conditional @useResult

[rofrankel]

The new @useResult is great! I am currently using it on generated extension methods. But there's a proposal for these generated extension methods to take an optional argument, which - when present - obviates the need for @useResult.

Example (based on a real example, but stripped of project-specific details, hopefully not in a confusing or distracting way):

@useResult
int hasCount() {
  return value.count;
}

...would become:

int hasCount([int? expected]) {
  if (expected != null) {
    if (value.count != expected) throw '${value.count} is not equal to $expected';
  }

  return value.count;
}

I want the behavior of @useResult only when expected is not provided (because when it is provided, the value of _count is "used" inside the method). Being able to use @useResult is important because the use case is a testing library, and failing to use the result would allow mistakes that could result in tests passing incorrectly.

A colleague had a couple ideas for possible "syntax", but we're open to anything:

@useResult
int hasCount([@usesResult int? expected]) {
 ...
}

OR:

@useResult(unless: 'expected')
int hasCount([int? expected]) {
  ...
}

[rofrankel]

@pq

[bwilkerson]

I believe you've had at least one conversation off-line, and I might be missing some context, but I thought I'd add my thoughts anyway.

My primary concern with the proposed change is the possibility that this could, over time, result in a design that's difficult to use. The reason I say that is because we're proposing adding support for an exception to the rule without understanding what additional exceptions we might need to support. Without knowing the set of exceptions we need to support we might end up with similar but distinct mechanisms for each exception rather than a more general way of handling exceptions.

Of course, it might not be possible to enumerate the set of plausible exceptions at this point, and there's also a risk of defining something far more general than we would ever needs, which has its own set of problems. I'm not proposing that we do nothing because we have imperfect knowledge, only wanting to make sure that everyone is aware of the design considerations we should keep in mind.

As for the two proposed options, the unless argument has a couple of advantages:

• it's general enough to be used for lots of other kinds of suggestions, and
• it keeps the logic about what's being checked in one place rather than having it spread across multiple annotations.

It also has a couple of disadvantages:

• we're effectively defining a small language in a string, which will then need to be parsed and interpreted, and
• that language isn't checked by the Dart compiler so we'd need to add some additional testing (such as whether the mentioned name matches the name of a declared parameter).

I'm undecided as to whether the advantages outweigh the disadvantages, and I'm curious to hear your thoughts, including any additional advantages or disadvantages you've thought of.

I'm also interested to know whether you've considered solving the problem by generating two methods, one with zero parameters that is annotated with useResult and one with one parameter that isn't annotated:

@useResult
int hasCount() {
  ...
}

int hasCountMatching(int expected) {
  ...
}

Not only does this remove the need to express an exception, but it also allows the parameter to be non-nullable.

[rofrankel]

Thanks, Brian. Yep, Phil and I had spoken about this previously, and we were actually discussing it again today just as you wrote your reply. Phil shared some of these concerns with me, and regarding packing a DSL into a String field (to which I share your aversion) or just wanting to poke some totally different hole later, I suggested that perhaps we could use more specific named arguments instead. For example:

@useResult(unlessArgumentProvided: 'expected')

Of course, you might worry that you will eventually want to specify arbitrary combinations of arguments - but A) YAGNI says not to worry too much about this kind of thing; and B) perhaps that could just be another named argument (unlessArgumentsProvided?) of some structured type (rather than just String).

Phil seemed to think it wouldn't be a big deal for the analyzer to make sure that the String matches the name of a parameter - I don't have enough familiarity with the analyzer to have an opinion either way.

The problems with having multiple methods include:

• Naming is hard
• Risk of name collisions (what if we also generated hasCountMatching for a field named countMatching?)
• The goal is concision and improved devx relative to writing out e.g. hasCount().equals(expected). I'm not sure hasCountMatching (or hasCountEqualTo, etc.) would achieve these goals enough to justify the extra API surface.

[pq]

To chime in and reiterate what Richard says above, I share the concerns generally (and think we're all on the same page). Having spent some time looking at the motivating examples, I don't see that hasCountMatching or similar really checks the boxes (e.g., conciseness) and am sort of leaning towards the optional named param approach (e.g., unlessArgumentProvided: 'expected'). It would invite extra checks but they should be straightforward. It's possible that we might want more unless* configurations (unlessInTests? or maybe some of the ignore contexts described in errorprone's @CheckReturnValue docs?) but I'd also be surprised if we didn't.

[bwilkerson]

tl;dr If you're both convinced that we want to add parameters to the constructor for UseResult, and assuming that they're named and specific, then I'm OK with that.

... perhaps we could use more specific named arguments instead.

If we're going to add parameters, then yes, we'll want them to be named and optional (so we can add more later) and specific (to avoid name clashes).

... YAGNI says not to worry too much about this kind of thing ...

I agree with the general principle when you control all the clients of the code, such as when you're building a proprietary application or even when working in a mono-repo. Maybe I'm wrong, but I disagree with it when you're designing an API that will be used by code you don't control. In those cases the cost of making significant changes can make it impractical to change or even extend the API. Hence, I think that additions like this need to be carefully considered.

But to be clear, I'm not suggesting that we allow such concerns to prevent us from making progress. I'm just explaining why I thought about it at all.

Phil seemed to think it wouldn't be a big deal for the analyzer to make sure that the String matches the name of a parameter

He's right, it isn't. I didn't intend to imply that the problems I was mentioning were insurmountable, or even hard, I was only trying to be complete in my analysis.

Risk of name collisions (what if we also generated hasCountMatching for a field named countMatching?)

Yeah, if this weren't a fluent API we'd have more flexibility in where the modifier was placed. The only way to prevent name collisions is for the modifiers to come before the name of the field. Following that restriction would give us something like hasAndMatchesCount, but that isn't quite as readable and isn't concise.

But assuming that generating two methods is off the table, I'll suggest that for the sake of future extensibility you might want to consider using optional named parameters rather than positional parameters.

int hasCount({int? equalTo}) { ... }

While it's not as concise (hasCount(equalTo: 3) rather than hasCount(3)), it does mean that you could generate additional forms in the future, such as

int hasCount({int? equalTo, int? lessThan}) { ... }

But maybe the equality case is the only case worth optimizing, in which case not requiring the name is probably a win. I don't know how the API is used in practice.

[rofrankel]

Thanks for thinking through this with us!

But to be clear, I'm not suggesting that we allow such concerns to prevent us from making progress. I'm just explaining why I thought about it at all.

OK, sure, we're on the same page then. I agree it's worth considering carefully, so long as we can still make progress.

But maybe the equality case is the only case worth optimizing, in which case not requiring the name is probably a win. I don't know how the API is used in practice.

Yeah, this is pretty much the case.

[pq]

OK, ready to bike shed APIs.

Because this.reason is an optional positional argument, the most obvious solution (UseResult({this.reason, this.usesResult})) would be breaking and so not ideal.

My proposal (up in https://dart-review.googlesource.com/c/sdk/+/208721), is something like:

 const UseResult.unless({this.reason = '', required this.parameterDefined});

which, in practice, would look like:

@UseResult.unless(parameterDefined: 'count')
Matcher hasCount([int? count]) => Matcher();

[rofrankel]

That definitely seems like it'd solve my problem. :) If the API details work for the core Dart team, then they work for me. Thanks!

[pq]

Good deal. Thanks @rofrankel!

This will land in a few stages, listed below if you want to follow along 😄

• add @UseResult.unless to package:meta (https://dart-review.googlesource.com/c/sdk/+/208721): 9491caf
  • changelog update: 1eb601b
• add checks to the use result verifier to notice passed "using" params (https://dart-review.googlesource.com/c/sdk/+/208822) : b820538
• add new target verification to ensure annotated members have a matching parameterDefined parameter (https://dart-review.googlesource.com/c/sdk/+/208862): 9fced4c

[pq]

Closing out as implemented. Feel free to re-open if anything comes up. 👍