Skip to content
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

Declared type is never respected using extensions in dart 3. #53481

Closed
jonataslaw opened this issue Sep 10, 2023 · 8 comments
Closed

Declared type is never respected using extensions in dart 3. #53481

jonataslaw opened this issue Sep 10, 2023 · 8 comments

Comments

@jonataslaw
Copy link

When declaring a type like:

AbstractType foo = ConcreteImplementation();

The AbstractType type is expected to be respected. This is a common feature in all existing languages, and the default behavior of dart before version 3.

Currently, this behavior is partially respected, within the classes where it is used, but it is definitely never respected using extensions.

We can verify this with the following example. What do we expect to be printed?
'Instance of Black'

What is printed? Nothing, because before executing this code we got an error:

 Exception has occurred.
_TypeError (type 'Black' is not a subtype of type 'White' of 'value')

Example:

void main() {
  final Wrapper<TestColor> theme = White().transform();
  theme.value = Black();

  print(theme.value);
}

sealed class TestColor {
  String get name => switch (this) {
        White() => 'white',
        Black() => 'black',
      };
}

class White extends TestColor {}

class Black extends TestColor {}

extension ThemeTransform<T> on T {
  Wrapper<T> transform() => Wrapper<T>(this);
}

class Wrapper<T> {
  T value;
  Wrapper(this.value);
}

Here is similar Flutter code, which shows the difference between using types within the class it will be used in, and in extensions. The second button works, the first triggers an error.

import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

sealed class TestColor {
  String get name => switch (this) {
        White() => 'white',
        Black() => 'black',
      };
}

class White extends TestColor {}

class Black extends TestColor {}

extension ValueNotifierExt<T> on T {
  /// Returns a `ValueNotifier` instance with [this] `T` as initial value.
  ValueNotifier<T> notifier() => ValueNotifier<T>(this);
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        colorScheme: ColorScheme.fromSeed(seedColor: Colors.deepPurple),
        useMaterial3: true,
      ),
      home: const HomePage(),
    );
  }
}

class HomePage extends StatefulWidget {
  const HomePage({super.key});

  @override
  State<HomePage> createState() => _HomePageState();
}

class _HomePageState extends State<HomePage> {
  TestColor stateColor = White();
  final ValueNotifier<TestColor> value = White().notifier();

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: [
            ValueListenableBuilder(
              valueListenable: value,
              builder: (context, value, child) => Text(
                value.name,
              ),
            ),
            FilledButton(
              // Error: type 'Black' is not a subtype of type 'White' of 'val'
              onPressed: () {
                switch (value.value) {
                  case White():
                    value.value = Black();
                  case Black():
                    value.value = White();
                }
              },
              child: const Text(
                'ValueNotifier Color',
              ),
            ),
            const SizedBox(height: 40),
            Text(stateColor.name),
            FilledButton(
              // Works well
              onPressed: () {
                switch (stateColor) {
                  case White():
                    setState(() {
                      stateColor = Black();
                    });
                  case Black():
                    setState(() {
                      stateColor = White();
                    });
                }
              },
              child: const Text(
                'StateChange Color',
              ),
            ),
          ],
        ),
      ),
    );
  }
}
@eernstg
Copy link
Member

eernstg commented Sep 11, 2023

This is working as intended, but there are a few steps in this process. Consider this variant:

void main() {
  final Wrapper<TestColor> theme = (White() as TestColor).transform();
  theme.value = Black();

  print(theme.value);
}

// Other declarations unchanged.

This variant compiles and runs without any errors. The underlying issue here is dynamically checked covariance: In the original program we have the following:

  • White().transform() is an extension method invocation where the value of the type parameter T is determined by inspecting the static type of the receiver. This receiver has type White and hence T is White. So we will create a Wrapper<White> and use that to initialize theme.
  • An important property of the previous step is that theme has static type Wrapper<TestColor> and dynamic type Wrapper<White>. In other words, the typing of theme relies on the fact that the type parameter of Wrapper is covariant. If it had had any other variance (e.g., if it had been invariant) then Wrapper<White> would not have been a subtype of Wrapper<TestColor>, and in that case we would have had a compile-time error at the declaration of theme.
  • Covariance in dart is subject to run-time checks in the situations where the compile-time checks are insufficient to ensure type safety. In particular, there's a run-time check when we execute theme.value = Black(). The check tries to confirm that the type argument of Wrapper in the type of theme is a supertype of Black (that's required in order to perform this assignment). This isn't true, so the test fails, and we get a dynamic error.

