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

consider annotation(s) for exclusive params (@OnlyOneOf, @AtMostOneOf, @AllOrNoneOf, ... ?) #47712

Open
pq opened this issue Nov 16, 2021 · 15 comments
Labels
analyzer-pkg-meta Issues related to package:meta area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@pq
Copy link
Member

pq commented Nov 16, 2021

TL;DR Use an annotation to document and (statically) enforce mutually exclusive named parameters

In Flutter there are APIs (such as Container construction) that enforce mutually exclusive named parameters (e.g., color or decoration but not both) at runtime. It would be better to provide this feedback statically.

dart-lang/linter#3063 proposes a lint for the Container case, but there are similar patterns in at least: CupertinoSearchTextField, Ink, TextStyle, and AnimatedContainer. An annotation could support these patterns robustly for Flutter and ecosystem packages.


Proposed Solution: an @OnlyOneOf annotation that allows API designers to identify groups of parameters that are exclusive.

In package:meta:

/// Used to annotate named parameters `p1..pn` in a method or function `f`.
/// Indicates that an invocation of `f` can contain at most one argument
/// corresponding to one of the named parameters `p1..pn`.
///
/// Optionally annotated parameters can be assigned to named groups which
/// allow for more than one mutually exclusive set of parameters in one
/// method or function `f`.  These group ids can also be used for
/// documentation purposes.
class OnlyOneOf {
  /// An id that defines a set of exclusive parameters.
  final String groupId;

  /// Initialize a newly created instance with the given [groupId].
  const OnlyOneOf([this.groupId = '']);
}

In flutter/lib/src/widgets/container.dart:

  Container({
    Key key,
    this.alignment,
    this.padding,
    @OnlyOneOf(‘decorationGroup’)
    this.color,
    @OnlyOneOf(‘decorationGroup’)
    this.decoration,
    this.foregroundDecoration,
    double width,
    double height,
    BoxConstraints constraints,
    this.margin,
    this.transform,
    this.child,
    this.clipBehavior = Clip.none,
  }) ...

In practice, a container created with a color and decoration would produce an analysis error like this:

Widget bodyWidget() {
  return Container(
    color: Colors.yellow,
    decoration: BoxDecoration( // ERROR MSG: “Cannot provide both a color and decoration”
      border: Border.all(color: Colors.black),
    ),
    child: Text("Flutter"),
  );
}

To improve error messages further, we might consider adding an optional message to the annotation so that we could report something identical to the runtime exception (e.g., "The color argument is just a shorthand for 'decoration: new BoxDecoration(color: color)'.").


/fyi @bwilkerson @InMatrix @leafpetersen @munificent @goderbauer who were in on an earlier conversation about annotating exclusive params vs. a language solution like overloading.

/fyi also @natebosch @jakemac53 @lrhn @eernstg for more language team perspective

@pq pq added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-linter Issues with the analyzer's support for the linter package pkg-meta labels Nov 16, 2021
@bwilkerson
Copy link
Member

I believe that the proposed annotation would be sufficient for Container, AnimatedContainer, Ink, and TextStyle, because there is one pair of mutually exclusive parameters ([color, decoration]). But for CupertinoSearchTextField there are two pairs of mutually exclusive parameters ([backgroundColor, decoration] and [borderRadius, decoration]).

I suppose that we could allow the annotation to be applied to the same parameter multiple times if it's mutually exclusive with more than one other parameter, something like:

  @OnlyOneOf(‘backgroundColorGroup’)
  Color? backgroundColor,

  @OnlyOneOf(‘borderRadiusGroup’)
  int? borderRadius,

  @OnlyOneOf(‘backgroundColorGroup’)
  @OnlyOneOf(‘borderRadiusGroup’)
  Decoration? decoration,

If there are other constraints that package authors want to be able to express, would we introduce similar annotations (such as ExactlyOneOf or AllOrNoneOf)?

Should such an annotation be on the parameters, or would it be better to have them on the function:

@AtMostOneOf([‘backgroundColor’, 'decoration'])
@AtMostOneOf([‘borderRadius’, 'decoration'])
CupertinoSearchTextField(...);

That might require less repetition. It's also less error prone in some ways (you can't mistype the group name) and more error prone in others (you could forget to remove a parameter from a list while deleting it).

