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

Add non_nullable_equals_parameter rule #3923

Closed
wants to merge 4 commits into from
Closed

Conversation

srawlins
Copy link
Member

Description

This rule checks that a parameter to an operator == implementation has a non-nullable type.

I intentionally did not enforce, in this rule, that the parameter is exactly Object. It is legal to narrow the parameter type to a different non-nullable type, like int. I can't imagine doing it, but it seems to be unrelated to whether the type should be nullable or not.

Fixes #3441

@coveralls
Copy link

coveralls commented Dec 17, 2022

Coverage Status

Coverage increased (+0.009%) to 95.646% when pulling 97ac5a6 on equals-non-nullable into 79b8433 on main.

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.

I'm not sure that this shouldn't be a warning rather than a lint, but other than that it looks good.

return;
}

if (parameters.parameters.isEmpty) {
Copy link
Member

Choose a reason for hiding this comment

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

Alternately, we could test for length != 1, because if there's more than one there will also be a different diagnostic.

Copy link
Member

Choose a reason for hiding this comment

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

Nice idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

Love it! Done.

@srawlins
Copy link
Member Author

srawlins commented Dec 19, 2022

I'm not sure that this shouldn't be a warning rather than a lint, but other than that it looks good.

Yeah I think it's pretty grey. I would immediately recommend this go to the core lints set, so shouldn't I just make it a Warning? Or should it take a natural path from Lint to Warning, as that does get more people involved in the decision / more vetting? A puzzle...

Do you have a preference, @pq ?

@bwilkerson
Copy link
Member

I don't think that all lints in the core set should be warnings. I think a check should only be a warning if it finds a real bug, or code that's never what the user intended, with no false positives. This seems to meet that criteria, but I might be missing something.

@srawlins
Copy link
Member Author

I don't think that all lints in the core set should be warnings. I think a check should only be a warning if it finds a real bug, or code that's never what the user intended, with no false positives. This seems to meet that criteria, but I might be missing something.

Agreed. I'll wait for @pq's response.

@pq
Copy link
Member

pq commented Dec 22, 2022

Sorry for the slow response!

This one seems like a great candidate for an analyzer warning proper for all the reasons Brian enumerated. I can't think of a downside.

@srawlins
Copy link
Member Author

Replaced by a new warning: https://dart-review.googlesource.com/c/sdk/+/277700

@srawlins srawlins closed this Dec 29, 2022
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jan 13, 2023
This rule checks that a parameter to an `operator ==` implementation has
a non-nullable type.

I intentionally did not enforce, in this rule, that the parameter is
exactly `Object`. It is legal to narrow the parameter type to a
different non-nullable type, like `int`. I can't imagine doing it, but
it seems to be unrelated to whether the type should be nullable or not.

Fixes dart-lang/linter#3441

Replaces dart-lang/linter#3923

Change-Id: I61d4a7b1ab8318dc9403da1633c352de95bfac61
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/277700
Reviewed-by: Mark Zhou <markzipan@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jan 17, 2023
This reverts commit 48ee1f2.

Reason for revert: flutter/flutter#118632

Original change's description:
> [analyzer] new warning for nullable '==' parameter type
>
> This rule checks that a parameter to an `operator ==` implementation has
> a non-nullable type.
>
> I intentionally did not enforce, in this rule, that the parameter is
> exactly `Object`. It is legal to narrow the parameter type to a
> different non-nullable type, like `int`. I can't imagine doing it, but
> it seems to be unrelated to whether the type should be nullable or not.
>
> Fixes dart-lang/linter#3441
>
> Replaces dart-lang/linter#3923
>
> Change-Id: I61d4a7b1ab8318dc9403da1633c352de95bfac61
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/277700
> Reviewed-by: Mark Zhou <markzipan@google.com>
> Commit-Queue: Samuel Rawlins <srawlins@google.com>
> Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>

# Not skipping CQ checks because original CL landed > 1 day ago.

Change-Id: I6f96936b8d4e785b5f8e9719751e4b61c2a6ca2a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/279141
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Siva Annamalai <asiva@google.com>
Reviewed-by: Mark Zhou <markzipan@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Dec 14, 2023
…pe""

This reverts commit 885457e.

[analyzer] new warning for nullable '==' parameter type

This rule checks that a parameter to an `operator ==` implementation has
a non-nullable type.

I intentionally did not enforce, in this rule, that the parameter is
exactly `Object`. It is legal to narrow the parameter type to a
different non-nullable type, like `int`. I can't imagine doing it, but
it seems to be unrelated to whether the type should be nullable or not.

Fixes dart-lang/linter#3441

Replaces dart-lang/linter#3923

Change-Id: Ic0be2bfebaf59b0336e9a3a58e5b7f5359eb8646
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/291042
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Stephen Adams <sra@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
@kevmoo kevmoo deleted the equals-non-nullable branch June 25, 2024 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

proposal: avoid_nullable_equals_parameter
4 participants