Skip to content

Sound null safety not working correctly #53922

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
AmrAbdelmagid opened this issue Nov 1, 2023 · 6 comments
Closed

Sound null safety not working correctly #53922

AmrAbdelmagid opened this issue Nov 1, 2023 · 6 comments
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). closed-as-intended Closed as the reported issue is expected behavior

Comments

@AmrAbdelmagid
Copy link

Issue Description:

The following code compiles without any compile time errors, but results in a runtime error:

class Foo {
  final int number;

  Foo({
    required this.number,
  });

  factory Foo.fromMap(Map<String, dynamic>? json) {
    return Foo(
      number: json?['number'] != null ? json!['number'] : null,
    );
  }
}

class Bar {
  Map<String, dynamic>? firstMap;

  void test() {
    // Runtime error: type 'Null' is not a subtype of type 'int'.
    final foo = Foo.fromMap(firstMap);
  }
}

void main() {
  final second = Bar();
  second.test();
}

The runtime error occurs because the number field of the Foo class is required and not nullable, but the factory Foo.fromMap() constructor allows to assign a null value in this case.

I think this is a bug in the Dart compiler. The compiler should generate an error at compile time, indicating that type int? is not a subtype of type int.

Steps to reproduce:

  1. Create a new Dart project
  2. Copy and paste the above code into the main.dart file
  3. Run the program

Or you can run this code snippet in DartPad.

Expected behavior:
The compiler should generate compile time error, indicating that type int? is not a subtype of type int.

Actual behavior:
The program compiles without any warnings, but results in a runtime error.

Environment:

Flutter version: 3.13.9
Dart version: 3.1.5
OS: macOS Ventura 13.5.2

@mit-mit mit-mit added the area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). label Nov 1, 2023
@mit-mit
Copy link
Member

mit-mit commented Nov 1, 2023

cc @lrhn

@parlough
Copy link
Member

parlough commented Nov 1, 2023

I can't speak to potential changes here and I'm not super familiar, but I can try to help out and provide safer alternatives.

I believe this is expected, since the type of the result of a ternary/conditional expression is the least upper bound of the types of both expressions. In this case json!['number'] has a static type of dynamic, so the entire expression has a static type of dynamic which can be default be assigned to int.

Two potential solutions to catch the mistake before runtime:

  • Avoid dynamic by typing the map parameter as something like Map<String, Object?>
  • Or enable the strict-casts analysis mode which will introduce a static warning here and require you to cast json!['number'] to an int. I personally highly recommend doing this and considering some of the other strict modes as well :)

@lrhn
Copy link
Member

lrhn commented Nov 1, 2023

This is indeed the upper-bound of the condtional expression (?/: operator) which ends up with dynamic as the type, which means that you don't get any static warnings about the result.

Which means this is working as currently specified, and the problem comes from having dynamic as a type, which turns any the static type help you can get from the compiler.

The solution, I hope, in the long run, will be to make it an error if either branch of a conditional expression is not assignable to the context type, and doing any necessary downcasts from dynamic before the join (dart-lang/language#3363).

Until then, avoid dynamic if possible. Definitely don't mix it with ?/:.

This code can be rewritten as:

class Foo {
  final int number;

  Foo({
    required this.number,
  });

  factory Foo.fromMap(Map<String, dynamic>? json) {
    if (json?['number'] case int n) {
      return Foo(number: n);
    } else {
      // Do something here. Trying to use `null` will give a compile-time error.
      throw "Badness";
    }
  }
}

class Bar {
  Map<String, Object?>? firstMap;

  void test() {
    final foo = Foo.fromMap(firstMap); // Throws "Badness".
  }
}

void main() {
  final second = Bar();
  second.test();
}

@lrhn lrhn closed this as completed Nov 1, 2023
@lrhn lrhn added the closed-as-intended Closed as the reported issue is expected behavior label Nov 1, 2023
@AmrAbdelmagid
Copy link
Author

@parlough @lrhn Really Thanks so much for your quick response and clear explanation!

@eernstg
Copy link
Member

eernstg commented Nov 1, 2023

@AmrAbdelmagid, you might want to get a heads up at compile time when the type dynamic arises in an expression (and in various other situations, e.g., during type inference). Put the following into a file named analysi-options.yaml in the root of your package:

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

linter:
  rules:
    - avoid_dynamic_calls

With this file in place, the analyzer reports the following issues on your example:

Analyzing n023.dart...                 2.1s

  error • n023.dart:10:15 • The argument type 'dynamic' can't be assigned to
          the parameter type 'int'.  • argument_type_not_assignable
warning • n023.dart:20:11 • The value of the local variable 'foo' isn't
          used. Try removing the variable or using it. •
          unused_local_variable

2 issues found.

@eernstg
Copy link
Member

eernstg commented Nov 1, 2023

Oops, @parlough already mentioned strict-casts and its ilk, sorry about the unnecessary comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). closed-as-intended Closed as the reported issue is expected behavior
Projects
None yet
Development

No branches or pull requests

5 participants