Introduce `ForbidNestedTMust` cop

[jacobobq]

This cop enforces replacing nested calls to T.must with a single call and safe-navigation operators.

[jacobobq]

Hello! 👋🏼 If you are interested in this PR let me know any necessary steps to merge it.

I've written this to help improve readability after using sorbet's automatic fixes and to enforce a consistent style.

[jacobobq]

I have signed the CLA!

[amomchilov]

Here's an exact, unedited case you can test against:

      assert_equal(
        I18n.t("sales.order_payments.errors.order_not_found"),
        T.must(T.must(result.error_value).errors.first).type
      )

It auto-corrected to:

      assert_equal(
        I18n.t("sales.order_payments.errors.order_not_found"),
        T.must(result.error_value&.errors.first).type
      )

Which has this type-checking error:

file.rb:74: Used &. operator on ActiveModel::Errors, which can never be nil https://vault.shopify.io/page/Sorbet-error-reference~dhbd3ef.md#7034
    74 |      assert_equal(I18n.t("sales.flash.orders.saving_failed"), T.must(result.error_value&.errors.first).type)

[jacobobq]

Here's an exact, unedited case you can test against:

      assert_equal(
        I18n.t("sales.order_payments.errors.order_not_found"),
        T.must(T.must(result.error_value).errors.first).type
      )

It auto-corrected to:

      assert_equal(
        I18n.t("sales.order_payments.errors.order_not_found"),
        T.must(result.error_value&.errors.first).type
      )

Which has this type-checking error:

file.rb:74: Used &. operator on ActiveModel::Errors, which can never be nil https://vault.shopify.io/page/Sorbet-error-reference~dhbd3ef.md#7034
    74 |      assert_equal(I18n.t("sales.flash.orders.saving_failed"), T.must(result.error_value&.errors.first).type)

Thanks a lot for the feedback @amomchilov! This one is a tricky one and I found the same issue while running it on our codebase. In the last two hours I've found and fixed a couple more edge cases.

For this specific case I'm unsure about how to proceed, to be able to determine if the conditional-send is actually necessary or not, I'd need to query sorbet to check the type of the expression contained inside the inner T.must before I unwrap it.

It's a weird case because the T.must was wrapping something that wasn't nilable. In my case, when I found these in our code after running the cop, I run srb tc --did-you-mean=false --isolate-error-code=7034 -a and made sorbet autofix all of them at once. A user running into this case when manually autocorrecting from their IDE would instantly run into a type error but luckily, one with an autofix.

Even if this case cannot be addressed with rubocop alone, I still think the cop is pretty useful but if you have any recommendations or ideas to try out to be able to implement it flawlessly, I'm all for it!

Edit: maybe it would make sense to mark the cop as unsafe?

[amomchilov]

Hey Jacobo,

We discussed this internally, and found several reasons why we won’t be able to adopt this cop:

1. It can’t auto-correct on its own, and would lead to a poor experience for anyone using Rubocop auto-formatting, but not Sorbet auto-formatting.
2. There are ongoing efforts to improve the T.must error message (to tell you not just the line that raised, but which exact part of the line, so you can tell two T.must calls apart), which the &.-ified form wouldn’t benefit form.
3. While the nested T.musts are unsightly, they make a stronger guarantee about the expected values in the expression (that not just the final result can’t be nil, but the intermediate values in the chain can’t be nil, either). Switching to &. removes the guarantees about the intermediate values.
   1. Also, the auto-corrected form can’t be undone, because it’s impossible to automatically if a &. was hand-written, or a replacement for what used to be a nested T.must, so it’s a permanent loss of this information.

I’d encourage you to publish it under your own gem, perhaps the broader community might be interested!