Support more recent Sorbet versions #26
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Get more recent versions that will install on the latest Apple Silicon machines with the latest OS.
bundle update standard bundle exec rake standard:fix
Sorbet used to do some of this, now Tapioca is the suggested tool for generating RBI files for dependencies for more accurate type checking. I've removed the old sorbet directories and initialized tapioca.
In more recent versions of Sorbet, instantiating a class that's marked as abstract will raise an exception, so all of our specs that test the Set class directly are now failing. We can get the same functionality by moving those to shared examples and including them in the Intersection and Union specs. This required very little to change in the specs themselves.
Now that we have more type information for core and stdlib Ruby classes, Sorbet is complaining that we're potentially sending things that aren't Strings to Date.parse (whose first argument is said to be a String). From my POV, this is a problem in the Sorbet type system because marking the first argument for Date#parse as a String doesn't cover some built-in functionality for that method. If you use a non-String as the first argument, you'll get an error complaining about that type not having an "implicit conversion to String." This is a big feature of a number of Ruby parsing & conversion methods. Classes can signal that they can behave and be treated just like a String by responding to #to_str (this is different from the #to_s method, which is _explicit_ conversion). But that functionality isn't captured in the type system or by including a certain class. This is a feature that I think is worth continuing to support for a conversion method. As this branch is the fallback branch and we intend to catch errors where non-String and non-String-like objects are passed to Date#parse, we'll use the T.let(object, T.untyped) escape hatch to keep our intended functionality while appeasing Sorbet. I hope that one day Sorbet can support interfaces that revolve around responding to certain methods rather than class hierarchy alone. Until then, this will do for us.
First, for more recent versions of Sorbet, I had the type wrong for these splatted arguments (i.e. *elements). The type should be the type for each of the individual args, not the group of them. Once that was sorted out, we hit a new problem. Calling super in Intersection and Union initialize methods seemed to confuse the type system into thinking that it was passing along the type wrapped in an extra array. Normally, we could splat the elements before calling super, but that led to error 7019[^1], which pretty plainly says that splats aren't very well supported in Sorbet right now. The suggestion is to have the methods take an Array instead of splatted arguments. Since the tests explicitly cover these methods accepting both a single array and separate individual arguments, I'd rather not break that functionality for this type issue. Perhaps we'll go that route in the future, but for now we can fall back on the fact that Set is an abstract class. We can change it to take only a single Array argument since nothing should be calling it directly (and if it were, they'd be getting a type error). But we'll leave the child classes as accepting splatted args. Also, because Sorbet doesn't quite understand how Array() works (i.e. if you pass it an Array, it won't wrap that but return it as-is), we have to do this Array(object).flatten dance to make sure that Sorbet is clear on what we're dealing with. Again, I don't love this, but it works for now. The type system is happy and all the tests are passing. [^1]: https://sorbet.org/docs/error-reference#7019
Some gems we're using for development & test require Ruby 3.1 for the versions in our lockfile. Let's update the default version for all tasks to 3.1 for now to make a few things easier.
Some gem versions in the lockfile now require newer Ruby versions. To try and not break support for older versions just yet, let's see if we can get tests running by removing the lockfile before running bundle install (if it's there the setup-ruby command will run in deployment mode).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The primary impetus here is to get to a spot where I can actually release a new version with recent changes. To get to a version that I can
bundle installlocally on an Apple Silicon Mac, I need a more recent Sorbet version, which includes a few changes we need to account for.I'm also trying hard not to make these breaking changes, which required jumping through a few hoops, again, mostly related to Sorbet things. While there's a lot I like about Sorbet, these sorts of constraints (e.g. tooling nearly forcing Ruby version deprecations–or at least making some support quite difficult) do make me question whether the payoff is worth it. I'd like this gem to be as stable as possible for as long as possible. Hopefully this was the most painful of the transitions 🤞🏼