Either way we probably want to have some consistency checks. For example, there needs to be more than one parameter in any given group / list.

@goderbauer
Copy link
Contributor

Would/could the lint also warn if it only finds one @OnlyOneOf(‘decorationGroup’) to remind you that it is no longer needed (because the other parameters where removed) and to protect against spelling errors?

@bwilkerson
Copy link
Member

Yes. That's exactly the kind of consistency check I was trying to describe.

@mraleph
Copy link
Member

mraleph commented Nov 17, 2021

Is it possible for linter to just look into the body of the constructor and partially evaluate assert's that are listed there against the provided (abstract) values? That would allow it to issue a lint when it discovers an assert that is guaranteed to fail in runtime. Seems like potentially a more generic feature.

@srawlins srawlins added the P3 A lower priority bug or feature request label Nov 18, 2021
@scheglov
Copy link
Contributor

Another way to restrict which pieces of data can go with other is to use separate sets of parameters.

@AtMostOneOf([‘backgroundColor’, 'decoration'])
@AtMostOneOf([‘borderRadius’, 'decoration'])
CupertinoSearchTextField(...);

could be

class DecorationsForCupertinoSearchTextField {
  DecorationsForCupertinoSearchTextField.v1({this.backgroundColor, this.borderRadius});
  DecorationsForCupertinoSearchTextField.v2({required this.decoration});
}
CupertinoSearchTextField({DecorationsForCupertinoSearchTextField decorations, ...});

Which of course has drawbacks of requiring extra class as a wrapper, and actual names for v1 and v2.

Using annotations to mark parameters is interesting, although seems outside of the language, with name in strings instead of actual language constructs.

@pq
Copy link
Member Author

pq commented Dec 8, 2021

Using annotations to mark parameters is interesting, although seems outside of the language, with name in strings instead of actual language constructs.

Yeah. Support from the language would be better but (at least) overloading is a big ask. In the meantime we have a bunch of APIs like Container that enforce these contracts at runtime and it'd be great to communicate these errors earlier somehow.

Defining a bunch of named constructors is definitely one thing we could do with the language currently but it'd be pretty disruptive for existing APIs and has I think some potential usability/discoverability issues.

Is it possible for linter to just look into the body of the constructor and partially evaluate assert's that are listed there against the provided (abstract) values?

This is a really interesting idea. I haven't thought about it too hard but a naive solution might be a little costly. (Caching could help here.)

@pq pq changed the title consider an OnlyOneOf annotation for exclusive params consider annotation(s) for exclusive params (@OnlyOneOf, @AtMostOneOf, @AllOrNoneOf, ... ?) Dec 8, 2021
@bwilkerson
Copy link
Member

Is it possible for linter to just look into the body of the constructor and partially evaluate assert's that are listed there against the provided (abstract) values?

I agree, that's an interesting idea, but we only have that information for asserts that are in the initializer list and in a constructor marked as const. And we could only evaluate the asserts if the values of the arguments to the constructor invocation were all compile-time constants. That might be sufficient for most of the cases in Flutter, but wouldn't be as general as an annotation-based solution.

@bwilkerson bwilkerson added analyzer-pkg-meta Issues related to package:meta and removed analyzer-linter Issues with the analyzer's support for the linter package pkg-meta labels Jul 18, 2022
@srawlins
Copy link
Member

srawlins commented Oct 4, 2022

Is it possible for linter to just look into the body of the constructor and partially evaluate assert's that are listed there against the provided (abstract) values?

I agree, that's an interesting idea, but we only have that information for asserts that are in the initializer list and in a constructor marked as const.

I don't understand this. I think @mraleph was not suggesting that we actually perform const evaluation, but that we "pretend" to evaluate certain asserts in functions (and constructor initializers) with a small set of supported operations. Something like:

void f(int? a, int? b) {
  assert(a == null || b == null); // At least one argument must be null.
  assert(a != null || b != null); // At least one argument must be non-null.
}

I think if we only supported: == null, != null (and maybe is! T), SimpleIdentifiers, and || and &&, and no other operations, we'd have a fairly robust check.

@srawlins
Copy link
Member

srawlins commented Oct 4, 2022

