Sdk: Analyzer/DDC: Allows downcasts of const?

[matanlurey]

Related to https://github.com/dart-lang/sdk/issues/33303, I wouldn't have ever hit this error case if the IDE/analyzer would have stopped me (really, an internal customer I'm proxying for) write this code and check it in.

import 'package:test/test.dart';

class Provider {
  const Provider();
}

class Module {
  final List<Provider> providers;
  const Module(this.providers);
}

const aGoodModule = const Module(const [
  const Provider(),
]);

const listOfThings = const [
  const [],
  const Provider(),
];

const aBadModule = const Module(listOfThings);

void main() {
  test('$aBadModule should be invalid', () {
    expect(aBadModule.providers, const isInstanceOf<List<Provider>>());
  });
}

... produces no errors, warnings, hints, or lints, even in strong mode. This is invalid, though:

const aBadModule = const Module(/*List<Object>*/listOfThings);

... because Module statically requires List<Provider>. I sort of understand why, at runtime, this is allowed (because of downcasts - and @leafpetersen has seen my many many bugs on this), but I'm not sure why it is allowed statically in a const context (the CFE/VM seems to reject it).

Additionally, DDC builds the code ... it only later fails at runtime:

Type 'List<Object>' should be 'List<Provider>' to implement expected type 'List<Provider>'.
  dart:sdk_internal                     check_C
  dart2_const_downcast_test.dart 21:32  get aBadModule
  dart:sdk_internal                     get
  dart2_const_downcast_test.dart 24:10  main

[matanlurey]

Interestingly, this does fails nicely at compile-time in Dart2JS with --preview-dart-2:

test/dart2_const_downcast_test.dart:21:26:
Error: `listOfThings` of type 'List<Object>' is not a subtype of List<Provider>.
const aBadModule = const Module(listOfThings);
                         ^
Error: Compilation failed.

[matanlurey]

Code: https://github.com/matanlurey/dart.scratch/tree/master/dart2_const_downcast

You can reproduce:

# DDC
$ pub run build_runner test 

# Dart2JS
$ pub run build_runner test -r

[leafpetersen]

DDC does runtime constant evaluation, so it's not too surprising that it wouldn't issue an error until then.

The analyzer does do const evaluation, and it is likely not doing so with Dart 2 semantics (including downcasts).

cc @bwilkerson @devoncarew

[vsmenon]

@bwilkerson - can you confirm if the analyzer is ignoring downcasts on consts per @leafpetersen 's comment?

[stereotype441]

I just looked into this, and it seems that the analyzer's constant evaluation engine was never updated for strong mode semantics. I wouldn't say that it ignores downcasts per se--it implements the Dart 1.0 rules for checked mode (when you ask it to), and those are similar to Dart 2.0 downcasts in many cases. But they aren't similar enough to catch the error in Matan's example.

Digging deeper into Matan's example, there are actually three bugs preventing an error from being reported:

• The analyzer's constant evaluation engine doesn't account for type inference when computing the type of a list literal, so even though it correctly infers a static type of List<Object> for the variable listOfThings, it produces a constant value for that variable having type List<dynamic>.
• The analyzer's mechanism for detecting constant evaluation errors is based on the Dart 1.0 type system, so it considers it valid to assign a constant value of type List<dynamic> to Module.providers (in Dart 1.0, dynamic was allowed to fill the role of either bottom or top during subtype checks, so List<dynamic> was considered a valid subtype of List<Provider>).
• By default the command line analyzer reports constant evaluation errors with severity info, even when strong/dart2 semantics are enabled. To see errors, you have to override the default by providing the flag --enable_type_checks, which, sadly, is a hidden flag.

I'll start to work on updating the constant evaluation engine to properly implement Dart 2.0 semantics, and once it does, I'll make sure that constant evaluation errors are reported as errors in Dart 2.0 mode.

[matanlurey]

Wow! Thanks for looking into this @stereotype441!

[stereotype441]

Looks like #21119 is another contributing cause.

[stereotype441]

Partial fix for the first issue is here: https://dart-review.googlesource.com/#/c/sdk/+/60203. Note that the summary format still needs to be extended to allow constants referenced via summaries to have proper list and map types (see #33441).

[MichaelRFairhurst]

I am moving this out of the dart stable milestone. I think we simply don't have resources to handle this in time. If anyone disagrees....we'll have to scour for helping hands I think.

[natebosch]

@MichaelRFairhurst - to make sure I understand correctly, the fix for this issue will involve moving a _guaranteed_ runtime error to a static error - it isn't possible to have code which compiles and runs successfully today (by avoiding this code path) which would fail to compile and run after the fix?

If so I think it's fine to move this to Dart2.1. If instead there can exist some code which will stop working because of the fix I think we should consider it for Dart2Stable.

[matanlurey]

Is this planned to be fixed for 2.1? @MichaelRFairhurst

[MichaelRFairhurst]

I'm not sure this is worth doing before the CFE/analyzer integration. It looks like a good chunk of work to move a compile time error to an analysis time error, which seems like in general a lower value proposition.

I'm also not particularly good at working in this system, which is quite advanced...and those who are are working on the CFE integration.

If you have any concerns with that, if this is particularly important to you, please do voice your concerns here!

Also +@devoncarew @leafpetersen for consideration.

[matanlurey]

I guess it's just a particularly confusing usability issue - specifically in annotations (Angular) that are used a lot, because downcasts from Object are now silently accepted everywhere.