There are many situations where the covariant typing is avoided because type inference in Dart gives priority to the context type. For instance, if we declare List<num> xs = [1, 2]; then type inference will turn [1, 2] into <num>[1, 2] (rather than <int>[1, 2] which would have been OK for the given elements). This is very helpful in many situations, because we can then proceed to trust the declared type List<num>, and we can do things like xs.add(5.9). If the list had been a List<int> with static type List<num> then that would also have been a dynamic error.

However, in this particular case the covariant typing cannot be avoided based on the context type. The reason for this is that the typing of an extension method invocation is based on the receiver, and nothing else. So we inspect the receiver White(), detect that it has static type White, and proceed to use the value White as the actual type argument for the extension.

If you wish to support an enhancement of Dart whereby this kind of issue can be detected at compile time then you could vote for dart-lang/language#524.

You can also emulate invariance already today. Replace the declaration of Wrapper by the following:

typedef _Inv<X> = X Function(X);
typedef Wrapper<X> = _Wrapper<X, _Inv<X>>;

class _Wrapper<T, Invariance> {
  T value;
  _Wrapper(this.value);
}

If you do that then final Wrapper<TestColor> theme = White().transform(); is reported as a compile-time error, because Wrapper<White> is no longer a subtype of Wrapper<TestColor>.

The idea behind this idiom is that we want the real wrapper class to take two type arguments, and they must always be of the form <S, S Function(S)> because this means that the subtype check will require two wrapper types to have the same type argument:

With the original Wrapper class we would check whether Wrapper<White> is a subtype of Wrapper<TestColor> (also written as Wrapper<White> <: Wrapper<TestColor>), and that's true (by covariance) if White is a subtype of TestColor, and that's true—done!

But with the new two-type-parameter version which is called _Wrapper, we'd need to check whether _Wrapper<White, White Function(White)> <: _Wrapper<TestColor, TestColor Function(TestColor)>, and that is only true if we have both White <: TestColor and White Function(White) <: TestColor Function(TestColor), and the latter requires both White <: TestColor (OK, we already checked that) and TestColor <: White (which is not true).

