Add cop against .void.checked(:tests)
#196
Conversation
|
I have signed the CLA! |
|
This interacts poorly with Sorbet's check on going to think about it for a while |
|
(for Shopifolk: I see only 3 instances of this across all our code). |
|
There are a few lint errors: https://github.com/Shopify/rubocop-sorbet/actions/runs/7575336207/job/20639488396?pr=196 |
|
Lint errors should be fixed, thanks for the heads up |
|
|
||
| if (parent = node.parent) && (sibling_index = node.sibling_index) | ||
| later_siblings = parent.children[(sibling_index + 1)..] | ||
| if (def_node = later_siblings.find { |sibling| sibling.is_a?(RuboCop::AST::DefNode) }) |
There was a problem hiding this comment.
Could this lead to miss cases like this one?
sig { params(foo: Integer).void.checked(:tests)
attr_writer :foo
def initialize; endThere was a problem hiding this comment.
Yes, good catch. In this case, it would default to no error (false negative) rather than a spurious, false positive error.
The specific reason why I implemented this was because it appears that Sorbet is fine with
sig { void }
X = 1
def foo; endwhich is weird, and happened once in Stripe's codebase.
An alternative would be to put it back to what I was originally doing, and only report the error if the next sibling node is a def node. That would lead to even more false negatives, I think, but maybe it would be easier to understand how to cop works.
There was a problem hiding this comment.
The attr case should be relatively unfrequent, I think we can go with the current implementation 👍
|
|
||
| if (parent = node.parent) && (sibling_index = node.sibling_index) | ||
| later_siblings = parent.children[(sibling_index + 1)..] | ||
| if (def_node = later_siblings.find { |sibling| sibling.is_a?(RuboCop::AST::DefNode) }) |
There was a problem hiding this comment.
The attr case should be relatively unfrequent, I think we can go with the current implementation 👍
I think I regenerated all the files that need to be regenerated but let me know if anything is missing.