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

[linter] noop_primitive_operations #59684

Open
stephane-archer opened this issue Dec 7, 2024 · 6 comments
Open

[linter] noop_primitive_operations #59684

stephane-archer opened this issue Dec 7, 2024 · 6 comments
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. linter-lint-request

Comments

@stephane-archer
Copy link

var a = [1,2,3].toList(); // no lint warning!

toList on a list should be considered a noop_primitive_operations and be added to the linter

@stephane-archer stephane-archer added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Dec 7, 2024
@bwilkerson
Copy link
Member

While it might make sense to lint the use of toList on a literal list, the method toList returns a copy of the list on which it invoked. If you run

void main() {
  var l = [1, 2, 3];
  print(identical(l, l.toList()));
}

it will print false.

So, invoking toList on a list isn't a no-op. Given the limited scope of the lint I suspect that we don't want to add it.

If we were to decide to extend the lint (or create a new one) to catch this case, it should also catch the use of toSet on a set literal.

@bwilkerson bwilkerson added analyzer-linter Issues with the analyzer's support for the linter package linter-lint-request labels Dec 7, 2024
@stephane-archer
Copy link
Author

Is there any reason why you decided to make toList on a list, not a no opt?
It's a way to make a copy that is not as explicit as I would like.
Suggesting to use List.from(l); in that case could make sense.

I suspect the use of toList on a list has been more often a mistake than something intentional

@FMorschel
Copy link
Contributor

I've just recently discovered that behaviour. I mostly use the spread operator or List.of or something. I do agree that it doesn't show fully the behaviour you'd expect by the name.

I noticed that all the other noop operations are on primitive types and that means that they basically can't have two instances with the same values if they are not identical like the number 1 or a specific set of characters that create a string.

So I understand the gist now, but I do agree this is not as intuitive for someone new to the language.

@FMorschel
Copy link
Contributor

It would also be a good addition for cases where you had a method that returned an iterable and you were using .toList and some api change made that original method return a List now so that would not be needed anymore and probably is only making more work than needed.

@bwilkerson
Copy link
Member

@lrhn For the design of toList.

@lrhn
Copy link
Member

lrhn commented Dec 8, 2024

The toList method is defined on Iterable, and it's the canonical way to convert a lazy iterable eagerly to its values.
The resulting list is independent of the original Iterable, so you can use it without risking that its values suddenly change. That's the primary reason to convert an iterable to a list, that or wanting to operate operate now than once.

Since a List is-an Iterable, it has a toList too, which also creates a list of the values that won't change if the original list does.

If you want "a new list, but only if this isn't already a list", you need to check whether you already have a list. There is no built-in feature which does that, because the usual reasons one may have for wanting a list to begin with are either to have your own copy that you can trust not to change (and a list is just the simplest data structure for that), or you want to do random access (like sorting or similar), and then you usually still don't want the length to change while you're random-accessing. Or you want to iterate more than once, but then you also want/expect the two iterations to have the same elements.

And if you really, really just want a list, even if someone might change it (you'll just tell them not to), maybe the parameter should be typed as List instead of Iterable to begin with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. linter-lint-request
Projects
None yet
Development

No branches or pull requests

4 participants