`var.should_not be_nil` does not eliminate var of Nil from subsequent type inference

Help & Support
1

This spec code compiles correctly:

    fail("bad") if node.nil?
    hr = node.xpath_node("hr")

The compiler does not complain that Nil#xpath_node does not exist.

However, this code fails to compile:

    node.should_not be_nil
    hr = node.xpath_node("hr")

It barfs “Error: undefined method ‘xpath_node’ for Nil (compile-time type is (XML::Node | Nil))”

Apparently the type inference system sees that fail("bad") if node.nil? eliminates the Nil type from subsequent uses of node, but the equivalent line node.should_not be_nil does not.

Is this a bug? and should I report it to the proper authorities?

2

It’s not a bug, and I don’t think there’s anything we can do to improve this.

You’ll have to do node.not_nil!.xpath_node("hr")

3

How it couldn’t be a bug, if the two statements are the same - raise an exception if node is nil?

4

They’re not really the same. The first example is basically the same as raise "bad" if node.nil?, while the logic in should_not is in an if statement that accepts any expectation, so there is a state that could result in there not being an exception, thus the value could be nil. There would have to be special logic in that if statement specific to the be_nil expectation to account for that. But your assertions should not be used as control logic.

5

We could add a new assert_not_nil value that returns the non-nil version. Same with assert_type x, Type that would return the value casted to that type.

:bulb: Actually, we could override should_not against the be_nil expectation to return a non-nil value!
:bulb: And maybe do the same with be!

I’ll try it later and send a PR if it’s possible.

Sorry for saying that there’s nothing we can do.

I actually needed this myself a couple of times :-)

4 Likes
6

Here it is! https://github.com/crystal-lang/crystal/pull/8240

You’ll be able to do:

node = node.should_not be_nil
hr = node.xpath_node("hr")

It’s still not magical (you need to assign the result of the should call), but I think it’s still better than the current state.

5 Likes