I wish we had `--no-dynamic-calls` as a lint

[matanlurey]

Filing under "please lets fix this in Dart 2.1+".

Migrating internal code to Dart2, I hit the following fun error case:

class Link {
  static Link parse(Map<String, dynamic> linkData) {
    var userIds = linkData['userIds'].map((id) => int.parse(id));
     return new Link(userIds: userIds.toList());
  }

  final List<int> userIds;
  const Link({this.userIds});
}

... throws ...

type 'List<dynamic>' is not a subtype of type 'List<int>'

Huh? Shouldn't this work:

  .map((id) => int.parse(id));

It turns out, no! .map is a dcall, because linkData['userIds'] is dynamic. DOH!

Would --no-implicit-dynamic fix this? It seems like its an "explicit dynamic" (the map is typed of having values of type dynamic explicitly). It sounds like we'd need something strong_er_, i.e .--no-dynamic-calls.

/cc @leafpetersen @lrhn

[lrhn]

The map entry is indeed an explicit dynamic. You have no static information about the map method being called, it's a completely dynamic call, and you can't get inference.

In dynamic code, you are in charge of types, so either do:

var userIds = linkData['userIds'].map<int>((id) => int.parse(id));

or

List userIdStrings = linkData['userIds'];
var userIds = userIdStrings.map((id) => int.parse(id));

or

var userIds = linkData['userIds'].cast<String>().map(int.parse);

(which allows you to do as the style guide recommends).

I'm not sure what there is to fix here, short of removing either dynamic or reified generics from the language entirely. This is very much expected behavior for dynamic code.
(The only other option would be to let map with no type argument "infer" the return type at runtime from the passed function, which is known to return int, but that moves us away from static inference to dependent types).

[leafpetersen]

@matanlurey What's the right lint here? Some possibilities:

• Warn if you call a method on something of type dynamic
  • Then need an idiom for opting in to dynamic calls (e.g. (x as dynamic).foo) is allowed.
• Warn if you call a method on a non-variable of type dynamic
  • This makes it a bit harder to do accidentally
• Warn if you call a method on a non-variable, or a non-explicitly typed variable of type dynamic
  • This is similar to the first option, where using a variable with explicit dynamic type is the idiom
• Other?

cc @munificent

[matanlurey]

Thanks @lrhn @leafpetersen. Some replies below:

@lrhn:

The map entry is indeed an explicit dynamic. You have no static information about the map method being called, it's a completely dynamic call, and you can't get inference.

Part of the issue is the user didn't know they were making a dynamic call. It's quite hard to tell.

I'm not sure what there is to fix here, short of removing either dynamic or reified generics from the language entirely. This is very much expected behavior for dynamic code.

A creative option might be you can only assign a dynamic call to something typed dynamic:

void doSomethingA(dynamic list) {
  // Lint
  Iterable<String> x = list.map((e) => '$e');
}

void doSomethingB(dynamic list) {
  // OK
  dynamic x = list.map((e) => '$e');
}

@leafpetersen:

Warn if you call a method on something of type dynamic

Sounds right. I think if you say "it's OK to dcall when the assignee is also dynamic" it might remove the boilerplate of (x as dynamic) in many cases. Might not though. It does look ugly, but at the same time /shrug.

[leafpetersen]

One note is that in the IDE semantic highlighting can already help with this. You can set up Intellij to mark any dynamic variables as red, and any methods called on dynamic things as red:

[matanlurey]

@leafpetersen:

One note is that in the IDE semantic highlighting can already help with this

Yes, this is the answer I've gotten for almost 5 years now. I actually have it enabled myself, but:

• It doesn't work in VS Code
• It doesn't work in Cider (Internal Editor)
• It doesn't work in GitHub, Gerrit, Critique, or Code Search

Of course, it also doesn't catch refactors. I've hit cases internally quite a bit where I used to get back a say, Future<Bar>, but for whatever reason it was downgraded to a Future<dynamic>. My code keeps working, until it doesn't.

[leafpetersen]

@matanlurey Totally agree.

[munificent]