What about helper functions that called these protected functions? E.g.

Container helper(Decoration? decoration) {
  var color = decoration == null ? Colors.yellow : null;
  return Container(
    color: color,
    decoration: decoration, // ERROR MSG: “Cannot provide both a color and decoration”
    child: Text("Flutter"),
  );
}

This helper is legal; it only passes one non-null argument for color and decoration, but it would (I posit) be too complicated to track the exclusive nullabilities here.

@Fernandomr88
Copy link

First of all, glad to find this discussion/proposal here.
I have stumbled with this situation past week where a friend of mine started to learn his way into Flutter and he kept making mistakes where he would use color AND decoration at the same time over and over again, only to find out at runtime. So he asked me if there were a linter or something to help him prevent this, so here I am.

Coming from dart-lang/linter#4805 (thanks @pq)

I was reading and came to my mind that (and please correct me if I'm wrong) there could be parameters like color and/or backgroundColor that could exist mutually but never if decoration exists. So there could be parameters "hierarchy like" situations where one (or more parameters) are "overridden" by larger groups.

What would that annotation be like?

@srawlins
Copy link
Member

Maybe something like @AtMostOneOf(#decoration, [#color, #backgroundColor]) or @AtMostOneOf(#decoration, @AnyCombinationOf(#color, #backgroundColor)). I think it could be done, and motivating examples like this are extremely helpful 😁

@bwilkerson
Copy link
Member

We do need to be careful about not making this more complex than it needs to be.

@munificent
Copy link
Member

munificent commented Nov 29, 2023

Personally, I consider it a tenet of good API design that parameters should be as orthogonal as possible. If you have a single function that contains parameters that are mutually exclusive, then what you really have is multiple separate functions masquerading as the same one. Whenever possible, I think it's better to split those into separate functions. Overloading would be nice, but even in the absence of that, you can give the functions separate names. That also works for constructors since Dart has named constructors.

Likewise, if you have a set of optional parameters that should all be passed together or not at all, that's a single parameter masquerading as multiple. Like @scheglov suggests, you're often better bundling that data together into a meaningful object which can be passed as a single parameter. With the new record syntax in Dart 3.0, you can do that without having to give it a name. A single nullable record parameter, instead of a series of separate nullable parameters means that you either have all the data or none of it, which is exactly what you're trying to represent.

@Fernandomr88
Copy link

Personally, I consider it a tenet of good API design that parameters should be as orthogonal as possible. If you have a single function that contains parameters that are mutually exclusive, then what you really have is multiple separate functions masquerading as the same one. Whenever possible, I think it's better to split those into separate functions. Overloading would be nice, but even in the absence of that, you can give the functions separate names. That also works for constructors since Dart has named constructors.

Likewise, if you have a set of optional parameters that should all be passed together or not at all, that's a single parameters masquerading as multiple. Like @scheglov suggests, you're often better bundling that data together into a meaningful object which can be passed as a single parameter. With the new record syntax in Dart 3.0, you can do that without having to give it a name. A single nullable record parameter, instead of a series of separate nullable parameters means that you either have all the data or none of it, which is exactly what you're trying to represent.

I like the idea of overloading it, Specially thinking about migrating from current widgets to new ones.

Something like

Container.decoration(
  decoration: BoxDecoration(
    gradient: LinearGradient(colors: [Colors.red, Colors.blue]),
    borderRadius: BorderRadius.circular(10),
  ),
  child: Text('Decorated Container'),
);

Etc.

@lrhn
Copy link
Member

lrhn commented Nov 29, 2023

What @munificent says.

The one var not directly covered by multiple methods instead of mutually exclusive parameters, or record arguments instead of both-or-neither parameters, is the "only pass B if passing A" case, like "only pass onTimeout if also passing timeoutDuration, otherwise it's ignored".

It's probably still something that should be multiple methods, one without any timeout, and one with a required timeout duration and an optional onTimeout.

(But then, everything can be solved using multiple members with different names, since everything here is something that languages with overloading just do using multiple members with the same name Sometimes just requiring an awful lot of members to cover the entire combinatorial space if there are several unrelated optional parameters. I guess that's why, fx, C# has both overloading and optional arguments.)

@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-pkg-meta Issues related to package:meta area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

9 participants