dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.31k stars 1.59k forks source link

`unecessary_lambdas` false positive: Generic tear-offs seems to not be dealing fine with dynamic types #58657

Open shinayser opened 2 years ago

shinayser commented 2 years ago

Sup folks! Check this code snipped taht is working fine:

import 'dart:convert';

final _json = '''{
  "listOfPeople": [
    {"name":"Daniel", "age":33},
    {"name":"Pedro", "age":10}
  ]
}''';

void main(List<String> arguments) async {
  final Map<String, dynamic> theMap = json.decode(_json);
  final List jsonList = theMap['listOfPeople'] ?? [];
  final listOfPeople = jsonList.map((e) => People.fromJson(e)).toList();

  print(listOfPeople);
}

class People {
  String name;
  int age;

  People({
    required this.name,
    required this.age,
  });

  factory People.fromJson(Map<String, dynamic> map) {
    return People(
      name: map['name'],
      age: map['age'],
    );
  }

  @override
  String toString() => 'People(name=$name, age=$age)';
}

This code snippet is working pretty fine. But after upgrading to Flutter 2.10 and dart 2.15.1, the analyzer started to warning me about replacing (e) => People.fromJson(e) by its tear-off version.

But when I do it, it doesn't compile, complaining with The argument type 'People Function(Map<String, dynamic>)' can't be assigned to the parameter type 'dynamic Function(dynamic)'.

But the wrost part of all is that I can't figure out a way to correct typing it in a way the everything works.

Seems a bug to bem or I am just being super noob?

eernstg commented 2 years ago

You're using the declared type List for the local variable jsonList. The type List is an abbreviation of List<dynamic>, so you do get the static and dynamic check that the value of theMap['listOfPeople'] ?? [] is actually a List<T> for some T, but you're leaving the type unspecified for the elements of that list.

As long as you're using (e) => People.fromJson(e) this works out: The function literal gets the parameter type dynamic (because jsonList is a List<dynamic>, so map will provide an object of type dynamic for each element in the list), and it is accepted without diagnostic messages that you are passing the argument e to People.fromJson — there will be a run-time type check on the value of e as needed, because that's exactly what we are asking for by using the type dynamic.

However, when you use the constructor tear-off People.fromJson the situation has more type information available, and it is not supported to assign a function of type People Function(Map<String, dynamic>) to a parameter of type dynamic Function(dynamic) (because the latter promises to accept any object as the actual argument, and the former won't do that).

So you probably just want to stop using "a half type" like List. You could use

final List<Map<String, dynamic>> jsonList = theMap['listOfPeople'] ?? [];

which may or may not succeed at run-time (I don't know whether it's safe to do that, but if it works then it's a good solution), or you could simply omit the declared type of jsonList (in which case jsonList gets the inferred type dynamic, which will eliminate all static type checking from the invocation of map, and ensure that we get the required type checks at run time.

shinayser commented 2 years ago

Hey @eernstg Thanks for the explanation. But I already tried your suggestion and still doesnt work. Yes, not it now compiles, but we get runtime errors:

type 'List<dynamic>' is not a subtype of type 'List<Map<String, dynamic>>'

I know that this now is unrelated to tear-ofs, but maybe this is a problem other people might be going through when trying to use the new tear off constructors for json mapping?

lrhn commented 2 years ago

This is a tricky case.

The problem is that (e) => People.fromJson(e) looks like it's just wrapping the People.fromJson function unnecessarily, but it's actually shorthand for (dynamic e) => People.fromJson(e as Map<String, dynamic>), the rest is added in the type inference step. Removing the (e) => ... (e) also removes that implicit as cast on the argument, from dynamic to Map<String, dynamic>, and changes the meaning.

So, the analyzer is wrong, this is not an unnecessary function literal.

You can't decide whether such a wrapping is unnecessary from the syntax alone, you need to do the type inference and see whether there is an implicit downcast.

eernstg commented 2 years ago

@shinayser wrote:

we get runtime errors: type 'List<dynamic>' is not a subtype of type 'List<Map<String, dynamic>>'

Yes, that's what I meant by

which may or may not succeed at run-time

Concretely, if the implementation of json.decode doesn't actually produce an object of type List<Map<String, dynamic>> in the given part of the returned object then there will be type errors at run time. The error message shows that it actually delivers a List<dynamic>, so we do get that error.

I agree that the analyzer should detect the inferred cast, and abstain from recommending that the function literal is abbreviated to a constructor tear-off.

srawlins commented 2 years ago

I don't get any error or lint report in dartpad. @shinayser can you tell us the error you see with the original code?

shinayser commented 2 years ago

@srawlins

image

srawlins commented 2 years ago

I wish the IDE included the error code, but 🤷 I think this is a code produced by the linter.