dart-lang / language

Design of the Dart language
Other
2.67k stars 205 forks source link

Null safety feedback: No warning for dynamic? #1324

Open suragch opened 3 years ago

suragch commented 3 years ago

Running the following code

void main() {
  const name = null;
  final result = greeting(name);
  print(result);
}

String greeting(String name) {
  return 'Hello $name';
}

gives the following runtime error:

type 'Null' is not a subtype of type 'String'

I suppose this is an error because of the inferred dynamic type and not with null safety itself, but it would be nice if static analysis could at least give a warning.

lrhn commented 3 years ago

The problem is indeed the inferred dynamic type. You don't get warnings for dynamic.

You can try disabling implicit casts in the analyzer. It might still warn about downcasts from dynamic.

However, I see no reason we should not infer Null as the type for name. It's a constant, and we know the value precisely, so there is no good reason to infer dynamic for the variable here like we do for mutable variables.

Moving this to the language issue tracker.

@leafpetersen @stereotype441

tallinn1960 commented 3 years ago

The problem with dynamic is that it allows non-nullable items get nullable items assigned. If you have something of type dynamic you can assign it to nun-nullable items, although it may be null at runtime. Even this does compile:

void main(List<String> args) { Map<String, dynamic> l = {"a": "a"}; String b = l["b"]; // runtime error }

Maybe you notice that Map<String, dynamic> is the natural type to process json data.

If null safety is about the compiler catching possible null assignments, then dynamic spoils this. We do not want runtime errors as of null, do we? Isn't null safety all about this?

I suggest we need a difference between dynamic and dynamic?. The latter being nullable, the other one not so. At least the compiler should treat dynamic as nullable, so essentially being "dynamic?".

Levi-Lesches commented 3 years ago

If null safety is about the compiler catching possible null assignments, then dynamic spoils this. We do not want runtime errors as of null, do we? Isn't null safety all about this?

dynamic is all about "disabling" type safety entirely -- including null safety. Whenever you use dynamic, you are telling the compiler "trust me, this will work". If you're wrong, that will be a runtime error.

Even this does compile:

void main() {
  Map<String, dynamic> l = {"a": "a"}; 
  String b = l["b"];  // runtime error
}

That's not just because of dynamic. The following also compiles:

void main(List<String> args) {
  Map<String, Object> l = {"a": "a"};  // no dynamic here
  String b = l["b"] as String;  // produces an Object?, which we cast to String. Runtime error
}

Those two examples are equivalent. Yours uses dynamic, and mine uses as, but they both tell Dart "this value will be a non-null String".

I suggest we need a difference between dynamic and dynamic?. The latter being nullable, the other one not so. At least the compiler should treat dynamic as nullable, so essentially being "dynamic?".

Seems like you're thinking of Object vs Object?, where the latter is pre-null-safety dynamic and the former is a non-nullable dynamic.

lrhn commented 3 years ago

Having more kinds of special dynamic types is not a new idea. Consider being able to make any type have dynamic properties. You write dynamic num as a type, meaning "some unknown subtype of num, trust me, I know what I'm doing". It requires subtypes of num as values, and can do non-dynamic invocations of valid num members, but you can still do potentially invalid dynamic calls and assignments to subtypes of num.

Then the current dynamic would be dynamic Object?, but you can also write dynamic Object for a non-null-containing variable with dynamic dispatch and assignment to non-null types.

(Short of adding such a feature, I don't think we want to twiddle with just nullability of dynamic. The real problem here is not that dynamic is unsafe, but that you received something with type dynamic without asking for it. The return type of jsonDecode should be Object? instead, and you should use Map<String,Object?> for your JSON maps. The fact that people generally use dynamic for JSON values is because it allows assignment to anything.)

tallinn1960 commented 3 years ago

Looks like I totally missed the existence of Object in Dart. Makes me feel dumb. Never mind. Was this always there or introduced with null safety? Well, Object and Object? do indeed resolve my issues around handling collections of items of various types, possibly nullable without the need to resort to dynamic and throw away any safety net.

Levi-Lesches commented 3 years ago

Object has always been there, it's a super-class of everything by definition. With null-safety, the question was asked "if Object is the super-type of everything, does that include nullable types?" And so Object? was born as the nullable counterpart.

mateusfccp commented 3 years ago

@Levi-Lesches said:

Object has always been there, it's a super-class of everything by definition. With null-safety, the question was asked "if Object is the super-type of everything, does that include nullable types?" And so Object? was born as the nullable counterpart.

Yes, but this caused a major change, which is that Object is not a top type anymore, Object? replaced its role. This caused every code that used Object as top type (I used them a lot, as I see no reason to use dynamic anywhere) to potentially break.

So, now, for top types we have Object?, dynamic (🤢) and void, although void is not really intended to be used this way by the final user.

LewisHolliday commented 1 year ago

The surprising inference of Null as dynamic renders it impractical for use in return types. The lack of safety from Null would almost certainly cause usability issues. An example of a pattern that is impractical due to this issue:

abstract class A {
  int? foo();
}

class B implements A {
  @override
  Null foo() {}
}

Correctly, does not compile:

void main() {
  final b = B();
  final String s = b.foo(); //A value of type 'Null' can't be assigned to a variable of type 'String'.
}

Compiles:

void main() {
  final b = B();
  final r = b.foo(); //r inferred as dynamic
  final String s = r; //exception: type 'Null' is not a subtype of type 'String'
}
eernstg commented 1 year ago

That is indeed a quirky corner case. It has probably been given little attention because (1) only few people have used Null as a return type (even though there's nothing wrong with that), and (2) the local variable r gets the inferred type dynamic because it is initialized by an expression whose value is statically known to be null.

The original motivation for doing this is that it was a breaking change to do anything else: var x = null; (or the implicit variant of the same thing, var x;) used to mean dynamic x = null;, and it would be a potentially massive source of breakage to say that it now means Null x = null;. So we didn't do that way back when type inference of local variable types was introduced.

So we don't have a general mechanism whereby Null is transformed to dynamic, it's a very special rule which is applied to variable declarations (yes, it's applied to top level and static variables as well, but they will probably be linted).

However, in the given example those two things conspire to make final r = b.foo(); work in a way which is needlessly typeless.

You can detect many issues, including the fact that you're assigning a value with static type dynamic to a String typed variable, by enabling a more strict static analysis. Use something like this analysis_options.yaml:

analyzer:
  language:
    strict-raw-types: true
    strict-inference: true
    strict-casts: true

linter:
  rules:
    - avoid_dynamic_calls

With that, you get this output from dart analyze:

Analyzing n002.dart...                 0.8s

  error • n002.dart:23:20 • A value of type 'dynamic' can't be assigned to a
          variable of type 'String'. Try changing the type of the variable,
          or casting the right-hand type to 'String'. •
          invalid_assignment
warning • n002.dart:23:16 • The value of the local variable 's' isn't
          used. Try removing the variable or using it. •
          unused_local_variable

2 issues found.