dart-archive / observe

Support for marking objects as observable, and getting notifications when those objects are mutated
https://pub.dartlang.org/packages/observe
BSD 3-Clause "New" or "Revised" License
13 stars 6 forks source link

_ObservedSet.close() tries to iterate a map #85

Closed munificent closed 8 years ago

munificent commented 8 years ago

I'm adding better static analysis of for-in loops to analyzer, and I discovered an error in observe. In path_observer.dart:

class _ObservedSet {
  // ...

  // Dart note: the JS implementation is O(N^2) because Array.indexOf is used
  // for lookup in this array. We use HashMap to avoid this problem. It
  // also gives us a nice way of tracking the StreamSubscription.
  Map<Object, StreamSubscription> _objects;

  void close(_Observer obs) {
    // ...

    if (_objects != null) {
      for (var sub in _objects) sub.cancel(); // <-- Line 863.
      _objects = null;
    }

    // ...
  }

Map doesn't implement Iterable, so that's not going to do anything good (and now it generates a static warning). You probably want _objects.value.

sigmundch commented 8 years ago

I might be missing something obvious, can you clarify? The only iteration I see is for _objects, which is a list.

munificent commented 8 years ago

_observers is a list, _objects is a map.

sigmundch commented 8 years ago

Sorry, thanks for clarifying, I mixed those up when looking at the original definition.

I however see the implementation going through _objects.values here: https://github.com/dart-lang/observe/blob/master/lib/src/path_observer.dart#L854?

jmesserly commented 8 years ago

We sent Jake back in time to fix that bug in April, 2015 :) https://github.com/dart-lang/observe/commit/354778b4d7e5629d6f1feae1f13c307a971fe761

jmesserly commented 8 years ago

I'm going to tentatively close as this looks like it was fixed and more recent versions have been published...

munificent commented 8 years ago

Heh, oops! Looks like the observatory folks need to upgrade to the latest observe package in the Dart repo then.

cc @johnmccutchan