We renamed the real wrapper class to _Wrapper and defined Wrapper as a type alias because we want to make sure that clients (at least in other libraries) must always provide type arguments of the form <S, S Function(S)> (because they can't see _Wrapper they must use the type alias Wrapper). Finally, _Inv was defined because any substantial use of this idiom calls for a slightly less verbose way to write S Function(S).

If dart-lang/language#524 or something similar is accepted into the language then we could get the same effect as follows:

class Wrapper<inout T> {
  T value;
  Wrapper(this.value);
}

The keyword inout is used to specify that the type variable T must be usable "as input" and "as output", which means that it can be used as a parameter type as well as a return type (in this case: a parameter type of the setter which is implicitly induced by the instance variable, and as the return type of the getter).

@jonataslaw
Copy link
Author

First and foremost, I genuinely appreciate your comprehensive explanation regarding the issue. It’s clear that a significant amount of thought has been invested in the design and implementation of the Dart type system. However, I'd like to address a few concerns further.

  • The code in question worked seamlessly in Dart 2. Given the smooth transition from Dart 2 to Dart 3, it was surprising to face this issue. I've scoured the changelog diligently, but couldn't pinpoint a clear mention of this particular change. While it's conceivable that this change might have gone unnoticed, it's crucial to remember that even seemingly minor shifts can profoundly impact our projects. Predictability is a essential part of the programming process.

  • Dart is renowned for its robust type system. When the static type checker remains silent, it typically instills confidence that our code will run without type errors. Hence, encountering a _TypeError at runtime, in the absence of any static checking, is quite disconcerting. This unpredictability introduces an element of doubt into development cycles, leading to concerns about possible unanticipated runtime errors, even in seemingly well-typed code.

  • While the foundational mechanisms and type theory are intricate, the experience for developers should ideally remain intuitive. The current situation makes it a tad challenging to predict code behavior based on static types, slightly diminishing the developer experience. That said, I acknowledge the ongoing discussion you mentioned, and I'm hopeful that it might offer a resolution (thanks for highlighting it).

To potentially mitigate this issue, it would be invaluable to have a lint warning for code segments that might lead to a runtime _TypeError. Lints and static analysis are pivotal tools that developers lean on to ensure consistent, error-free coding. Implementing such a lint would proactively alert developers to potential pitfalls.

@mraleph
Copy link
Member

mraleph commented Sep 11, 2023

The code in question worked seamlessly in Dart 2.

Could you provide the example which works differently in Dart 2 and Dart 3? The piece of code included in the issue (White().transform()) works the same way in both versions: it produces an instance of Wrapper<White>.

When the static type checker remains silent, it typically instills confidence that our code will run without type errors.

Runtime checked covariance is always there (e.g. List<Animal> animals = <Dog>[]; animals.add(Cat());) and can be a source of runtime errors. Null checking (x!) or any sort of dynamic casts (x as Y). Any of these can cause type error in runtime.

That's not to say that you should not trust your code - but rather to highlight the fact that even well typed code can throw runtime errors.

@eernstg
Copy link
Member

eernstg commented Sep 11, 2023

Agreeing with @mraleph (in particular, I don't understand how it could work in Dart 2), I'll just add a couple of extra comments:

@jonataslaw wrote:

Hence, encountering a _TypeError at runtime, in the absence of any static checking, is quite disconcerting.

Oh, but there is a static check! (So you could say that the situation is even worse ;-)

The fact that a class uses a type variable in a non-covariant position (e.g., as the type of an instance variable) is known to the compiler, and it will generate the required code in order to check at run time whether or not that situation has caused an actual argument to be passed (in this case: to the setter of the variable value) even though it doesn't have the required type.

So that _TypeError is only thrown because it is known at compile time that it may be needed.

If we get support for declaration-site variance (dart-lang/language#524) and make Wrapper invariant in T then it is known at compile time that this type check will never be needed (and then the code to perform the dynamic type check won't be generated in the first place).

@jonataslaw
Copy link
Author

This is working as intended, but there are a few steps in this process. Consider this variant:

void main() {
  final Wrapper<TestColor> theme = (White() as TestColor).transform();
  theme.value = Black();

  print(theme.value);
}

// Other declarations unchanged.

This variant compiles and runs without any errors. The underlying issue here is dynamically checked covariance: In the original program we have the following:

  • White().transform() is an extension method invocation where the value of the type parameter T is determined by inspecting the static type of the receiver. This receiver has type White and hence T is White. So we will create a Wrapper<White> and use that to initialize theme.
  • An important property of the previous step is that theme has static type Wrapper<TestColor> and dynamic type Wrapper<White>. In other words, the typing of theme relies on the fact that the type parameter of Wrapper is covariant. If it had had any other variance (e.g., if it had been invariant) then Wrapper<White> would not have been a subtype of Wrapper<TestColor>, and in that case we would have had a compile-time error at the declaration of theme.
  • Covariance in dart is subject to run-time checks in the situations where the compile-time checks are insufficient to ensure type safety. In particular, there's a run-time check when we execute theme.value = Black(). The check tries to confirm that the type argument of Wrapper in the type of theme is a supertype of Black (that's required in order to perform this assignment). This isn't true, so the test fails, and we get a dynamic error.

There are many situations where the covariant typing is avoided because type inference in Dart gives priority to the context type. For instance, if we declare List<num> xs = [1, 2]; then type inference will turn [1, 2] into <num>[1, 2] (rather than <int>[1, 2] which would have been OK for the given elements). This is very helpful in many situations, because we can then proceed to trust the declared type List<num>, and we can do things like xs.add(5.9). If the list had been a List<int> with static type List<num> then that would also have been a dynamic error.

However, in this particular case the covariant typing cannot be avoided based on the context type. The reason for this is that the typing of an extension method invocation is based on the receiver, and nothing else. So we inspect the receiver White(), detect that it has static type White, and proceed to use the value White as the actual type argument for the extension.

If you wish to support an enhancement of Dart whereby this kind of issue can be detected at compile time then you could vote for dart-lang/language#524.

You can also emulate invariance already today. Replace the declaration of Wrapper by the following:

typedef _Inv<X> = X Function(X);
typedef Wrapper<X> = _Wrapper<X, _Inv<X>>;

class _Wrapper<T, Invariance> {
  T value;
  _Wrapper(this.value);
}

If you do that then final Wrapper<TestColor> theme = White().transform(); is reported as a compile-time error, because Wrapper<White> is no longer a subtype of Wrapper<TestColor>.

The idea behind this idiom is that we want the real wrapper class to take two type arguments, and they must always be of the form <S, S Function(S)> because this means that the subtype check will require two wrapper types to have the same type argument:

With the original Wrapper class we would check whether Wrapper<White> is a subtype of Wrapper<TestColor> (also written as Wrapper<White> <: Wrapper<TestColor>), and that's true (by covariance) if White is a subtype of TestColor, and that's true—done!

But with the new two-type-parameter version which is called _Wrapper, we'd need to check whether _Wrapper<White, White Function(White)> <: _Wrapper<TestColor, TestColor Function(TestColor)>, and that is only true if we have both White <: TestColor and White Function(White) <: TestColor Function(TestColor), and the latter requires both White <: TestColor (OK, we already checked that) and TestColor <: White (which is not true).

We renamed the real wrapper class to _Wrapper and defined Wrapper as a type alias because we want to make sure that clients (at least in other libraries) must always provide type arguments of the form <S, S Function(S)> (because they can't see _Wrapper they must use the type alias Wrapper). Finally, _Inv was defined because any substantial use of this idiom calls for a slightly less verbose way to write S Function(S).

If dart-lang/language#524 or something similar is accepted into the language then we could get the same effect as follows:

class Wrapper<inout T> {
  T value;
  Wrapper(this.value);
}

The keyword inout is used to specify that the type variable T must be usable "as input" and "as output", which means that it can be used as a parameter type as well as a return type (in this case: a parameter type of the setter which is implicitly induced by the instance variable, and as the return type of the getter).

This piece of code might indeed address the problem. However, after examining it closely, I noticed that while the solution appears to tackle the issue, it does seem to conflict with one of Dart's rules. For clarity, you can refer to this: Dart Diagnostic Messages - Unnecessary Cast.

If we don't manually disable this rule, the lint tool will automatically remove the casting upon saving, affecting the execution.

I also observed that the error in question is somewhat elusive. At one point, this exception was introduced, possibly because of the Dart code plugin. I just tried to run code that had been in production since last year, and suddenly it stopped working.

While I understand your viewpoint, I would like to kindly express my difference in opinion regarding its classification as a compile-time error. The issue emerges when the state of the variable shifts, making it hard to anticipate during compilation. It's accurate to say that unit tests might spot this, but ideally, the lint rule should prevent such constructs:

final Wrapper<TestColor> theme = White().transform();
theme.value = Black();

If value is an instance of White, it's we expect that Black(); shouldn't be assignable to it.

Ideally, linting and static typing tools should collaborate seamlessly to ensure such consistency. It would undoubtedly be advantageous for developers to trust the language's static typing without needing comprehensive unit tests for these fundamental verifications.

@eernstg
Copy link
Member

eernstg commented Sep 11, 2023

@jonataslaw wrote, about unnecessary_cast:

If we don't manually disable this rule, the lint tool will automatically remove the casting upon saving

Oh, that's unfortunate. That diagnostic probably does not make any attempt to detect whether the cast is actually unnecessary, it just lints every upcast (based on the wrong assumption that they are always unnecessary).

You can use // ignore: unnecessary_cast to disable it, hopefully that will also cancel the automatic removal-upon-save.

While I understand your viewpoint, I would like to kindly express my difference in opinion regarding its classification as a compile-time error.

I did not intend to say that this is a compile-time error!

The situation where _TypeError is thrown is clearly a run-time error. I was just saying that this error does not happen at arbitrary locations in the code, it happens exactly in the location where the compiler has generated code to perform the run-time type check.

The point is that it is not particularly difficult to get rid of this kind of run-time error: We just need statically checked variance.

We do need to recognize that statically checked variance isn't going to happen everywhere in Dart anytime soon. It's too much of a breaking change, and there is a huge amount of code out there which is actually working, and which was probably simpler to write because of dynamically checked covariance. However, if we get something like statically checked declaration-site variance then each class author (who isn't tied on their hands and feet based on existing code) can choose to use statically safe variance, and hence turn this run-time error into a compile-time error.

@vsmenon
Copy link
Member

vsmenon commented Sep 11, 2023

Closing this as working as intended. FWIW, this is the expected behavior of covariant generics. It's fundamentally the same as the following Java example, where you'll get no static error, but a runtime ArrayStoreException error:

         Object[] foo = new String[] { "hello", "world" };
         foo[1] = 42;

There are several open issues around generics here: https//github.com/dart-lang/language/issues

It might make sense to open a separate linter issue if we need to ignore a cast here.

Another option for your code is to redefine the extension:

extension ThemeTransform on TestColor {
  Wrapper<T> transform<T extends TestColor>() => Wrapper<T>(this as T);
}

A generic method (instead of a generic extension) will infer the type of T based on the surrounding context.

@vsmenon vsmenon closed this as completed Sep 11, 2023
@jonataslaw
Copy link
Author

@eernstg Thank you very much for the clarification. English isn't my first language, so this seems clearer to me now.
Using lint rule exclusion is a definitive way (along with casting) to solve the problem.

The solution proposed by @vsmenon also works as intended.

I will open an issue about the problem with the linter in the appropriate repository.

Thank you very much for your time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants
@mraleph @vsmenon @eernstg @jonataslaw and others