Immutable collections are not recognized by analyzer

[dmitry-fbm]

const collections are supposed to be immutable; however it's possible to write code that modifies them, and errors happen only at runtime. Moreover, the errors are different for different collection types:

    const Map<int, String> m = {1: "const"};
    m[1] = "modified";
    // Uncaught Error, error: Error: Unsupported operation: Cannot modify unmodifiable map

    const List<int> l = [1];
    l[0] = 2;
    // Uncaught Error, error: Error: Unsupported operation: indexed set

[ykmnkmi]

In my wish list for 4.0 version. Immutable by default. More interfaces like List (immutable list), FixedLengthList (fixed size, can change values, dart-lang/language#2477), MutableList.

[lrhn]

Nothing in the current language allows an error for this program. The type List allows the operation, and it's possible to implement a constant object which implements List and won't throw on write operations.

So, in this particular case, the only thing that would make one expect an error is knowing the exact implementation class of the constant object, by following the data flow from where it was created to where it's attempted modified, and knowing the behavior of its mutating operations.

That sounds more like a job for the Analyzer than the language, do the issue pregnant belongs in the SDK repo.

[dmitry-fbm]

Nothing in the current language allows an error for this program. The type List allows the operation, and it's possible to implement a constant object which implements List and won't throw on write operations.

I'm sorry but this contradicts with actual behavior: const collections can't be modified at runtime, throwing the errors. Also language documentation clearly says:

To create a {list | set | map} that's a compile-time constant, add const before the list literal:

Compile-time constants are immutable by definition.

That sounds more like a job for the Analyzer than the language, do the issue pregnant belongs in the SDK repo.

Well this is what issue title says. Sorry if posted it into wrong place - hopefully someone may fix that, or I can repost.

[bwilkerson]

The case given above is a special case of a broader issue. The operator []= isn't the only way to modify the value of an object (for example, setters, methods, and the assignment operators) and many object can be made const where they are created. Hence, there are lots of ways to write code that will fail at runtime because of attempting to modify a const value.

The general case isn't really something we can support because the analyzer doesn't currently have any way to track the const-ness of an object. Tracking the const-ness would require support for flow analysis and probably a whole-world analysis, neither of which is something we're likely to add to the analyzer in the near term and maybe not ever.

The same problems hinder us from supporting this for the limited case of list, map, and set literals, though we could potentially do this for assignments to local variables that are only assigned once and possibly for final or const fields (that are also guaranteed to be assigned exactly once). I have to wonder about the value of a diagnostic for such a limited set of cases.

So, while I'd love for us to be able to produce a static diagnostic for this class of error, I don't think we reasonable can.

[dmitry-fbm]

@bwilkerson thanks for the explanation, indeed makes sense. Cannot this be done the opposite way? While there are many ways to modify objects, and those highly depend on each class' nature, ways to read their values/properties are more limited and common. Perhaps you can allow only read access instead of disabling modifications?

[bwilkerson]

I'm probably missing something, but that sounds equivalent to me. Both appear to require knowing

• whether an object is known to be mutable and
• whether the operation will attempt to modify the state of the object.

But maybe I didn't understand what you were proposing.

[dmitry-fbm]

I'm probably missing something, but that sounds equivalent to me

It is, more or less - just thinking that in such case the analysis would require less cases to cover, and these will be more generic - thus might be easier to implement. I could be wrong though.

Ok what about compile-time errors? Sounds much easier to me; the compiler definitely knows the object is a constant, and therefore can throw an error on every attempt to modify it, no mater how.

[bwilkerson]

Ok what about compile-time errors?

The analyzer is essentially the front-end of the compiler, but enhanced to produce both warnings and lints in addition to all the normal compile-time errors.

... the compiler definitely knows the object is a constant, and therefore can throw an error on every attempt to modify it, no mater how.

The compiler doesn't know whether an object at some invocation site will be a constant object or not because there can be multiple execution paths leading to that invocation site.

Consider:

void main() {
  f([2]);
  f(const [5]);
}

void f(List<int> list) {
  list[0] = 3;
}

The best it could do is try to identify places where a constant value definitely can't be passed in, but that requires global data-flow analysis, and even then might be impossible to get 100% right (I'd have to think about it for a bit to come up with an example).

[eernstg]

I think it's fair to mention that it is not difficult to model mutability as a property which is reflected by the static type. Here's an example with a Box data structure. This is probably the simplest possible collection type, but it is enough to show the underlying idea:

import 'dart:math';

abstract class Box<X extends Object> {
  X get value;
  const Box();
  bool get isEmpty;
}

class ReadOnlyBox<X extends Object> implements Box<X> {
  final X value;
  const ReadOnlyBox(this.value);
  get isEmpty => false;
}

class ReadWriteBox<X extends Object> implements Box<X> {
  X? _value;
  ReadWriteBox(): _value = null;
  get isEmpty => _value != null;
  set value(X newValue) => _value = newValue;
  X get value => _value!;
}

void main() {
  final roBox = ReadOnlyBox(1);
  final rwBox = ReadWriteBox<int>()..value = 2;
  final box = Random().nextBool() ? roBox : rwBox;

  void show() => print([roBox.value, rwBox.value, box.value]);

  // All boxes can read the value.
  show();

  // Only read/write boxes can set a new value.
  rwBox.value = 3; // OK!
  //roBox.value = 4; // Compile-time error.
  //box.value = 5; // Compile-time error.
  show();

  // It can be tested at run time which kind of box we have.
  if (box is ReadWriteBox<int>) {
    box.value = 100; // OK: `box` now has type `ReadWriteBox<int>`.
    show();
  }
}

Note that the plain Box class has the read-only interface, because some subtypes only have those members, but the read-write interface has some additional members.

