crystal icon indicating copy to clipboard operation
crystal copied to clipboard

Fix `TypeNode#nilable?` for root types

Open HertzDevil opened this issue 3 years ago • 3 comments

This companion to #12353 makes TypeNode#nilable? return true for all supertypes of Nil:

{{ Value.nilable? }}  # => true
{{ Object.nilable? }} # => true

module Foo; end
struct Nil; include Foo; end
{{ Foo.nilable? }} # => true

Value and Object are mainly for theoretical correctness. The module case has little influence because TypeNode#nilable? is often used in tandem with Object#nil?, and the latter doesn't perform type filtering on variables of module types:

module Foo; end
struct Int32; include Foo; end
struct Nil; include Foo; end

# before:
(1.as(Foo)..nil.as(Foo)).sample # Error: no overload matches 'Int32#<' with type Foo
(1.as(Foo)..100.as(Foo)).sample # Error: no overload matches 'Int32#<' with type Foo

# after:
(1.as(Foo)..nil.as(Foo)).sample # raises ArgumentError
(1.as(Foo)..100.as(Foo)).sample # Stack overflow (e.g., infinite or very deep recursion)

It happens because of the following implementation:

struct Range(B, E)
  def sample(random = Random::DEFAULT)
    # ...
    {% elsif B.nilable? || E.nilable? %}
      b = self.begin
      e = self.end

      if b.nil? || e.nil?
        raise ArgumentError.new("Can't sample an open range")
      end

      # no type filtering occurs above; `b` still has type `Foo`
      # therefore the `Range` below is inferred to be `Range(Foo, Foo)`
      Range.new(b, e, @exclusive).sample(random)
    {% else %}
      super
    {% end %}
  end
end

Calling b.not_nil! works as the module is expanded into its including types during method lookup, but it looks awkward when a nil check is already present. (That said, this issue also happens without this PR, as Range#each(&) would show.)

The other use of TypeNode#nilable? is in JSON / YAML deserialization. This PR is inadequate because instance variables of module types will still complain about a missing or disallowed .new on module types.

HertzDevil avatar Aug 03 '22 15:08 HertzDevil

Thanks! But I don't understand... Value.nilable? should be false. Object.nilable? should be false. As I explained in the other PR, nilable? means it's either Nil, or it's a Union containing the type Nil. For Value and Object that doesn't hold.

asterite avatar Aug 03 '22 21:08 asterite

Oh, I guess Value can be nil as one of its sublcasses is Nil... Hmm...

I guess I never thought about including a module in Nil, then using that module in a range. But maybe there's a valid use case for that... 🤔

asterite avatar Aug 03 '22 21:08 asterite

For illustration, this is what we're talking about:

def foo(v : Value)
  v.nil? # => true
end
def bar(o : Object)
  o.nil? # => true
end
foo nil
bar nil

straight-shoota avatar Aug 03 '22 21:08 straight-shoota