Add Sorbet/FetchWhenMust cop
#141
Conversation
sambostock
force-pushed
the
fetch-when-must
branch
2 times, most recently
from
659b1a3
to
98ba1b4
Compare
|
I'd love to have this, but I worry there will be some pushback since |
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
sambostock
force-pushed
the
fetch-when-must
branch
from
98ba1b4
to
01d4e6b
Compare
This allows examples to include `# rubocop:disable`, while still catching rogue comments.
sambostock
force-pushed
the
fetch-when-must
branch
from
01d4e6b
to
7066e9d
Compare
Checks for `T.must(object[key])` and recommends `object.fetch(key)` instead. While usually safe, false positives are possible if `object` does not respond to `fetch`, or if `Hash#default_proc` (or similar) is being used. To avoid correction clobbering, we only correct nodes that are not nested in another offending node. Since investigation is repeated until no corrections are made, this eventually corrects all offenses.
sambostock
force-pushed
the
fetch-when-must
branch
from
0ecdd8c
to
ac1ecbd
Compare
|
I discovered an issue with nested offense correction where correcting the inner node would clobber the outer node correction. After investigating, I found rubocop/rubocop#10511 & rubocop/rubocop#10461 which demonstrate how to address this skipping correction for nodes within an already corrected node. Because autocorrection repeats investigation until the file no longer requires correction, all nodes are eventually corrected. I have taken this approach here. |
|
cc @vinistock for your opinion on this, since you commented about it recently. |
| # | ||
| # # good | ||
| # # If `object` does not `respond_to? :fetch`, or if using `Hash` `default_proc` | ||
| # T.must(object[key]) # rubocop:disable Sorbet/FetchWhenMust |
There was a problem hiding this comment.
I'm not completely convinced by this cop exactly because of this case.
From a user experience point of view:
- I write some code that is seemingly correct
- The cop complains that I should use
fetch - I try to use fetch but now the type checker complains
- What should I do? Adding a
rubocop:disablealways feels wrong... - Confusion state
I agree that is this something we generally recommend in code reviews but the inability to only target hashes / arrays from the cop is a huge limitation in this case.
Do we have an idea of how many rubocop:disable we would need to add in core for example?
I'm also worried about the massive change needed to enable this cop. What if the Sorbet nor tests do not catch that one of the fetch call is actually invalid?
There was a problem hiding this comment.
Is there a way to leverage Sorbet's type information to narrow this cop to only Arrays/Hashes and other similar container types?
|
Minor data point – a Stripe engineer suggests
|
|
I think I'm going to close this PR due to the experience @Morriar brought up. However, I must say I like this feature and think we can get close to it inside Sorbet autocorrect. @sambostock what do you think of working on an implementation inside Sorbet that checks to see if the type's methods contain |
Checks for
T.must(object[key])and recommendsobject.fetch(key)instead.While usually safe, false positives are possible if
objectdoes not respond tofetch, or ifHash#default_proc(or similar) is being used.