The analyzer does not understand unmodifiable list

[adnanjpg]

This tracker is for issues related to:

• Analyzer

In your issue, please include:

• Dart SDK Version:
  • Dart SDK version: 2.16.1 (stable) (Tue Feb 8 12:02:33 2022 +0100) on "macos_x64"
• MacOSX Intel

let's go over an example:

import 'dart:math';

void main(List<String> arguments) {
  const foo = [1, 2, 3];

  final ran = Random();
  foo.shuffle(ran);
}

here, as clearly it is that foo cannot be modified as it is constant, the analyzer does not understand that and does not warn, so this bug is only catchable on runtime.

the output:

Unhandled exception:
Unsupported operation: Cannot modify an unmodifiable list
#0      UnmodifiableListMixin.shuffle (dart:_internal[/list.dart:154:5]())
#1      main
bin/bug.dart:7
#2      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch[/isolate_patch.dart:295:32]())
#3      _RawReceivePortImpl._handleMessage (dart:isolate-patch[/isolate_patch.dart:192:12]())

Exited (255)

[eernstg]

It could be useful to track the type of a list literal in such a way that the constant variable in this example is known to be an unmodifiable list. However, we do specify in the section 'List Literal Inference' that every list literal has a type of the form List<T> for some T:

In both cases, the static type of \metavar{list} is \code{List<$T$>}.

@lrhn, do you plan to introduce system classes that describe the behavior of an unmodifiable list, set, and map? Presumably, they would be superinterfaces of the current List, Set, and Map classes. We could then consider to make unmodifiable collections visible to the type system, starting with the static types of constant collection literals.

However, this does look like a topic that has been raised a number of times in the past, and it is not obvious how we'd do it, without causing hugely breaking changes...

[lrhn]

@lrhn, do you plan to introduce system classes that describe the behavior of an unmodifiable list, set, and map?

We have no plan to introduce an interface for the "non-modifying List" API.