This is pretty frustrating, but it's hard to fix this cleanly while still actually having dynamic in the language. I don't think "no implicit dynamic" buys you much. Like you note, you did opt into dynamic in this code. I think it's probably too painful to require every single invocation on an object of type dynamic to have some extra required ceremony. At that point, we basically don't have dynamic any more.

Instead, here's some other ideas:

• Maybe JSON decoding (which is where I presume this comes from) should return Map<String, Object> instead of Map<String, dynamic> That makes JSON a lot more tedious to work with, and eliminates one of the primary motivations of even having a dynamic type. But it would have caught this. If we don't change the JSON API, you could maybe at least change the type annotation in your code.

• No implicit casts could help. It wouldn't highlight the place where the error originates, but if we disallowed implicit casting from dynamic, then you'd get an error here:

  return new Link(userIds: userIds.toList());

  when it tries to cast the dynamic to List<int>. That would give you a signal to look upstream and figure out where the dynamic came from.

[matanlurey]

This is pretty frustrating, but it's hard to fix this cleanly while still actually having dynamic in the language. I don't think "no implicit dynamic" buys you much. Like you note, you did opt into dynamic in this code.

Right. Something like --no-dynamic-calls is probably what I want (or to the same effect).

I think it's probably too painful to require every single invocation on an object of type dynamic to have some extra required ceremony. At that point, we basically don't have dynamic any more.

I don't think we need to require it, but I think being able to opt-in to those semantics is ideal. For example I'd think that framework authors would want to make sure they are never using dcalls on accident (as is this case), and certainly some teams would as well.

• Maybe JSON decoding (which is where I presume this comes from) should return Map<String, Object> instead of Map<String, dynamic> That makes JSON a lot more tedious to work with, and eliminates one of the primary motivations of even having a dynamic type. But it would have caught this. If we don't change the JSON API, you could maybe at least change the type annotation in your code.

Wasn't my code. Was helping a team debug, and as you mentioned it's hard to change the type down-stream several hops (might be code gen'd etc). It really seems like it needs to be something that the invocation level, or --treat-dynamic-as-object-like. Just spit-balling.

[natebosch]

+1 on being able to opt-in to --no-dynamic-calls. I suspect that a lot of dynamic calls today are not intentional and the ones that are intentional the extra ceremony might not be that bad. I'd be very curious to see how much this comes up in a corpus of code if we had a flag we could experiment with.

[munificent]

I think being able to opt-in to those semantics is ideal.

Doesn't that opt-in already exist, though? Use Object on any variable of unknown type that you want to prevent dcalls on and it becomes a static error.

[matanlurey]

Doesn't that opt-in already exist, though?

No, because:

• There is no lint to prevent me from doing this (so now it becomes code review burden)
• There are some sneaky subtle ways to get dynamic on "accident" (typedef Foo(String)).
• If an API I consume decides to return dynamic (or Future<dynamic>), I can still d-call.

... believe me, my team cares a lot about this, and even we've added d-calls on accident.

[natebosch]

Use Object on any variable of unknown type

The problem is when it's dynamic due to inference rules and we're trained to use var. Here's some sanitized code written by someone else that was unintentionally dynamic that I found when @leafpetersen ran a check treating dynamic as Object

var expectingListOfStrings = someList.fold(<String>[], (list, value) {
    list.add(getStringForValue(value));
    return list;
});

Note that because the callback takes (list, value) and not (List<String> list, value) the variable is dynamic and not List<String> like the author intended. One could argue that var wasn't appropriate here since it's non-obvious... but it remains that this is an easy hole to fall in to. (FWIW I rewrote this using .map(getStringForValue).toList() which gets the right type and is more readable)

Another can come up when we don't want to introduce an intermediate variable for the sake of avoiding dynamic.

var result = someMapOfDynamic['key'] + 1; // I want a warning here.

vs

Object value = someMapOfDynamic['key'];
var result = value + 1; // I get a warning here, but I'd never write code like this

[matanlurey]

Good examples @natebosch!

[zoechi]

With

  strong-mode:
    implicit-casts: false
    implicit-dynamic: false

I get static warnings

for implicit-dynamic: false

for implicit-casts: false

Is this not what you are looking for?