Add and vendor rubocop-sorbet and sorbet-runtime-stub.

[reitermarkus]

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes? Here's an example.
• Have you successfully run brew style with your changes locally?
• Have you successfully run brew tests with your changes locally?
• Have you successfully run brew man locally and committed any changes?

---

This adds rubocop-sorbet for checking common type-checking issues and sorbet-runtime so we can add the type-checking annotations inline in Ruby files.

Also fixed the remaining errors, except for the one fixed in sorbet/sorbet#3438.

[issyl0]

Wow, big PR. Thanks for all this effort, Markus - I didn't expect you to go quite this far yet! 😲

A few comments on the general things you've done here - hopefully I'll have time to test this out and review the results of these changes later or tomorrow!

I remember in the original GSoC PR, Mike requested that Sorbet stuff be kept out of the main files and done separately, which is why we went for "sorbet/files.yaml" with all the files and only adding RBI files, not actually adding signatures to the code using sorbet-runtime. But he didn't specify "until x time". As a result, I suggest leaving this open until tomorrow and asking him.

These days, now GSoC is over and it's a thing we're definitely doing, in my opinion we're only going to increase adoption if we make Sorbet's typing mechanisms visible.

Adding rubocop-sorbet was a thing on Vidushee's original project plan for towards the end of GSoC, but it was eclipsed by other priorities because the project was so expansive. Thanks for doing it!

Again, those were some comments on the approach. If there are any clarifications you need/want, then shout!

[reitermarkus]

Sorbet stuff be kept out of the main files and done separately

I don't think it's feasible to keep maintaining type information in separate files. It's already hard enough adding this information, but maintaining two sets of files and keeping them in sync is just way too tedious for this to gain adoption in the code base.

[issyl0]

@reitermarkus I 100% agree.

[MikeMcQuaid]

This adds rubocop-sorbet for checking common type-checking issues

I'm 👍🏻 on vendoring this

and sorbet-runtime so we can add the type-checking annotations inline in Ruby files.

Given reasoning below: I'm 👎🏻 on vendoring this for now

I remember in the original GSoC PR, Mike requested that Sorbet stuff be kept out of the main files and done separately, which is why we went for "sorbet/files.yaml" with all the files and only adding RBI files, not actually adding signatures to the code using sorbet-runtime. But he didn't specify "until x time". As a result, I suggest leaving this open until tomorrow and asking him.

These days, now GSoC is over and it's a thing we're definitely doing, in my opinion we're only going to increase adoption if we make Sorbet's typing mechanisms visible.

I still think we're not there yet and that typing information in-file makes the Ruby code significantly harder to read and follow to non-Sorbetists (which we're far from having all maintainers being, let alone contributors). The stage I'd see a rollout be:

1. add type information for more files until we hit a certain threshold
2. require brew typecheck to be run in order to pass CI rather than being a scheduled, after-the-fact job
3. consider using Sorbet at runtime
4. consider embedding Sorbet type information in files

It's already hard enough adding this information, but maintaining two sets of files and keeping them in sync is just way too tedious for this to gain adoption in the code base.

I don't think "two sets of files" is more harmful to wider adoption than "understanding Sorbet is non-trivial" and I really don't want to start making that a requirement for casual contributors (yet, at least).

[issyl0]

1. add type information for more files until we hit a certain threshold

Now Markus has done the majority of the annoying things in the base output (the current true files), I feel this is going to go faster and also inspire others (and I know Vidushee is planning to carry on with it)! Do you have an idea for what "a certain threshold" looks like? It's easier to track things now we've got false in a single list in sorbet/files.yaml, at least.

2. require brew typecheck to be run in order to pass CI rather than being a scheduled, after-the-fact job

It already is run on CI, but when we're starting from 0 errors we can remove the --quiet so it actually reveals its results and people can react to it.

[MikeMcQuaid]

Now Markus has done the majority of the annoying things in the base output (the current true files), I feel this is going to go faster and also inspire others (and I know Vidushee is planning to carry on with it)!

Agreed, great work @reitermarkus 👏🏻

Do you have an idea for what "a certain threshold" looks like?

I'm not sure yet 😭 I guess I see that being variable depending on what 👇🏻 looks like?

It already is run on CI, but when we're starting from 0 errors we can remove the --quiet so it actually reveals its results and people can react to it.

Yes, sorry, this is what I mean 🎉. Presumably this will also start us down the route of "if new contributors add new functions: they need to add type information" too? I think that's a good starting point.

[reitermarkus]