Not a Unmodifiable marker interface that unmodifiable lists implement, and not a Sequence interface with only the non-modifying operations. (Two very different things, where Unmodifiable allows you to detect at runtime that a List isn't modifiable, and Sequence being a non-List interface that List implementes.)

The Unmodifiable isn't useful here. It's useful for, say, checking if an argument list is unmodifiable, so you don't need to make a clone to prevent against modifications, but won't affect static typing.

We made that decision a long time ago to not have a "read-only" interface for our collections, and haven't changed it since. We could reconsider, but that would be a big change to the libraries, and would potentially require a large non-trivial migration.

The usual argument against "read only" interfaces is that it makes code authors have to choose which interface to use in their APIs. Do you accept a (let's call it) Sequence<T> or a List<T> as argument? Do you produce a Sequence<T> or a List<T> as return value?
If someone decides to produce a Sequence<T> and someone else accepts a List<T>, then you can't pass the former to the latter.
If we change the platform libraries to accept Sequence where possible, that's non-breaking. Any place we change them to return Sequence instead of List, it's a breaking change. If we don't do that, there is little benefit from introducing the interface.

We double the complexity of the core Dart collections API for everybody.
And that's without introducing a FixedLengthList too, which is another restriction, just not as strict as "unmodifiable".
(And it won't actually help with argument list cloning, since a List is-a Sequence).

The advantages of having those interfaces would be that you won't get run-time errors when passing a fixed-length or unmodifiable list into something which does an add on it.
That has not been a big issue, possibly because almost all lists are growable lists anyway, and people are pretty good at knowing when a function modifies a list, because then it's part of the API of that function, and you pass in a list that you want to have modified.

All in all, the cost doesn't seem to match the benefit.

If the analyzer wants to detect that you're calling modifying methods on a constant list created by a constant list literal, they should have all the information needed.

[eernstg]

All in all, the cost doesn't seem to match the benefit.

Thought so. ;-)

If the analyzer wants to detect that you're calling modifying methods on a constant
list created by a constant list literal, they should have all the information needed.

This was the part that I was worried about: Noting that we won't make 'unmodifiable' for collections a property of the static type itself, the analyzer would need to add some "unofficial extras" to DartType or similar static analysis classes, e.g., remembering that this particular List<int> is one of those special lists that aren't modifiable. How far do we want to go down that path?

[lrhn]

How far do we want to go down that path?

However far the benefits outweigh the costs. If it brings benefit to users, and the benefit is worth the cost (and opportunity cost) of implementing and maintaining it, I see no reason to not make our tools provide special-cased warnings about specific misuses of API.

Calling add on a list which we can can know is actually the result of a constant list literal (or the result of List.unmodifiable, for that matter), is a guaranteed run-time error, and something the analyzer can always choose to warn about.

[adnanjpg]

It is easy to know that the const lists cant be modified, and it is easy to detect when you put the code in the way I've provided above in the issue's body

import 'dart:math';

void main(List<String> arguments) {
  const foo = [1, 2, 3];

  final ran = Random();
  foo.shuffle(ran);
}

however, it really gets out of track when you have some lines between the list initializer and the list modifier. and this issue gives the user one more thing to worry about and focus on

[eernstg]

@lrhn wrote:

something the analyzer can always choose to warn about.

Sure, lints and hints can do whatever they want, and 'warnings' might be treated similarly at some point. However, I made the distinction between properties that rely on specified information (in particular, static types), and properties that rely on non-specified information. The fact that both kinds of information must be propagated between program locations implies that any "extra" information will make common objects consume more memory (e.g., a DartType needs an extra field), or time must be spent looking up the extra information in some data structure on the side each time it might be needed.

Perhaps this is not a problem at all, but I thought it was at least a thing to keep in mind, which is the reason why I pushed a bit on the (impossible) idea that it could be maintained as a proper part of the static types.

In any case, we're just looking at this from the outside. @stereotype441, @scheglov, WDYT?

[stereotype441]

Writing a lint that can see the flaw in @adnanjpg's example would definitely be possible, and not too difficult. But most real-world examples of this sort of problem require synthesizing information from multiple functions across the program. Consider this example:

import 'dart:math';

class C {
  void foo() {
    var listOfInts = const [1, 2, 3];

    bar(listOfInts);
  }
  void bar(List<int> values) {
    print('$values');
  }
}
mixin M {
  void bar(List<int> values) {
    values.shuffle(Random());
  }
}
class D extends C with M {}
main() {
  D().foo();
}

This example has the same flaw, but in order to see that the flaw exists, you can't look at individual functions in isolation. You have to see that C.foo and M.bar are incompatible (because the former produces a const List and the latter only works with a non-const List), and then you have to see that since D mixes C and M together, it produces a way for those two incompatible methods to talk to one another.

Automating this sort of analysis is definitely possible, but it's tricky. I don't think we have any definitive plans to add this sort of functionality to the linter or the analyzer, though we have talked about things like this before. IIRC I've discussed ideas like this with @pq and @bwilkerson in the past. So they might want to weigh in with their thoughts.

[bwilkerson]

I think that sums it up fairly well.

The ability to detect at analysis time where runtime exceptions will occur is hugely valuable, but in this case the percentage of places where we could do so, given the performance constraints, is small enough that it probably isn't worth it.

We have discussed adding some kind of whole-program analysis (@srawlins), and this is a good example of what we'd be able to do with it, but there are no concrete plans at the moment for adding such a tool.

[MelbourneDeveloper]

@adnanjpg big +1 on this one.

The basic problem with immutable lists and Dart is that we don't see any problems at compile time. There are two ways that Dart could deal with the problem:

1. Analysis errors, as you mention
2. Create a more basic List<> class that is Iterable<> and only has a length property

Meantime, check out this package: fixed_collections

It just deprecates the mutation members, so you get a warning when you try to use this. You can use FixedList<> to pass lists around, so nobody modifies them. But, the lists are still normal. No conversion is necessary.

[scheglov]

It does not look to me that fixed_collections solves this issue.
If you have a function that accepts List, you can give it FixedList, and there are no type based errors.

import 'package:fixed_collections/fixed_collections.dart';

main() {
  const items = [0, 1, 2];
  f(FixedList(items));
}

void f(List<int> items) {
  items.add(42);
}

[MelbourneDeveloper]

@scheglov, you have to use the FixedList<> type. List<> allows mutation at compile time, but FixedList<> does not