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

dart:core : how is a consumer of an API that yields a List to know if they can call sort ? #13926

Closed
DartBot opened this issue Oct 9, 2013 · 12 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-not-planned Closed as we don't intend to take action on the reported issue library-core type-enhancement A request for a change that isn't a bug

Comments

@DartBot
Copy link

DartBot commented Oct 9, 2013

This issue was originally filed by ross.dart.la...@gmail.com


0.8.1_r28355

Consider the following program:

//////////////////////////////////////
List<String> provider1() => ['oranges', 'apples'];
List<String> provider2() => const ['oranges', 'apples'];
main() {
  List l1 = provider1();
  List l2 = provider2();
  print(l1..sort((String a, String b) => a.compareTo(b)));
  print(l2..sort((String a, String b) => a.compareTo(b)));
}
//////////////////////////////////////

In the dart vm this program yields:

[apples, oranges]
Unsupported operation: Cannot modify an immutable array

How, as a user of an API such as provider1 or provider2, are you supposed to deduce that it has given you a List which you cannot call sort on?

One solution would be to add a method on the List interface to check:

    bool get isFixed;

thanks,

@DartBot
Copy link
Author

DartBot commented Oct 9, 2013

This comment was originally written by ross.dart...@gmail.com


correction:
    bool get isMutable;

@DartBot
Copy link
Author

DartBot commented Oct 9, 2013

This comment was originally written by ross.dart...@gmail.com


Another solution is to provide a type in dart:core for ImmutableList or ReadOnlyList and then the consumer can perform an is type test.

@lrhn
Copy link
Member

lrhn commented Oct 9, 2013

There is currently no way to distinguish an unmodifiable list from a modifiable one.
We have decided to have only one List type, and some lists are allowed to only support some of the operations of the general interface. There could be other List types that support other subsets of operations, and the only way to know it is to document it in the function providing the list.

It does seem, though, that unmodifiable lists and fixed-length lists are the most typical restricted variants.


Removed Type-Defect label.
Added Type-Enhancement, Area-Library, Library-Core, Triaged labels.

@floitschG
Copy link
Contributor

If your method is changing an object it receives it should state so, and thus receive a mutable version.
If it can behave differently (more efficiently?) it should take an extra boolean argument, and let the caller decide if it is willing to let its list be modified.
It is usually bad style to silently modify input arguments.


Added NotPlanned label.

@DartBot
Copy link
Author

DartBot commented Oct 10, 2013

This comment was originally written by ross.dart...@gmail.com


comment #­4 I think that you misunderstood.

The API is this:

List<String> provider1() => ['oranges', 'apples'];
List<String> provider2() => const ['oranges', 'apples'];

The consumer of the API (some application code) is this:

  List l1 = provider1();
  List l2 = provider2();
  print(l1..sort((String a, String b) => a.compareTo(b)));
  print(l2..sort((String a, String b) => a.compareTo(b)));

The consumer of the API doesn't need to state anything; in fact it has noone to state anything to. It is an application, a user of an API.

As comment #­3 suggests the only way for the provider (the API) to indicate the mutability of the List is via documentation. My understanding is that one of the tenants of optional types is that the types are the documentation, so I would expect to be able to deduce from the List type whether or not it is mutable.

The use of getters such as List.isFixed or List.isMutable is a good fit for Dart in my opinion. I understand that you are trying to limit the number of types and in general I like that approach. However, I believe the types in the core libraries need to be bulletproof. This approach would fit well with dart:mirrors. In the mirror classes, many methods can fail dynamically, but there is almost always a safe way to navigate around using the getters such as ClassMirror.isTopLevel, etc...

The example I gave is very important. I am writing an application (large, complex) that uses many service interfaces that exposes Lists. The code that consumes the service does not know how it is implemented. It can be written, fully unit tested, and shipped in a working state. Then, later, it could fail when it receives a new implementation of that service interface via a provider which for one reason or another delivers it an immutable list. This is not a far-fetched or unrealistic scenario, and in fact it happened to me yesterday which is why I wrote this bug. Of course, in the current Dart there is no way to dynamically load a class so you could argue that I can see all the implementation logic, which is true but it took me about 2 hours yesterday to find the bug. In the future, if Dart gets dynamic class loading, which I hope it does, I may have no way to feel confident in shipped code if I cannot rely on the interface types.
  

@floitschG
Copy link
Contributor

True. I misread.

It's very unlikely (read: "don't get your hopes up") that we will add "isModifiable" and "isFixedLength", but I see your point and we will discuss it again. Until then I'm reopening.

Wrt the library changes: without interfaces (and we won't add interfaces) you still need to check yourself. This could be done with a "assert(list.isModifiable);", but it's easy to forget.


Added Triaged label.

@floitschG
Copy link
Contributor

Issue #8113 has been merged into this issue.

@floitschG
Copy link
Contributor

Issue #17469 has been merged into this issue.

@lrhn
Copy link
Member

lrhn commented Mar 21, 2014

We don't intend to have types that marks a list as being unmodifiable or fixed length,
nor do we have any current plan of adding boolean flags to the lists. Each List implementation can choose to implement only some parts of the interface, and fixed-length and unmodifiable lists are not the only possible restrictions.


Added NotPlanned label.

@DartBot
Copy link
Author

DartBot commented Mar 21, 2014

This comment was originally written by @zoechi


I can live with your decistion, I just want to note that other list implementations can be recognized because they differ on their type. if(new FancyList() is FancyList) ... this is not possibly with fixed-length and unmodifiable lists because the type is still List.

@lrhn
Copy link
Member

lrhn commented Jun 19, 2014

Issue #19505 has been merged into this issue.

@DartBot
Copy link
Author

DartBot commented Jun 19, 2014

This comment was originally written by ir...@google.com


Would creating a transformer that converts all const list into new UnmodifiableListView() be the thing to do then? Because other than coding optimization and maybe less words to type for the programmer, I don't see it helping the developer in any meaningful way.

It seems like I need to change all list references in the code base to

var list = new UnmodifiableListView(const ['abc', 'def']);

to achieve what I want. Which sort of defeats the need for being able to create a const List since I can just do

var list = new UnmodifiableListView(['abc', 'def']) and save myself some typing.

@DartBot DartBot added Type-Enhancement area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core closed-not-planned Closed as we don't intend to take action on the reported issue labels Jun 19, 2014
@kevmoo kevmoo added type-enhancement A request for a change that isn't a bug and removed priority-unassigned labels Mar 1, 2016
@lrhn lrhn mentioned this issue Oct 29, 2022
This issue was closed.
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-not-planned Closed as we don't intend to take action on the reported issue library-core type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

4 participants