Also note that the Box interface allows clients to avoid mutating the box, even in the case where it is a ReadWriteBox, but you need to use a type like ReadOnlyBox in order to know for sure that it won't be mutated by someone else who has a reference of type ReadWriteBox to the same object.

Hence, it's not that we can't capture the relationship between an interface that enables read-only access to certain instances (e.g., of a collection type) and an interface that also supports mutation. That's a straightforward design choice, at this minimalistic level.

However, the implications for larger scale development are less clear: You do get the benefit of better static type checking with the separated read-only/read-write interfaces, but you also get an obligation to choose one or the other in a huge number of locations in code (because collections are used everywhere), which makes any modification from read-only to read-write a potentially substantial breaking change.

The approach which was chosen in Dart many years ago was to have a single interface for each of the collections rather than a separate read-only and read-write interface. The choice could have been different today (where Dart puts a substantially greater emphasis on static type checking), but it would be a hugely breaking change to switch from the dynamically checked to the statically checked approach today (that is, introducing read-only supertypes to all the existing read-write interfaces, connecting them properly in the subtype hierarchy, and then the big one: using them properly everywhere).

Note that you do have several ways to move in the direction of statically specified immutability: You could consider using built_value, whose collections are always immutable. You could also choose to access some collections using an extension type that doesn't have the mutating members:

import 'dart:math';

class OldBox<X extends Object> {
  X? _value;
  OldBox(): _value = null;
  get isEmpty => _value != null;
  set value(X newValue) => _value = newValue;
  X get value => _value!;
}

extension type const Box<X extends Object>._(OldBox<X> _box) {
  X get value => _box.value;
  bool get isEmpty => _box.isEmpty;
}

extension type ReadOnlyBox<X extends Object>._(OldBox<X> _box)
    implements Box<X> {
  ReadOnlyBox(X x): this._(OldBox<X>()..value = x);
}

extension type ReadWriteBox<X extends Object>._(OldBox<X> _box)
    implements Box<X> {
  ReadWriteBox(): this._(OldBox<X>());
  get isEmpty => _box.isEmpty;
  set value(X newValue) => _box._value = newValue;
  X get value => _box.value;
}

void main() {
  final roBox = ReadOnlyBox(1); // Could be `const`, too.
  final rwBox = ReadWriteBox<int>()..value = 2;
  final box = Random().nextBool() ? roBox : rwBox;

  void show() => print([roBox.value, rwBox.value, box.value]);

  // All boxes can read the value.
  show();

  // Only read/write boxes can set a new value.
  rwBox.value = 3;
  //roBox.value = 4; // Compile-time error.
  //box.value = 5; // Compile-time error.
  show();

  // With extension types, run-time type tests rely on the
  // underlying representation object, so it is _always_
  // revealed that `box` is a read-write capable object.
  if (box is ReadWriteBox<int>) {
    box.value = 100;
    show();
  }
}

Extension types are compile-time only types (they are erased at run time). This implies that they have zero cost (there is no wrapper object around the underlying object of type OldBox, and member invocations can be inlined). On the other hand, a type test at run time (like box is ReadWriteBox<int>) will rely on the underlying type (here: OldBox<int>), so you can't "rediscover" the type: If it's gone then it's gone, and we can't know whether this object used to be accessed via a reference of type ReadOnlyBox<int> or ReadWriteBox<int>, because it's always an OldBox<int> at run time.

In other words, if you want to impose a special discipline on the members that you are using on a given object (for example: "this collection should be used in a read-only manner") then you can use an extension type to do that. You just need to remember that these types only exist at compile-time, and they shouldn't be combined with run-time type queries such as e is T, or e as T, or even Object? o = anExpressionWhoseTypeIsAnExtensionType;.

All in all, you have one extreme: compile-time only types, and another extreme: run-time only immutability, in addition to the standard Dart approach (where immutability is mostly an informal convention), plus finally some approaches that have a different take altogether (such as built_value, where every collection is immutable).

In any case, @dmitry-fbm, I hope you can find an approach that fits your purpose and style!

[dmitry-fbm]

@eernstg thanks so much for such detailed response. Though, would you be so kind to elaborate on this point a bit further?

it would be a hugely breaking change to switch from the dynamically checked to the statically checked approach today

Honestly am not quite getting what such change may break, as code that attempts to modify const objects is already broken, it will fail at runtime; pointing out to such bugs at compile time would definitely be beneficial.

As for workarounds you offered - thanks again, I admit they exist. However all add severe cognitive overhead, as compared to plain const Object declaration - so having static or compile-time analysis added would be much preferred.

[bwilkerson]

Given the complexity of the work, I'm going to close this as not-planned.

[eernstg]

@dmitry-fbm wrote:

Honestly am not quite getting what such change may break

Well, I'm generally on the side that favors adding a read-only interface to the collection type hierarchy, so I won't be able to make a huge case against it.

However, it does give rise to added complexity, especially because millions of member signatures in existing code will now have the option to use a read-only type or a read-write type, and the choice to use a read-only type will propagate to all dependencies (method A.m1 accepts a read-only list and passes it on to B.m2, so we must migrate B before A). This is the same kind of mass migration that we just had when null-safety was introduced, and I'm not sure we as a community are ready for another round of a similar magnitude. Moreover, we'd want to use objects that are actually read-only when the migration of all the member signatures (and other API elements) is complete, such that you don't have to worry about if (myList is List<int>) myList.add(42); where myList has type ReadOnlyList<int>.

Considerations along these lines are discussed in several places in the Dart github issue trackers, for instance in this comment.

So it's probably fair to say that it is about the migration effort at least as much as it is about regular, hard breakage.