Rails/TransactionExitStatement - not applicable anymore

[FunnyHector]

As @javierjulio has noticed in this PR, rails/rails/pull/39453 was committed which seems to make this cop condition not applicable. Rails won't emit such deprecation if there is no write to database, whereas Rails/TransactionExitStatement doesn't consider that.

I'm not sure if there is a way for Rubocop to detect write to database. I don't think so given that Rubocop is just a static analysis tool.

Maybe we should decommission this cop now?

cc: @javierjulio

---

Expected behavior

User.transaction do
  return if some_crtieria_met?

  # do work that write to database
end

This code won't emit deprecation, because there hasn't been any write into database. This is a new behaviour since rails/rails/pull/39453

Actual behavior

Rails/TransactionExitStatement will flag it.

Steps to reproduce the problem

Just run rubocop with this cop on this code.

RuboCop version

Since the version where Rails/TransactionExitStatement is introduced.

My local version:

➜  bundle exec  rubocop -V
1.25.1 (using Parser 3.1.2.0, rubocop-ast 1.17.0, running on ruby 3.0.4 arm64-darwin21)
  - rubocop-performance 1.13.2
  - rubocop-rails 2.14.2
  - rubocop-rspec 2.10.0

[FunnyHector]

Any updates on this one? I propose to remove this cop, if no solution can be found.

[aprescott]

It's worth mentioning that, with Rails 7.1, the behavior of return inside a transaction depends on config.active_record.commit_transaction_on_non_local_return. It seems that the current plan is for non-local returns to commit transactions on 7.2.

See rails/rails#48600 and related info linked from that PR.

[aprescott]

To keep the context flowing: 7.2 deprecates config.active_record.commit_transaction_on_non_local_return.

[Earlopain]

Ugh, this one. I always found that behaviour confusing and not something I ever really wanted to think about. I'm glad they did the work to make this more intuitive.

Thanks for the reminder, I'll open a PR tomorrow to disable this cop on >= 7.2. You could argue about 7.1 but it depends on the config value 🤷. 7.2 at least is pretty clear

[aprescott]

7.2 still uses the config value, I think, but I'd argue the cop is already "broken" pre-7.2 anyway if it doesn't account for the setting or the change in behavior across versions. You could maybe switch the cop off on > 7.2 rather than >= 7.2? (I likewise avoid return inside a transaction block completely and restructure the flow of the block, though. Kind of a mess all around here! 😅)

[Earlopain]

7.2 still uses the config value, I think

It doesn't: rails/rails@eccc606

I'd argue the cop is already "broken" pre-7.2 anyway if it doesn't account for the setting or the change in behavior across versions

That's a bit more arguable, there's nothing wrong with not using a feature. This is important for engines where the version of rails is not necessarily known upfront. I know that for most users that is not applicable though, most will already have opted in to the new behaviour since the old one shows a deprecation warning. I guess in most cases it is more like a style cop then.

PR: #1366

[aprescott]

It doesn't: rails/rails@eccc606

Ah. Surprising! I only saw the description of the commit_transaction_on_non_local_return config setting being removed deprecated. Little confused how commit_transaction_on_non_local_return is only deprecated, when the behavior was completely cut over, but I guess that's a different matter.

[Earlopain]

Little confused how commit_transaction_on_non_local_return is only deprecated

Rails (almost) never removes things without showing a deprecation to potential users. This usually leads to things like this remaining for one extra release cycle, even if their only purpose was to help with deprecated behavior in the first place.

[aprescott]

Of course :) it just seems more confusing to me to have a config called commit_transaction_on_non_local_return that is deprecated yet does nothing. It misleads. I would expected the deprecated config to still have an effect, or to be removed at the same time the conditional behavior is. But I understand.