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

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

Closed
TimWhiting opened this issue Oct 3, 2022 · 14 comments
Closed

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

TimWhiting opened this issue Oct 3, 2022 · 14 comments
Assignees
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. P1 A high priority bug; for example, a single project is unusable or has many test failures web-dev-compiler web-triage-0 repro is available

Comments

@TimWhiting
Copy link

TimWhiting commented Oct 3, 2022

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

Very similar issue to #44636.

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

  • dart create -t web-simple call_stack_overflow && cd call_stack_overflow
  • Replace main with the following
import 'dart:html';
import 'package:riverpod/riverpod.dart';

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.<computed> (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 (<anonymous>)
    at window.$dartRunMain (main.dart.bootstrap.js:272:32)
    at <anonymous>:1:8
    at Object.runMain (client.js:8777:21)

Originally reported here: rrousselGit/riverpod#1713

I tried creating a more minimal sample, but was unsuccessful.

@TimWhiting
Copy link
Author

Same error doesn't exist with dart compile js

@lrhn lrhn added the area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. label Oct 3, 2022
@schultek
Copy link

schultek commented Oct 3, 2022

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
Copy link

Any update on this ?!

@kevmoo
Copy link
Member

kevmoo commented Oct 25, 2022

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

@annagrin annagrin added the P0 A serious issue requiring immediate resolution label Oct 26, 2022
@annagrin annagrin self-assigned this Oct 26, 2022
@annagrin annagrin added P1 A high priority bug; for example, a single project is unusable or has many test failures and removed P0 A serious issue requiring immediate resolution labels Oct 26, 2022
@RandalSchwartz
Copy link

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

@leehack
Copy link

leehack commented Oct 27, 2022

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
Copy link
Member

kevmoo commented Oct 27, 2022

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

@leehack
Copy link

leehack commented Oct 27, 2022

@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
Copy link

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
Copy link
Contributor

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: #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
Copy link
Contributor

annagrin commented Oct 27, 2022

@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
/// 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
Copy link
Contributor

annagrin commented Nov 1, 2022

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
Copy link
Contributor

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
Copy link
Contributor

annagrin commented Nov 7, 2022

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

copybara-service bot pushed a commit that referenced this issue Nov 11, 2022
Generate calls to super class methods from mixin application
stubs using `super` instead of `this`.

Bug: #50119
Change-Id: I02802ea828d72d9221f1886195acb91e6c9520ac
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/267822
Fixed:#50415
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/268604
Reviewed-by: Sigmund Cherem <[email protected]>
Reviewed-by: Joshua Litt <[email protected]>
copybara-service bot pushed a commit that referenced this issue Nov 22, 2022
Generate calls to super class methods from mixin application
stubs using `super` instead of `this`.

Bug: #50119
Change-Id: I87a851579d94d8c497985ee02babb413f56fad21
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/267822
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/268609
Commit-Queue: Alexander Thomas <[email protected]>
Reviewed-by: Sigmund Cherem <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. P1 A high priority bug; for example, a single project is unusable or has many test failures web-dev-compiler web-triage-0 repro is available
Projects
None yet
Development

No branches or pull requests

10 participants