pkg/meta: using a function with a @required parameter as tear-off does not warn about missing required parameter

[denniskaselow]

Dart VM version: 2.1.0-dev.4.0 (Fri Sep 7 16:44:38 2018 +0200) on "windows_x64"

import 'package:meta/meta.dart';

void main() {
  // analyzer info: The parameter 'paramB' is required.
  doStuff(1);
  // nothing wrong here according to the analyzer
  // but I would expect the analyzer to inform me about the missing parameter
  [1].forEach(doStuff);
}

void doStuff(int paramA, {@required bool paramB}) {}

[matanlurey]

My guess (?) is this is WAI (working-as-intended), since @required is not a language feature, and the tools are not tracking the intention of this annotation - you see some of the discussion at dart-lang/language#15 for details.

For example, what about the following cases:

• Is @required applicable to function types void Function({@required String name})?
• Is @required inherited across extends/implements/with?

class X1 {
  void doThing({@required String name}) {}
}

class Y1 extends X1 {
  // Ok? Lint?
  void doThing({String name)} {}
}

class X2 {
  void doThing({String name}) {}
}

class X2 extends Y2 {
  // OK? Lint?
  void doThing({@required String name}) {}
}

One silly idea to move forward is to introduce a lint, i.e. avoid_tearoff_required, for the above case.

/cc @Hixie @bwilkerson @munificent

[munificent]

Yeah, this is a good example of why relying on metadata annotations for features like this doesn't give as nice of an experience as baking the feature fully into the language.

Because @required isn't part of the language, it's not part of the type system. Even if analyzer was changed to statically propagate the @required through tear-offs and function types, it's still not able to do that to the runtime type of the function, so you'll still be able to get into bad situations.

[Hixie]

What bad situations? @required is only for static analysis, it shouldn't do anything at runtime.

[matanlurey]

@Hixie:

Here is one:

import 'package:meta/meta.dart';

class A {
  void doThing() => print('doThing()');
}

class B implements A {
  void doThing({@required String name}) => print('doThing($name)');
}

void main() {
  B b = B();
  A a = b;
  // Required not linted... resulting in:
  // Runtime error: `name` is null.
  a.doThing();
}

[eernstg]

The grammar does not currently allow metadata on the parameter types in a "new style" function type like void Function({@required String name}), but I don't see any particular difficulties in adding support for that.

However, it may be somewhat tricky to support that with an analysis which is an enhanced version of the one we have currently to use this kind of information on function types. We would flag certain casts as violations of "requiredness preservation":

void foo1({int i}) {}
void foo2({@required int i}) {}

void Function({@required int i}) f1;
void Function({int i}) f2;
void Function() f3;

main() {
  f1 = foo1; // OK, requiredness can be overestimated, no problem.
  f2 = foo1; // OK, extended type unchanged.
  f3 = foo1; // OK, regular function type upcast.
  f1 = foo2; // OK, extended type unchanged.
  f2 = foo2; // Flagged as bad, forgets requiredness.
  f3 = foo2; // Flagged as bad, forgets required parameter.
}

It may be surprising that we are flagging otherwise always-ok transitions like upcasts as bad, and there may be some tricky corners around type variables:

Y foo<X extends Y, Y>(X x) => x;

main() {
  Function() f = foo(({@required int i}) => i); // Bad!
}

It is surprising that the invocation of foo is "bad", because the declaration seems safe (it is a plain upcast to return x because X extends Y).

We can't see at the call site that the execution of foo will perform an upcast from the first to the second type argument (so it will be overkill to flag all such generic function calls), and we can't see in the implementation that it is unsafe to perform that upcast (so it will be overkill to flag all such function declarations). But that upcast for this call site will be an upcast from Function({@required int i}) to Function(), and that would allow us to "forget a required parameter".

There may be an easy and elegant solution, but it hasn't dawned upon me yet. ;-)

PS: I don't think this difficulty has anything to do with being part of the language or not, there's nothing stopping the linter from knowing all the things that the type checker knows, and then some, and there's nothing (other than convention) stopping the type checker from seeing metadata.

[eernstg]

Ah, I forgot this: The relationship between A.doThing and B.doThing here is exactly one of those situations involving an upcast: It is generally considered safe to access a B using the static type A, but when some upcasts are bad (like that from void Function({@required String name}) to void Function()) we make standard OO subsumption a problem.

[denniskaselow]

I was using the meta package for the first time so I was surprised not to see a warning.

But after completely reading the dartdoc of @required I see that it actually is WAI (sorry for only reading the first paragraph before), as it explicitly states that it's only working for invocations.

Tools, such as the analyzer, can provide feedback if

• ...
• an invocation of a method or function does not include an argument corresponding to a named parameter that has this annotation.

But I am not sure, whether I would have immediately realized that no feedback will be provided by the analyzer when passing methods or functions with @required parameters as parameters. Especially when using the linter rule unnecessary_lambdas where a change from invocation to tear-off is just a simple automated refactor away (Replace function literal with tear off).

At least for my current use case I can just use an enum instead of a @required named boolean parameter and it'll convey the same information and additionally warn me if I'm using it where the @required annotation wouldn't work. But with the knowledge that @required only works sometimes, I'll probably avoid using it in the future or at least use it very cautiously, knowing it only provides a false sense of security.

[Hixie]

@matanlurey I assume B extends A? I don't understand why that's a runtime error caused by @required. It's exactly what would happen if the annotation was absent. Fundamentally that code is bogus, anyway, and ideally would be linted by something else (the subclass is changing the API in ways that aren't valid in reasonable OOP).

[bwilkerson]

But I am not sure, whether I would have immediately realized that no feedback will be provided by the analyzer when passing methods or functions with @required parameters as parameters.

The restriction to only warn for invocations is more accidental than intentional. There isn't any reason, for example, that we couldn't check for cases where a function with a required parameter (such as doStuff) is being passed as an argument where the parameter's type does not define the same required parameter. After all, we'd get the same behavior if we just converted the required named parameter to a language-defined required parameter:

void doStuff(int paramA, bool paramB) {}
void main() {
  doStuff(1);
  [1].forEach(doStuff);
}

Yes, there will always be ways to get around the type system (dynamic), but that doesn't mean that we can't do a better job than we currently are.

[srawlins]

@required is being replaced by required, so no further work will be done to support @required.

[denniskaselow]

I just tested with required instead of @required and it's working correctly.