Tapioca::Dsl::Compilers::ActiveJob missing Exception as a possible return type in perform_now type compiled signature

[JiaboHou]

Description

Tapioca's compiled type signature for ActiveJob::Base.perform_now essentially uses the same as ActiveJob::Base#perform. However, this is not fully representative of the behaviour because .perform_now will return the exception that got raised inside #perform when that exception has been rescued with rescue_from or retried with retry_on. Other scenarios are possible, these are just the 2 scenarios I tested.

To illustrate this point, consider the following job definition:

# failing_job_with_rescue_from_noop.rb

# typed: strict
# frozen_string_literal: true

class FailingJobWithRescueFromNoop < ActiveJob::Base CustomCops/EssentialsApplicationJob,Rails/ApplicationJob
  rescue_from(StandardError) { p "error!" }

  sig { void }
  def perform
    raise "FailingJobWithRescueFromNoop"
  end
end

Actual behaviour

Running bin/tapioca dsl FailingJobWithRescueFromNoop produces the following RBI. Note the type signature for .perform_now:

# failing_job_with_rescue_from_noop.rbi

# typed: true

# DO NOT EDIT MANUALLY
# This is an autogenerated file for dynamic methods in `FailingJobWithRescueFromNoop`.
# Please instead update this file by running `bin/tapioca dsl FailingJobWithRescueFromNoop`.


class FailingJobWithRescueFromNoop
  class << self
    sig { params(block: T.nilable(T.proc.params(job: FailingJobWithRescueFromNoop).void)).returns(T.any(FailingJobWithRescueFromNoop, FalseClass)) }
    def perform_later(&block); end

    sig { void }
    def perform_now; end
  end
end

However, when one runs the following in a rails console:

> result = FailingJobWithRescueFromNoop.perform_now rescue "error raised"
> result
=> #<RuntimeError: FailingJob>

As illustrated above, .perform_now actually returns the error, it does not raise it. This appears to be expected behaviour as confirmed in a comment in rails/rails#48281. Shopify/job-iteration also had to update their custom Tapioca compiler with Shopify/job-iteration#537.

Expected behaviour

Assuming we want the RBI to reflect the possibility of returning exceptions, running bin/tapioca dsl FailingJobWithRescueFromNoop should instead produce something like the following for perform_now:

# failing_job_with_rescue_from_noop.rbi

# ...

class FailingJobWithRescueFromNoop
  class << self
    # ...

    sig { returns(T.any(NilClass, Exception)) }
    def perform_now; end
  end
end

This is what was done in Shopify/job-iteration#537, but this is only possible because that gem provides a #perform method that always returns nil. If used in the general case, any type sigs for #perform that currently have "void" as the return type will fail typechecks if the method returns a non-nil value. Perhaps using T.untyped is the next best thing here?

[KaanOzkan]

I think there are 2 more solutions than T.untyped:

1. Instead of wrapping every generated signature in a T.any we can check Foo.rescue_handlers which should be populated when using rescue_from. Modify the compiler to return T.any(Foo, Exception) if there's a rescue handler.
2. Right now the signature is imported from the perform method, so we could leave it to the job authors to specify that perform can raise in a signature and not update Tapioca.

[JiaboHou]

1. Instead of wrapping every generated signature in a T.any we can check Foo.rescue_handlers which should be populated when using rescue_from. Modify the compiler to return T.any(Foo, Exception) if there's a rescue handler.

I think this still has the issue of void types not being fully compatible. For example if I had the following job:

class JobWithVoidReturn < ActiveJob::Base
  rescue_from(StandardError) { p "Error!" }

  sig { void }
  def perform
    1 + 2 # Technically returns 3, but we've signalled to Sorbet that this should be ignored.
  end
end

The proposed generated signature for .perform_now would be:

sig { returns(T.any(NilClass, Exception)) }

However, #perform's implementation breaks this signature since it would actually return 3.

[KaanOzkan]

Good point, I think to mitigate that we can special case void. For other return types T.any(Type, Exception) will be more desirable than T.untyped.