Inconsistent behavior when modifying during iteration

[matanui159]

Info:

• Dart SDK Version: Dart VM version: 2.1.0 (Tue Nov 13 18:22:02 2018 +0100) on
  "windows_x64"
• OS: Windows 10 64-bit
• Browser: tested on chrome, probably happens on all

---

Given the example code:

void main() {
   var x = [1, 2, 3];
   for (var y in x) {
      print(y);
      if (y == 2) {
         x.remove(y);
      }
   }
}

On webdev serve/dartdevc it outputs:

1
2

and it skips 3

On webdev build/webdev serve --release/dart2js it outputs:

1
2
Uncaught Error: Concurrent modification during iteration: Instance of 'minified:n<int>'.

This sort of inconsistent behavior is an issue during development as it makes bugs and crashes go pass into production unnoticed (which happened to me while developing/releasing a game).

[lrhn]

We don't generally promise to report all concurrent modifications. In many cases, it's a "best effort" approach, and some compilers remove the concurrent modification checks to improve performance.

That does lead to situations like this, where one platform lets a concurrent modification pass, and another catches it.
The development compiler should probably be the most strict here.

I can't see why the dev-compiler should not throw in this situation.
Looking at the source code, the iterator should throw at this point.

If the compiler somehow rewrites the iteration to not use the iterator, then it probably decided to not insert the modification check in that case.

[matanui159]

This seems to be the output code for a non-release build:

define(['dart_sdk'], function(dart_sdk) {
  'use strict';
  const core = dart_sdk.core;
  const _interceptors = dart_sdk._interceptors;
  const dart = dart_sdk.dart;
  const dartx = dart_sdk.dartx;
  const _root = Object.create(null);
  const main = Object.create(_root);
  const $remove = dartx.remove;
  let JSArrayOfint = () => (JSArrayOfint = dart.constFn(_interceptors.JSArray$(core.int)))();
  main.main = function() {
    let x = JSArrayOfint().of([1, 2, 3]);
    for (let y of x) {
      core.print(y);
      if (y === 2) {
        x[$remove](y);
      }
    }
  };
  dart.trackLibraries(/* lots of unimportant stuff */);
  // Exports:
  return {
    main: main
  };
});

By the way it has sort of bugged me about Dart that there are two compilers, one for development and a completely different one for release. Wouldn't these kind of issues be alot less common if there was only a single compiler for development and release?

[zoechi]

@matanui159 https://webdev.dartlang.org/tools/dartdevc might provide the info to make it more clear why there are two Dart-to-JS compilers.

[jmesserly]

there's another example in #36349

[vsmenon]

I believe this only happens when iterating on a JsArray (which we usually default to for literals). E.g., replacing [1, 2, 3] with [1, 2, 3].cast() triggers the error. We're missing the check in the former case - probably because we're directly using JS iteration.

[nshahan]

@lrhn It appears that the behavior is unspecified

sdk/sdk/lib/core/iterator.dart

Lines 12 to 13 in 40dfd1b

	/// If the object iterated over is changed during the iteration, the
	/// behavior is unspecified.

Should this test in the corelib suite be updated or removed?
https://github.com/dart-lang/sdk/blob/main/tests/corelib/list_concurrent_modify_test.dart

[lrhn]

While behavior for iteration in general is unspecified, it's OK for us to decide how the platform libraries should behave. Or at least keep regression tests to show us if we change it inadvertently.

I wouldn't remove the test.

I believe some compilers might have moved concurrent modification checks into asserts, in which case we should perhaps just always enable asserts for the test.

Development compilers should generally not disable checks, they should always be stricter than production mode.