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

The exception thrown when calling current on a ListIterator before calling moveNext() when E is non-nullable is unclear #47757

Closed
Hixie opened this issue Nov 23, 2021 · 4 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-duplicate Closed in favor of an existing report library-core type-enhancement A request for a change that isn't a bug

Comments

@Hixie
Copy link
Contributor

Hixie commented Nov 23, 2021

I would expect the following code to throw an exception like StateError with a message like cannot read current value of an iterator before the moveNext() method has been called at least once or some such:

void main() {
  <int>[1].iterator.current;
}

Instead the error is:

type 'Null' is not a subtype of type 'int' in type cast
#0      ListIterator.current (dart:_internal/iterable.dart:330:29)
#1      main (file:///home/ianh/dev/scratch/test.dart:2:21)
#2      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:297:19)
#3      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:192:12)

...which is rather misleading.

@lrhn
Copy link
Member

lrhn commented Nov 29, 2021

This is another one of the cases where we optimized for speed in the migration of iterators.

In Dart 2, you must not call the current getter of Iterable before calling moveNext() at least once, or after moveNext() has returned false or has thrown.
If you do so anyway, it's unspecified behavior, unless a specific iterable documents its iterator in more detail.

Because some legacy code depended on the old behavior, where current returned null when not having current value, we tried to make the code still do that in unsound null safe mode, where possible, and throw in sound null safe mode when then iterator value type was non-nullable.

So, the code you wrote will return null when compiled in unsound null safe mode. It will also try to return null in sound null safety mode, using the same code path (we don't have separate code paths for the two modes), which is why it gets caught in a type check.

The code for ListIterator.current is:

  E? _current;
  E get current => _current as E;

In unsound null safe mode, the as E is effectively doing as E? and becomes a no-op.
In sound null safe mode, the as E can be recognized and implemented as "check if E is nullable, if not check if _current is null", where I hope we have an efficient internal way to check if a type is nullable, and can avoid doing the more expensive subtype check.

The current getter implementation is important for the performance of iteration.

I'm not aware of any efficient rewrite which works the same in both sound and unsound null safe mode (suggestions accepted!)
We could wrap it in try/catch and throw a different error if the cast throws, but that very much sounds like something that would prevent a lot of optimizations (like inlining the current, which might be possible if you statically know that your class returns a ListIterable).

@lrhn lrhn added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core type-enhancement A request for a change that isn't a bug labels Nov 29, 2021
@Hixie
Copy link
Contributor Author

Hixie commented Dec 1, 2021

The solution we usually use in Flutter is asserts; that way you get a pretty error message in debug mode, and in release mode you get whatever you need to get to optimize for performance.

@DanTup
Copy link
Collaborator

DanTup commented May 14, 2024

(This might be a dupe of #45650?)

@lrhn
Copy link
Member

lrhn commented May 14, 2024

Does seem so. I don't know which one is more general, but let's close this one as the dupe.

@lrhn lrhn closed this as not planned Won't fix, can't repro, duplicate, stale May 14, 2024
@lrhn lrhn added the closed-duplicate Closed in favor of an existing report label May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-duplicate Closed in favor of an existing report library-core type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

3 participants