dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.21k stars 1.57k forks source link

Error with private variable setters in mixins on dart web. #50119

Closed TimWhiting closed 1 year ago

TimWhiting commented 2 years ago

dart --version 2.19.0-266.0.dev, also on stable / beta channels web / chrome

Very similar issue to https://github.com/dart-lang/sdk/issues/44636.

Simplest reproduction so far: Edit: See @schultek's minimum sample below for a simpler reproduction.

void main() { final container = ProviderContainer(); final value = container.read(stringValue.state); querySelector('#output')?.text = 'Your ${value.state} app is running.'; }

final stringValue = StateProvider.autoDispose((ref) => 'Hello world');

- Add Riverpod to pubspec with `riverpod: ^2.0.0`
- Run sample with `webdev serve`
- Visit web app and check the console.

Stack overflow on private setter in a mixin. Making the field in the package public shifts the error to another private member.

....... at set [_keepAliveLinks] (auto_dispose.dart:42:7) at set [_keepAliveLinks] (auto_dispose.dart:42:7) at set [_keepAliveLinks] (auto_dispose.dart:42:7) at AutoDisposeProviderElementMixin. (auto_dispose.dart:6:24) at StateProviderElement_AutoDisposeProviderElementMixin$36. (base.dart:81:48) at new AutoDisposeStateProviderElement. (auto_dispose.dart:42:7) at AutoDisposeStateProvider.new.createElement (auto_dispose.dart:31:44) at [_create] (container.dart:49:32) at framework._StateReader.new.getElement (container.dart:41:52) at container.dart:434:37 at framework.ProviderContainer.new.readProviderElement (container.dart:466:14) at ProviderElementProxy.new.read (proxy_provider_listenable.dart:112:25) at framework.ProviderContainer.new.read (container.dart:257:20) at main$ (main.dart:25:26) at main.dart.bootstrap.js:273:10 at Array.forEach () at window.$dartRunMain (main.dart.bootstrap.js:272:32) at :1:8 at Object.runMain (client.js:8777:21)


Originally reported here: https://github.com/rrousselGit/riverpod/issues/1713

I tried creating a more minimal sample, but was unsuccessful.
TimWhiting commented 2 years ago

Same error doesn't exist with dart compile js

schultek commented 2 years ago

Here is a minimal reproducible snippet without riverpod (using the web-simple template):

// main.dart
import 'baz.dart';

void main() {
  var foo = Foo();
  print(foo.bar());
}

class Foo = Bar with Baz;
class Bar {}
// baz.dart
mixin Baz {
  int? _foo;

  void bar() {
    _foo = 100;
  }
}

The error happens only, when the Baz mixin is defined in a different library than Foo.

tomas-carv-com commented 2 years ago

Any update on this ?!

kevmoo commented 1 year ago

I'm "engaging" with the engineers now. Thanks for your patience!

RandalSchwartz commented 1 year ago

Upvoting this because this is a blocking issue with a client.

leehack commented 1 year ago

I narrowed down a little more from @schultek's example. So basically, the issue occurs with this syntax when it's used with Mixin: class Foo = Bar with Baz

// main.dart
import 'baz.dart';

void main() {
  var foo = Foo();
  var fooWork = FooWork();
  fooWork.bar();
  foo.bar();
}

class Foo = Bar with Baz; // crash
class FooWork extends Bar with Baz{} // doesn't crash

class Bar {}
// baz.dart
mixin Baz {
  int? _foo;

  void bar() {
    _foo = 100;
  }
}
kevmoo commented 1 year ago

@leehack – did that patch to riverpod fix the issue?

leehack commented 1 year ago

@kevmoo yep. It's tested and merged in the main, but I wish it to be fixed from dart anyway. I think the syntax makes the code cleaner. Not sure how widespread the syntax is, but maybe there are many packages impacted by this issue.

RandalSchwartz commented 1 year ago

Also, it'd be nice for that syntax to also be documented somewhere. The first I saw it was in the RiverPod code, and Remi claims that he found it by going through the compiler sources. :)

annagrin commented 1 year ago

Thank you @TimWhiting and other for reporting the issue and providing a great repro! I am looking into this currently, apologies for the delay.

Maybe related: https://github.com/dart-lang/sdk/issues/46867

Looks like this is the culprit DDC-generated code that causes the stack overflow. Will see how it can be fixed:

class AutoDisposeFutureProviderElement extends FutureProviderElement_AutoDisposeProviderElementMixin$36 {
      static ['_#_#tearOff'](T, provider) {
        return new (future_provider.AutoDisposeFutureProviderElement$(T)).__(provider);
      }
      get [_keepAliveLinks]() {
        return this[_keepAliveLinks]; // Calling itself here!
      }
      set [_keepAliveLinks](value) {
        return this[_keepAliveLinks] = value;
      }
      get [_maintainState]() {
        return this[_maintainState];
      }
      set [_maintainState](value) {
        return this[_maintainState] = value;
      }
      get maintainState() {
        return super.maintainState;
      }
      set maintainState(value) {
        return super.maintainState = value;
      }
      keepAlive() {
        return super.keepAlive();
      }
      mayNeedDispose() {
        return super.mayNeedDispose();
      }
      runOnDispose() {
        return super.runOnDispose();
      }
    }
