Skip to content
This repository has been archived by the owner on Nov 20, 2024. It is now read-only.

skip nullable iterables in avoid_function_literals_in_foreach_calls #2752

Merged
merged 2 commits into from
Jul 7, 2021

Conversation

pq
Copy link
Member

@pq pq commented Jul 6, 2021

@google-cla google-cla bot added the cla: yes label Jul 6, 2021
@pq pq changed the title skip nullable iterables skip nullable iterables in avoid_function_literals_in_foreach_calls Jul 6, 2021
@coveralls
Copy link

coveralls commented Jul 6, 2021

Coverage Status

Coverage increased (+0.003%) to 94.005% when pulling 9e33504 on skip_nullable_iterables into 501f0a3 on master.

Copy link
Member

@bwilkerson bwilkerson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming it works with dynamic.


bool _isNonNullableIterable(DartType? type) =>
type != null &&
type.nullabilitySuffix != NullabilitySuffix.question &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how this works for dynamic. Consider

void f(dynamic iter) => iter?.forEach(...);

Is the suffix always a question even when it isn't explicitly given?

I'll just note that an equivalent test might be to look for the ?. operator on the method invocation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the suffix always a question even when it isn't explicitly given?

I believe so? (@scheglov may knot for sure.)

In any event, I added a dynamic test case and it seems to work as written.

Thanks for the catch!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type dynamic is always nullable, and it does not have any nullabilitySuffix, it is always NullabilitySuffix.none. Note that the suffix is not the same as nullability, e.g. FutureOr<int?> is nullable.

@pq pq merged commit 5f8e992 into master Jul 7, 2021
@pq pq deleted the skip_nullable_iterables branch July 7, 2021 18:05
oprypin added a commit that referenced this pull request May 6, 2023
Fixes #4328. Reverts #4039 that caused false negatives in non-null-safe mode. And reverts #2752 along with it because it was not the exact right fix in the first place. Null-aware calls are since handled by #4305 and all the existing tests still pass.
oprypin added a commit that referenced this pull request May 6, 2023
Fixes #4328. Reverts #4039 that caused false negatives in non-null-safe mode. And reverts #2752 along with it because it was not the exact right fix in the first place. Null-aware calls are since handled by #4305 and all the existing tests still pass.
oprypin added a commit that referenced this pull request May 6, 2023
Fixes #4328. Reverts #4039 that caused false negatives in non-null-safe mode. And reverts #2752 along with it because it was not the exact right fix in the first place. Null-aware calls are since handled by #4305 and all the existing tests still pass.
pq pushed a commit that referenced this pull request May 8, 2023
…#4330)

Fixes #4328. Reverts #4039 that caused false negatives in non-null-safe mode. And reverts #2752 along with it because it was not the exact right fix in the first place. Null-aware calls are since handled by #4305 and all the existing tests still pass.
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Aug 23, 2023
…dart-lang/linter#4330)

Fixes dart-lang/linter#4328. Reverts dart-lang/linter#4039 that caused false negatives in non-null-safe mode. And reverts dart-lang/linter#2752 along with it because it was not the exact right fix in the first place. Null-aware calls are since handled by dart-lang/linter#4305 and all the existing tests still pass.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants