dart-lang / language

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

Enforce strict method override behavior in Dart #4018

Open TomaszCz opened 2 months ago

TomaszCz commented 2 months ago

Currently, Dart allows methods marked with @override to be declared with different return types, including asynchronous versions of methods that are expected to be synchronous. This can lead to runtime exceptions in frameworks like Flutter, where certain lifecycle methods must be strictly synchronous. Enforcing stricter method override behavior at compile-time would prevent these issues.

Imagine method declaration:

  @protected
  @mustCallSuper
  void initState() {
    assert(_debugLifecycleState == _StateLifecycle.created);
    if (kFlutterMemoryAllocationsEnabled) {
      FlutterMemoryAllocations.instance.dispatchObjectCreated(
        library: _flutterWidgetsLibrary,
        className: '$State',
        object: this,
      );
    }
  }

Now it is fine to override it like this: ( and correct )

 @override
  void initState() {
    super.initState();
    // Synchronous initState
  }

but we can also write sth like this ( incorrect! could lead to flutter build method executed before initState finished ... )

  @override
  Future<void> initState() async {
    print('init state method called');
    super.initState();
    await Future<void>.delayed(const Duration(seconds: 5));
    print('init state method finished');
  }

Expected Behavior:

The Dart compiler should enforce stricter method override rules, producing a compilation error if an overridden method's return type differs from the expected type. This ensures that methods expected to be synchronous (e.g., initState) cannot be overridden as asynchronous methods.

Actual Behavior:

Methods marked with @override can be declared with different return types, including asynchronous versions, leading to runtime exceptions in frameworks like Flutter.

Additional Context:

Enforcing stricter method override behavior in Dart would align with best practices and prevent runtime exceptions caused by incorrect method overrides. This change would improve the developer experience and ensure that methods expected to be synchronous remain synchronous when overridden.

Steps to Reproduce:

  1. Create a new Flutter project. flutter create test.
  2. Implement a stateful widget with an asynchronous initState method, which is supposed to be synchronous:
  3. Run the app and observe the runtime exception:

image

lrhn commented 2 months ago

The problem isn't changing the return type, it's changing it to a Future type. And that is because ignoreing a future is a potential problem. (In this case, the error suggests that the problem is that you wrote async, but the thing it actually detects is that the call returns a Future.)

If you return a future, and the calling code does not expect that, the future will not be awaited. And that's potentially bad, since the future completing with an error can take down the entire application. (Or it might be irrelevant because the future never completes with an error, and is only there in case you want to know when some operation has finished, but it's impossible to say from the available information.)

So this is a problem. It mainly occurs when overriding a void-returning function, because any other return type means someone will be looking at the value, and will likely notice the future. (Also because almost no function returns Object or Object?, because you can't use that value for anything anyway. Those could be overriddent ot return a Future too, but it's not as likely to be completely ignored as overriding something returning void.)

It's unlikely that Dart will disallow overriding with a subtype as return type in general. For example num.operator+ returns num, but double.operator+ returns double, and that's fine - a subtype of substitutable for the supertype. That's one of the premises of object oriented design.

The place to focus is either on overriding void, or on overriding a non-Future type with a Future type. Likely the latter, since returning a value to someone expecting void will just have the value ignored, and the kind of value that we can generally agree that you should not ignore is Future. (I guess CancelableOperation should also count, since it contains a future. So things containing futures may also be problems, but those are rare.)

If we don't change the language, which may be hard and breaking, maybe we can warn.

Not having a void function being async is already a lint, avoid_void_async.

There could be other lints for overriding void with Future too, if that seems useful. I'd start with avoiod_void_async, and be careful about not just changing the return type to Future<void> when the lint triggers. It's warning for a reason, it's not juts an annoyance to silence.

(None of those would help you here, since void initState() async { would give you the same runtime error in development mode, since it checks the returned value, not the return type, and void initiate() async {...} always returns a future value. So maybe extend the lint to also not have async functions with non Future/FutureOr return types.)

TomaszCz commented 2 months ago

Thank you for the detailed response.

I understand the complexities involved in changing the language itself, especially regarding backward compatibility and ecosystem impact. However, I'd like to highlight an inconsistency that arises due to Dart's transpilation into different languages across platforms.

For instance, Dart's compilation to C++ for the Windows platform enforces strict method override rules, preventing non-strict method overrides. It must be like that if you use https://en.cppreference.com/w/cpp/language/virtual ("A function with the same name but different parameter list does not override the base function of the same name, but hides it") - basically if you use build in C++ inheritance we have inconsistency, but now when i think of it, maybe you did your custom virtualisation and then it works. I don't really know. Anyway, on the other hand, platforms that compile Dart to JavaScript or use LLVM-based native code preserve Dart's current behaviour, allowing non-strict method overrides.

If you use build in c++ inheritance, then there is inconsistency which suggests a need for uniform enforcement of method override rules within Dart itself. While I will enable the avoid_void_async lint in my projects and propose new linting rules to help in the short term, I believe that addressing this at the language level would provide a more robust and consistent solution.

Would it be possible to raise a formal proposal to change this aspect of Dart's method override behavior?

mraleph commented 2 months ago

However, I'd like to highlight an inconsistency that arises due to Dart's transpilation into different languages across platforms.

For instance, Dart's compilation to C++ for the Windows platform enforces strict method override rules, preventing non-strict method overrides. [...] I don't really know. Anyway, on the other hand, platforms that compile Dart to JavaScript or use LLVM-based native code preserve Dart's current behaviour, allowing non-strict method overrides.

Dart does not have a compiler which emits C++ code nor a compiler which uses LLVM. Can you clarify what you are trying to say here?

All Dart implementations support overriding method returning void with method returning something else (e.g. a String or Future<String> etc), simply because of assignability rules. void in Dart is not an absence of value, but rather an special type which signals that value should not be used.

TomaszCz commented 2 months ago

Oh, you are right, please ignore this part, I am still learning dart, I saw some native C++ code in 'windows' folder and I got it completely wrong. I see this code is part of the Flutter engine and platform-specific integrations and not transpiled version of dart.