annagrin commented 1 year ago

@leehack thank you for the great repro! Here is where the fault lies. I'll post updates on my progress.

main.dart.lib.js from @leehack 's example
```js /// Generated by DDC, the Dart Development Compiler (to JavaScript). // Version: 2.19.0-319.0.dev (dev) (Tue Oct 18 17:25:31 2022 -0700) on "macos_x64" // Module: packages/hello_world/main.dart // Flags: soundNullSafety(true), enableAsserts(true) define(['dart_sdk', 'packages/hello_world/baz.dart'], (function load__packages__hello_world__main_dart(dart_sdk, packages__hello_world__baz$46dart) { 'use strict'; const core = dart_sdk.core; const dart = dart_sdk.dart; const dartx = dart_sdk.dartx; const baz = packages__hello_world__baz$46dart.baz; var main = Object.create(dart.library); dart._checkModuleNullSafetyMode(true); dart._checkModuleRuntimeTypes(false); const CT = Object.create({ _: () => (C, CT) }); var I = ["package:hello_world/main.dart"]; var _foo = dart.privateName(baz, "_foo"); main.Bar = class Bar extends core.Object { static ['_#new#tearOff']() { return new main.Bar.new(); } }; (main.Bar.new = function() { ; }).prototype = main.Bar.prototype; dart.addTypeTests(main.Bar); dart.addTypeCaches(main.Bar); dart.setLibraryUri(main.Bar, I[0]); const Bar_Baz$36 = class Bar_Baz extends main.Bar {}; (Bar_Baz$36.new = function() { baz.Baz[dart.mixinNew].call(this); }).prototype = Bar_Baz$36.prototype; dart.applyMixin(Bar_Baz$36, baz.Baz); main.Foo = class Foo extends Bar_Baz$36 { static ['_#new#tearOff']() { return new main.Foo.new(); } get [_foo]() { return this[_foo]; } set [_foo](value) { return this[_foo] = value; // CALLS ITSELF! } bar() { return super.bar(); } }; (main.Foo.new = function() { main.Foo.__proto__.new.call(this); ; }).prototype = main.Foo.prototype; dart.addTypeTests(main.Foo); dart.addTypeCaches(main.Foo); dart.setMethodSignature(main.Foo, () => ({ __proto__: dart.getMethods(main.Foo.__proto__), bar: dart.fnType(dart.void, []) })); dart.setGetterSignature(main.Foo, () => ({ __proto__: dart.getGetters(main.Foo.__proto__), [_foo]: dart.nullable(core.int) })); dart.setSetterSignature(main.Foo, () => ({ __proto__: dart.getSetters(main.Foo.__proto__), [_foo]: dart.nullable(core.int) })); dart.setLibraryUri(main.Foo, I[0]); const Bar_Baz$36$ = class Bar_Baz extends main.Bar {}; (Bar_Baz$36$.new = function() { baz.Baz[dart.mixinNew].call(this); }).prototype = Bar_Baz$36$.prototype; dart.applyMixin(Bar_Baz$36$, baz.Baz); main.FooWork = class FooWork extends Bar_Baz$36$ { static ['_#new#tearOff']() { return new main.FooWork.new(); } }; (main.FooWork.new = function() { main.FooWork.__proto__.new.call(this); ; }).prototype = main.FooWork.prototype; dart.addTypeTests(main.FooWork); dart.addTypeCaches(main.FooWork); dart.setLibraryUri(main.FooWork, I[0]); main.main = function main$() { let foo = new main.Foo.new(); let fooWork = new main.FooWork.new(); fooWork.bar(); foo.bar(); }; dart.trackLibraries("packages/hello_world/main.dart", { "package:hello_world/main.dart": main }, { }, '{"version":3,"sourceRoot":"","sources":["main.dart"],"names":[],"mappings":";;;;;;;;;;;;;;;;;;;;;;;;;EAkBW;;;;;;;;;;;;;;AAHL;;;+BAAK;;;AAAL;;;;;;EAAkB;;;;;;;;;;;;;;;;;;;;;;;;;;;;;EACW;;;;;AAP7B,cAAM;AACN,kBAAU;AACD,IAAb,AAAQ,OAAD;AACE,IAAT,AAAI,GAAD;EACL","file":"../../../../../../../../packages/hello_world/main.dart.lib.js"}'); // Exports: return { main: main }; })); //# sourceMappingURL=main.dart.lib.js.map ```
annagrin commented 1 year ago

Update: found the culprit - we haven't marked the mixin declaration class fields as virtual, so overrides were not working correctly. Testing out the fix currently.

hacker1024 commented 1 year ago

Will this get included in a hotfix release of Dart and Flutter? It's quite a serious issue. I myself ran into it in my large Flutter app that does not use Provider.

annagrin commented 1 year ago

@hacker1024 I am working on it, will update on if and when!