Shopify / tapioca

The swiss army knife of RBI generation
MIT License
733 stars 122 forks source link

Module/Class aliases confuse sorbet #1086

Closed fsateler closed 2 years ago

fsateler commented 2 years ago

tapioca gem, when confronted with a constant that is a Module, but under a different name, generates an assignment in rbi:

class Bar
end
Foo = Bar

But this is problematic because now Foo is not the same as Bar according to sorbet, and some normal usages fail to typecheck: sorbet.run example

This is a known limitation of sorbet, and one that is not planned to be lifted anytime soon.

Therefore, I suggest tapioca gems should stop generating such assignments. Alternatives could be:

  1. Don't generate anything. Safest, but probably less useful
  2. Generate NewName = Class.new(OldName). This is strictly speaking a lie, but can get you a long way.
  3. Add a way to tell tapioca to omit a given constant from the generated output

This problems happened to me with the delayed_job gem, which has different backends, and each backend defines Delayed::Job = Delayed::Backend::Blabla.

rafaelfranca commented 2 years ago

The problem in the theoretical example isn't the constant aliases, but its usage in the signature. We have a very large application with close to 100% test coverage and constant aliases where never a problem, so maybe we should be discussing the concrete example.

What is the rbi generated for the delayed_job gem? And does Sorbet already fail on that rbi on only in usage of the aliased constants?

fsateler commented 2 years ago

Hi, thanks for the quick reply.

The problem in the theoretical example isn't the constant aliases, but its usage in the signature

Indeed, the rbi file on its own does not fail. However, it does creates constants that are not module or classes, and thus are not very useful.

What is the rbi generated for the delayed_job gem?

It generates (among many other things), this:

Delayed::Job = Delayed::Backend::ActiveRecord::Job

And does Sorbet already fail on that rbi on only in usage of the aliased constants?

Only in the usage. In our codebase, we have:

# service for updating some attributes in the scheduling
class Delayed::Job::Update
end

And this fails.

fsateler commented 2 years ago

Ooh, I managed to hack my way around this by adding this to require.rb:

Delayed::Job = Class.new(Delayed::Backend::ActiveRecord::Job)

This somehow prevents tapioca from seeing that Delayed::Job is an alias, and no longer generates an annotation for Delayed::Job.

rafaelfranca commented 2 years ago

Indeed, the rbi file on its own does not fail. However, it does creates constants that are not module or classes, and thus are not very useful.

They are module/classes, it is just that Sorbet namespacing and inheritance lookup don't work on them. But in your example Sorbet can totally understand Bar. That is why the include Bar line works. So they are not useless. It is fine to reference to them, and sometime people do.

Only in the usage.

Yeah, that is a problem on Sorbet, not tapioca. The generated RBI is correct. But, Sorbet doesn't allow constant aliases to be used as namespace and it thinks it is different from the source constant, when it isn't.

You can totally do Delayed::Job.some_method_that_exists in your app and you not get any error, and should not. What proves the RBI isn't incorrect, and that the aliases isn't useless.

See example

That is why I don't think there is something to be fixed here. If anything, maybe DelayedJob should not be using constant aliases to change their backend, and just be using an adapter pattern. They even know that by looking at this silence_warning call https://github.com/collectiveidea/delayed_job/blob/11e0212fb112c5e11e4555ef1e24510819a66347/lib/delayed/worker.rb#L71.

The workaround you found seems to be enough to me. I don't think we should change Tapioca to be less useful for genuine cases because some gems are using a bad design.

Thank you for the issue.