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

make prefer_collection_literals less suggestive for sets #57905

Closed
pq opened this issue Feb 10, 2019 · 5 comments
Closed

make prefer_collection_literals less suggestive for sets #57905

pq opened this issue Feb 10, 2019 · 5 comments
Assignees
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-false-positive linter-new-language-feature

Comments

@pq
Copy link
Member

pq commented Feb 10, 2019

The initial implementation flags lots of places in the flutter repo that would be tricky to migrate. I'd like to review and relax before trying to push the updated lint.

(Note that this gates integration into the SDK.)

@pq pq self-assigned this Feb 10, 2019
@pq
Copy link
Member Author

pq commented Feb 10, 2019

Mulling over a few concrete examples

final Set<RenderObject> hits = LinkedHashSet<RenderObject>();
final List<String> where = Set<String>.from(skipped).toList()..sort();

(and a few related), I'd like to drop the flagging of:

  1. LinkedHashSet constructors altogether, and
  2. of and from named constructors as well

@bwilkerson: I think these were the kinds of cases you maybe anticipated in your earlier review? Any other thoughts?

@pq
Copy link
Member Author

pq commented Feb 11, 2019

If you want to peruse any more of the failures yourself, here's the failing run:

https://ci.chromium.org/p/dart/builders/luci.dart.try/flutter-analyze-try/26

@bwilkerson
Copy link
Member

I'd like to drop the flagging of:

  1. LinkedHashSet constructors altogether, and

I'm not sure why. As I understand it, a set literal creates an instance of LinkedHashSet, so the two should be consistent.

of and from named constructors as well

I think the assist only works for of and from when the argument is a literal list. We can convert new Set.from([1, 2, 3]) into {1, 2, 3}, but without the spread operator we can't do anything when the argument is not a list literal.

@pq
Copy link
Member Author

pq commented Feb 11, 2019

I'm not sure why. As I understand it, a set literal creates an instance of LinkedHashSet, so the two should be consistent.

Sorry, I should have shared more of my thinking. It's because of cases like this:

image

that provoke an error like:

error: The set literal type 'Set<String>' isn't of expected type 'HashSet<String>'. The set's type can be changed with an explicit generic type argument or by changing the element types. (strong_mode_invalid_cast_literal_set at ...)

For a first cut I think missing cases like this:

Set<String> s = new HashSet<String>();

which I do think should be potentially flagged is OK.

But then maybe it's alright that folks need to address the type error above after applying an assist?

I think the assist only works for of and from when the argument is a literal list.

Aha. Right. I had this logic as I recall and then talked myself out of needing it. Can't recall now why though... 🤔

@bwilkerson
Copy link
Member

But then maybe it's alright that folks need to address the type error above after applying an assist?

I would say no. Our policy on assists has always been that if the code was valid before applying an assist it should be valid after. If the assist can produce invalid code then we should capture the reproduction case (in an issue, or better as a failing test) and fix the bug.

@devoncarew devoncarew added 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. labels Nov 18, 2024
@devoncarew devoncarew transferred this issue from dart-lang/linter Nov 18, 2024
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-false-positive linter-new-language-feature
Projects
None yet
Development

No branches or pull requests

3 participants