Expand union type support

[billy1kaplan]

Following up on our discussion here: #71

My first attempt at getting something to work for this. One interesting case that came up while testing this is that String is able to coerce pretty much anything (a hash, an integer, etc.). It seems like this could cause some confusing behavior around T.any(String, Integer). I kind of cheated my around this by rearranging the test. Another option would be to always check T.any in the order of T.any(...String, NilClass) if needed.

I'm definitely open to feedback on this approach!

[mattxwang]

Thanks @billy1kaplan, I'll try to get to this today. Something else has broken CI so I've opened a PR to resolve that first! (#74)

[codecov]

Codecov Report

Merging #73 (9db5500) into main (beefa45) will increase coverage by 0.00%.
The diff coverage is 95.45%.

@@           Coverage Diff           @@
##             main      #73   +/-   ##
=======================================
  Coverage   99.25%   99.25%           
=======================================
  Files           6        6           
  Lines         403      404    +1     
=======================================
+ Hits          400      401    +1     
  Misses          3        3           

Impacted Files	Coverage Δ
lib/sorbet-coerce/converter.rb	97.32% <88.88%> (-0.05%)	⬇️
spec/coerce_spec.rb	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[mattxwang]

Apologies for the late review on this! A couple of thoughts:

• re: the ordering with String that you discuss, I agree that this is a problematic use-case, and I want to have a behaviour that seems intuitive to users - swapping the order of the types in T.any shouldn't cause a behavioural change, in my opinion.
  • I personally find String to be the most lax type. I'm good with evaluating it last!
  • perhaps we should sort the types if they're not already sorted (I was under the impression that they were)
• I think the return of _convert_union being value and the sorbet_test_cases.rb failing are related - it seems like when raise_coercion_error is false, you're returning the value instead of the type!
• I would add a case or two that covers raise_coercion_error: false
• a small nit, but I would keep the wording consistent with context and avoid nesting when necessary
  • rspec generates sentences out of the tags: for example, one of your test cases will read "unsupported union types keeps the values as-is"

[billy1kaplan]

Thank you for the review!

I'll try to create an iteration on this early next week. I also saw the comment here: #71 (comment) and will make this an optional, opt-in feature.