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?

[matanlurey]

@zoechi:

You happened to catch some errors, but not all. Consider:

linkData['userIds'].forEach(invokeSomeFunctionInAnotherLibrary);

... here there is a d-call .forEach which is not highlighted.

[zoechi]

Right. The IDE shows it as unresolved but the analyzer doesn't report a warning

[jmesserly]

FWIW, I was planning to try a DDC flag for --no-dynamic-calls. I wanted to find unintended dynamic calls in DDC's SDK implementation code and fix them. (For places where the dynamic call is intended, (x as dynamic).dynamicCall(...) would disable the error message.)

[jmesserly]

The tracking bug for the DDC flag is #30481. Originally I'd thought of the flag for my own benefit: it's easy to accidentally introduce dynamic into the SDK implementation with inline JS code (DDC and dart2js SDKs contain inline JS code snippets, similar to Dart VM's native calls). That said, maybe it gives y'all a way to test the feature and see if it meets your needs. If it does, then perhaps we could consider an Analyzer/linter option. (edit: linter is another possibility)

[bwilkerson]

We definitely want a lint for this, but the DDC support will probably happen first.

[matanlurey]

/cc @srawlins @leafpetersen @vsmenon

We (AngularDart) and Sam (google3) would be happy to help develop/test/rollout this flag. It definitely would be better to be a static analysis warning/error than a runtime (DDC-only) behavior, though.

[jmesserly]

It definitely would be better to be a static analysis warning/error than a runtime (DDC-only) behavior, though.

It would be a compile-time (static analysis) flag in DDC. Agree that linter flag is more useful though. If we can enable that lint from DDC, it would be ideal, saves me some work 👍

[matanlurey]

Oh ok, I see. Yeah, even just in DDC would be a good step (better than nothing). I think for our internal users though, folks would be annoyed if the analyzer didn't flag code that the compiler would, if that makes sense.

Just thinking out loud, I'm wondering what a good pattern for "intentional" dynamic calls is:

(x as dynamic) doesn't require anything special, but reads bad like something is broken.

What if we added in, as an example, package:meta (or similar), a invokeDynamic function:

/// Returns an object that may be dynamically invoked without flagging `--strict-dynamic-calls`.
dynamic allowDynamicCall(Object any) => ...

It might read better, for example:

void example(dynamic x) {
  // LINT: strict_dynamic_calls
  x.doUntyped();

  // OK
  allowDynamicCall(x).doUntyped();
}

[lrhn]

About allowDynamicCall, isn't that what lint-comments like // ignore: dynamic_invocation is for?

[matanlurey]

@lrhn:

isn't that what lint-comments like // ignore: dynamic_invocation is for?

That's true! I don't like everything about those flags though:

• There is no way to detect, today, that // ignore: ... is a no-op (i.e. not ignoring anything)
• You are able to ignore at the file level // ignore_for_file: , making it hard to catch in code reviews

If both of those were changed, I'd consider using the comment notation, but until then, similar to my request for unsafeCast, I'd prefer an explicit way into opting into the old semantics.

[munificent]

Relevant: TypeScript's new unknown type is, I think what you describe. Like dynamic, any value can be assigned to it. Like a top type, it has no operations on it, so you have to cast to something more specific before you can use it.

I'm hesitant to get excited about yet another magical type, but it's a good reference for research.

[matanlurey]

How is unknown different than our Object? @munificent

[munificent]

I think mechanically it's the same, but it signals a different intent. "Object" says "I work with any object of any type". "unknown" would say "I don't have a way to express the set of types I work with.".

[natebosch]

"unknown" would also presumably not allow implicit casts like "Object" does today.

[matanlurey]

Hi @denniskaselow! I don't think this is directly related to --no-dynamic-calls.

Instead, you might want to file your support/interest of:

• Would like a way to avoid raw generic types #33119
• I wish to remove implicit downcasts by default #31410
• Dart should warn on guaranteed cast errors #33372

[matanlurey]

Another use case might be linting Function (i.e. untyped function definitions):

https://www.dartlang.org/guides/language/effective-dart/design#prefer-signatures-in-function-type-annotations

[MichaelRFairhurst]

Late to the thread here, but given that the only difference between Object and dynamic is that dynamic allows dynamic calls, should this not be a lint like "no dynamic" ?

Rather than linting:

Map<String, dynamic> x;
x["y"].z; // lint!

why not lint the dynamic originally, and request:

Map<String, Object> x; // was lint, fixed
x["y"].z; // error!

I suppose the obvious counterargument is if you import a lib that gives you dynamic, you still want to be warned when you use it.

But it seems like with no_dynamic_calls, that should imply no_dynamic, so maybe it should just be one lint, no_dynamic, which catches both dynamic calls, and, dynamic declarations (implicit or explicit).

[matanlurey]

@MichaelRFairhurst:

Using Object instead of dynamic was discussed (to death) above already:

• https://github.com/dart-lang/sdk/issues/33121#issuecomment-391783668
• https://github.com/dart-lang/sdk/issues/33121#issuecomment-391785411
• https://github.com/dart-lang/sdk/issues/33121#issuecomment-391789451

There are many many many reasons that this can't be fixed without linting the actual call.

[jmesserly]

There are many many many reasons that this can't be fixed without linting the actual call.

yeah, and we have previous experience with no-implicit-dynamic, which was difficult to adopt because of how many locations one needs to fix. no-dynamic-calls should have a higher signal/noise ratio.

[pq]

Amazing @matanlurey!

I'll work on rolling a new release with this in it early next week. 👍

[proninyaroslav]

@matanlurey
What if the project uses jsonDecode? It returns dynamic as the result of parsing, and if I want to write, for example, code like this

final root = jsonDecode(jsonStr);
if (root['foo'] != null) {
  final foo = root['foo'] as String;
  ...
}

then the linter is throwing a bunch of avoid_dynamic_calls messages, but how can I work differently with json? I just had to disable avoid_dynamic_calls for this file.

[lrhn]

You can (and I'd recommend to) write that as:

Map<String, dynamic> root = jsonDecode(jsonStr);
String? foo = root['foo'];
if (foo != null) {
  ... 
}

If you want to avoid implicit downcasts too, that would be:

var root = jsonDecode(jsonStr) as Map<String, dynamic>;
var foo = root['foo'] as String?;
if (foo != null) {
  ... 
}

but it means exactly the same thing.