typing information in-file makes the Ruby code significantly harder to read and follow to non-Sorbetists (which we're far from having all maintainers being, let alone contributors)

And keeping this information in separate files only lengthens the time for them to become familiar with it because no one will just casually look at the .rbi files.

[MikeMcQuaid]

And keeping this information in separate files only lengthens the time for them to become familiar with it because no one will just casually look at the .rbi files.

This begs the quest: does a casual Homebrew contributor or all Homebrew/brew maintainers need to become familiar with it now? I would strongly argue: "no" or at least "not yet".

[reitermarkus]

I would strongly argue: "no" or at least "not yet".

Why not and when would “not yet” ever become “now“ with that mindset?

[MikeMcQuaid]

I would strongly argue: "no" or at least "not yet".

Why not and when would “not yet” ever become “now“ with that mindset?

It adds friction to contributing to Homebrew with essentially no end-user benefit. I'm yet to see a single user-reported issue that has or would have been fixed by Sorbet and enabling it at runtime will further slow down boot of all brew commands.

I think it's worth having as an investment in the future and moving in that direction gradually but: we're not there yet.

Here's what I'd see as being the steps along the way:

1. we get brew typecheck running without --quiet
2. we get 100% code coverage for brew typecheck
3. we start using sorbet-runtime optionally at runtime for CI/testing and maybe HOMEBREW_DEVELOPERs (perhaps only with a specific opt-in environment variable)
4. if we can get the boot time overhead down to a minimal level (or almost zero): we vendor sorbet-runtime and enable it for everyone

[reitermarkus]

It adds friction to contributing to Homebrew

How so? It is optional type information. If you are editing an existing file there's no requirement to do anything unless everything in it is already typed, in which case you can just follow the existing pattern.

I'm yet to see a single user-reported issue that has or would have been fixed by Sorbet

Take any issue ever which was caused by a missing method. In any case, type-checking is not for fixing errors but to prevent them from being introduced in the first place.

I think it's worth having as an investment in the future and moving in that direction gradually but: we're not there yet.

And we won't be there for a lot longer if we make it more difficult than it needs to be.

What do other @Homebrew/maintainers think here?

[SMillerDev]

I don't think it would be too much of a hassle for new contributors since I would assume the majority of them isn't fluent in ruby yet and sorbet won't be that much of a hurdle to copy-paste.

What I am really curious about is the change in boot time for brew, since that's something we get complaints about already.

[MikeMcQuaid]

And we won't be there for a lot longer if we make it more difficult than it needs to be.

You're arguing against an incremental rollout of new technology because it'll be slower. Yes, that's the point: it's also more cautious and allows us to see and deal with issues as we move forwards iteratively.

I don't think it would be too much of a hassle for new contributors since I would assume the majority of them isn't fluent in ruby yet and sorbet won't be that much of a hurdle to copy-paste.

I disagree. It's one thing being unfamiliar with Ruby, it's another being unfamiliar with manual type declarations in language/frameworks which is the case for many developers now.

[reitermarkus]

You're arguing against an incremental rollout of new technology because it'll be slower.

I'm not arguing against an incremental rollout. Moving from .rbi files to in-line annotations is an increment if you ask me.

[vitorgalvao]

I don’t mean to derail the conversation (so feel free to not reply to this, but it felt worth mentioning), but seeing as Ruby 3 will support type annotations and it may be released not too far into the future (this year?) is Sorbet something we’ll want to stick with or eventually replace with Ruby’s implementation?

If the latter and there’s an implementation difference (which seems to be the case) between Ruby 3 and Sorbet, the slower we go now the fewer we’ll need to change later.

[reitermarkus]

From https://sorbet.org/blog/2020/07/30/ruby-3-rbs-sorbet:

Sorbet will happily incorporate RBS as a way to specify type annotations

[MikeMcQuaid]

I'm not arguing against an incremental rollout. Moving from .rbi files to in-line annotations is an increment if you ask me.

With what I mentioned earlier:

1. we get brew typecheck running without --quiet
2. we get 100% code coverage for brew typecheck
3. we start using sorbet-runtime optionally at runtime for CI/testing and maybe HOMEBREW_DEVELOPERs (perhaps only with a specific opt-in environment variable)
4. if we can get the boot time overhead down to a minimal level (or almost zero): we vendor sorbet-runtime and enable it for everyone

You're jumping from 1. to 4. here and putting 2. at the end.

I would like to see each of these as separate PRs, please (and 2. be split into many different PRs along the way). 4. will also require profiling/benchmarking to demonstrate it's not regressing the boot time.

[reitermarkus]

putting 2. at the end.

I'd argue it should be at the end. Would you also argue that we should have 100% test coverage and only then actually run the tests?

Anyways, I have replace the vendored sorbet-runtime with stubs, so it doesn't impact boot time while still allowing us to add the annotations directly in Ruby files, so the order can be 1, 3, 2, 4 now.

[MikeMcQuaid]

Would you also argue that we should have 100% test coverage and only then actually run the tests?

No.

Anyways, I have replace the vendored sorbet-runtime with stubs, so it doesn't impact boot time while still allowing us to add the annotations directly in Ruby files, so the order can be 1, 3, 2, 4 now.

Nice work! A few final requests before this is merged:

• can you point to where/how the delegator is a no-oped unless e.g. HOMEBREW_DEVELOPER or whatever is being set/setup? I was trying to read through here but it's hard to view just that part.
• I think it would be good to avoid vendoring until it's used for everyone. Can have it loaded using Bundler (like we used to do with e.g. patchelf) when the necessary variables are set.
• could the stub code get some tests?

[reitermarkus]

can you point to where/how

https://github.com/Homebrew/brew/pull/8781/files#diff-07a58dc567223bb118b941b96f9dba79R4-R7

could the stub code get some tests?

Running brew tests or brew typecheck will use them, no need to test them separately.

[MikeMcQuaid]

1. we get 100% code coverage for brew typecheck

I am kind of curious what is meant by this statement, as there are multiple ways to measure type coverage in sorbet. For example, "no files are ignored by sorbet" vs "all files are typed: true" vs "all methods have signatures" vs "every call site is guaranteed to call a method that exists". Usually we recommend turning Sorbet on after the first (no files are ignored). But of course, you make some good points in favor of being cautious, so feel free to adjust this recommendation.

Yeh, turning on after the first makes sense to me (combined with turning off --quiet) 👍🏻

[MikeMcQuaid]

Looking good! One final question then think we're good to go.

[MikeMcQuaid]

Great work and thanks for persevering on this @reitermarkus!

[issyl0]

Yay, it's great to see this shipped!