Skip to content

Customer-reported issue with NNBD migration tool #44345

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

Closed
escamoteur opened this issue Nov 30, 2020 · 8 comments
Closed

Customer-reported issue with NNBD migration tool #44345

escamoteur opened this issue Nov 30, 2020 · 8 comments
Labels
area-migration (deprecated) Deprecated: this label is no longer actively used (was: issues with the `dart migrate` tool). NNBD Issues related to NNBD Release P2 A bug or feature request we're likely to work on

Comments

@escamoteur
Copy link

Hi,
I'm stuck while trying to port my package functional_listener to NNB.
you can find the curenty state here:

https://github.com/escamoteur/functional_listener/tree/null-safety

Working with the tool was fine, but now not all tests work and even compile anymore what they did before.
The tool again added some strange down casts on lambda functions.

I have no idea why the `debounceTest' doesn't work anymore.

some of the tests threw late initialization errors

Please have a look at it and tell me what to do. The code is easy to overview.

  • Dart SDK Version (dart --version)
    Dart SDK version: 2.12.0-76.0.dev (dev) (Wed Nov 25 05:22:28 2020 -0800) on "windows_x64"
[√] Flutter (Channel master, 1.24.0-8.0.pre.375, on Microsoft Windows [Version 10.0.19041.630], locale de-DE)
    • Flutter version 1.24.0-8.0.pre.375 at C:\Entwicklung\Flutter
    • Framework revision a7f5fd5360 (5 hours ago), 2020-11-30 13:14:13 +0100
    • Engine revision 20caf54969
    • Dart version 2.12.0 (build 2.12.0-76.0.dev)
@lrhn lrhn added the area-migration (deprecated) Deprecated: this label is no longer actively used (was: issues with the `dart migrate` tool). label Nov 30, 2020
@stereotype441 stereotype441 added the P2 A bug or feature request we're likely to work on label Dec 1, 2020
@mit-mit mit-mit added the NNBD Issues related to NNBD Release label Dec 1, 2020
@mit-mit
Copy link
Member

mit-mit commented Dec 1, 2020

I see three tests failing:

'Where Test'

This fails with:

00:12 +3 -1: Where Test [E]
  type '(dynamic) => dynamic' is not a subtype of type '(int) => bool' in type cast
  listenable_pipe_test.dart 68:34  main.<fn>

Changing:

    final subscription = listenable
        .where(((x) => x.isEven) as bool Function(int))
        .listen((x, _) => destValues.add(x));

to

    final subscription =
        listenable.where((x) => x.isEven).listen((x, _) => destValues.add(x));

seems to resolve this.

'mergeWith Test'

This fails with:

00:07 +5 -2: mergeWith Test [E]
  LateInitializationError: internalHandler
  package:functional_listener/src/functional_value_notifiers.dart         FunctionalValueNotifier.internalHandler
  package:functional_listener/src/functional_value_notifiers.dart 20:38   FunctionalValueNotifier.removeListener
  package:functional_listener/src/functional_value_notifiers.dart 140:11  MergingValueNotifiers.removeListener
  package:functional_listener/functional_listener.dart 223:17             ListenableSubscription.cancel
  listenable_pipe_test.dart 160:18                                        main.<fn>

As described on https://api.dart.dev/stable/2.10.4/dart-core/LateInitializationError-class.html, this is thrown when

  1. a late variable is read before its been initialized, or

  2. when a late final variable is written to twice. Given handler is not final, looks like we're in the case 1). (note: I think the error could be more clear here; filed Explain why a LateInitializationError was thrown #44361)

Looking at the stacktrack, looks like the failure is in the call to removeListener here:

  @override
  void removeListener(VoidCallback listener) {
    super.removeListener(listener);
    if (!hasListeners) {
      previousInChain.removeListener(internalHandler);
    }
  }

I don't fully understand the code, but should that if test be unnegated (i.e. if hasListeners is true, then remove the listener)? The tests seem to pass if I change if (!hasListeners) to if (hasListeners).

'Debounce Test'

The error is:

LateInitializationError: debounceTimer

When the exception was thrown, this was the stack:
#0      DebouncedValueNotifier.debounceTimer (package:functional_listener/src/functional_value_notifiers.dart)
#1      new DebouncedValueNotifier.<anonymous closure> (package:functional_listener/src/functional_value_notifiers.dart:73:7)
#2      ChangeNotifier.notifyListeners (package:flutter/src/foundation/change_notifier.dart:243:25)
#3      ValueNotifier.value= (package:flutter/src/foundation/change_notifier.dart:309:5)
#4      main.<anonymous closure> (file:///Users/mit/Downloads/functional_listener-null-safety2/test/listenable_pipe_test.dart:92:16)
#5      main.<anonymous closure> (file:///Users/mit/Downloads/functional_listener-null-safety2/test/listenable_pipe_test.dart:84:25)
#6      Declarer.test.<anonymous closure>.<anonymous closure> (package:test_api/src/backend/declarer.dart:200:19)

Looking at your version before null safety, you had:

      debounceTimer?.cancel();
      debounceTimer =
          Timer(debounceDuration, () => value = previousInChain.value);
    };

And now:

      debounceTimer.cancel();
      debounceTimer =
          Timer(debounceDuration, () => value = previousInChain.value);

It looks like we call .cancel() on something that hasn't been initialized yet?

@escamoteur
Copy link
Author

@mit-mit I figured out what happens to the debounce and the mergeWidth test. it makes sense.

But in case of the whereTest, this cast was added by the migration tool. And the combineTest doesn't even compile anymore since I ran the migration.
thanks for looking at it.

@mit-mit
Copy link
Member

mit-mit commented Dec 2, 2020

@stereotype441 can you comment on this concrete migration question in whereTest? The code is:

final listenable = ValueNotifier<int>(0);

final destValues = <int>[];
final subscription =
    listenable.where((x) => x.isEven).listen((x, _) => destValues.add(x));

The migration tool suggests:

final subscription =
      listenable.where(((x) => x.isEven) as bool Function(int)).listen((x, _) => destValues.add(x));

Which causes:

00:12 +3 -1: Where Test [E]
  type '(dynamic) => dynamic' is not a subtype of type '(int) => bool' in type cast
  listenable_pipe_test.dart 68:34  main.<fn>

@escamoteur
Copy link
Author

@mit-mit please also have a look at the combine Test which does not compile anymore after the migration

@mit-mit
Copy link
Member

mit-mit commented Dec 2, 2020

That one looks similar; the tool suggests:

 final subscription = listenable1
        .combineLatest<String , StringIntWrapper >(
            listenable2, ((i, s) => StringIntWrapper(s, i)) as StringIntWrapper Function(int, String))
        .listen((x, _) {
      destValues.add(x);
    });

@stereotype441
Copy link
Member

stereotype441 commented Dec 2, 2020

@stereotype441 can you comment on this concrete migration question in whereTest? The code is:

final listenable = ValueNotifier<int>(0);

final destValues = <int>[];
final subscription =
    listenable.where((x) => x.isEven).listen((x, _) => destValues.add(x));

The migration tool suggests:

final subscription =
      listenable.where(((x) => x.isEven) as bool Function(int)).listen((x, _) => destValues.add(x));

Which causes:

00:12 +3 -1: Where Test [E]
  type '(dynamic) => dynamic' is not a subtype of type '(int) => bool' in type cast
  listenable_pipe_test.dart 68:34  main.<fn>

This is a bug in the migration tool. It was fixed in e047dd6.

@escamoteur
Copy link
Author

Thanks, I solved everything so far. I also migrated the example which was fine. Only thing I realized was the tool added a ´dynamic´
image
And the analyser is not able to infer the correct type for s2

@stereotype441
Copy link
Member

As of 1c7fe71, the null safety migration tool has been removed from active development and retired. No further work on the tool is planned.

If you still need help, or you believe this issue has been closed in error, please feel free to reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-migration (deprecated) Deprecated: this label is no longer actively used (was: issues with the `dart migrate` tool). NNBD Issues related to NNBD Release P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

4 participants