-
Notifications
You must be signed in to change notification settings - Fork 166
Add non_nullable_equals_parameter rule #3923
Conversation
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it! Done.
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 ? |
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. |
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. |
Replaced by a new warning: https://dart-review.googlesource.com/c/sdk/+/277700 |
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 https://github.com/dart-lang/linter/issues/3441 Replaces dart-archive/linter#3923 Change-Id: I61d4a7b1ab8318dc9403da1633c352de95bfac61 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/277700 Reviewed-by: Mark Zhou <[email protected]> Commit-Queue: Samuel Rawlins <[email protected]> Reviewed-by: Brian Wilkerson <[email protected]>
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 https://github.com/dart-lang/linter/issues/3441 > > Replaces dart-archive/linter#3923 > > Change-Id: I61d4a7b1ab8318dc9403da1633c352de95bfac61 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/277700 > Reviewed-by: Mark Zhou <[email protected]> > Commit-Queue: Samuel Rawlins <[email protected]> > Reviewed-by: Brian Wilkerson <[email protected]> # 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 <[email protected]> Reviewed-by: Brian Wilkerson <[email protected]> Commit-Queue: Siva Annamalai <[email protected]> Reviewed-by: Mark Zhou <[email protected]> Reviewed-by: Phil Quitslund <[email protected]>
…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 https://github.com/dart-lang/linter/issues/3441 Replaces dart-archive/linter#3923 Change-Id: Ic0be2bfebaf59b0336e9a3a58e5b7f5359eb8646 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/291042 Reviewed-by: Brian Wilkerson <[email protected]> Reviewed-by: Stephen Adams <[email protected]> Reviewed-by: Konstantin Shcheglov <[email protected]>
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, likeint
. I can't imagine doing it, but it seems to be unrelated to whether the type should be nullable or not.Fixes dart-lang/sdk#58753