Add name to error message in non-nil asserting getters

[icy-arctic-fox]

Adds the getter's name to the error message when the non-nil assertion fails. This resolves issue #6431 . Affects getter!, property!, class_getter!, and class_property!.

class Foo
  getter! bar

  def initialize(@bar : Int32? = nil)
  end
end

Foo.new.bar # NilAssertionError: bar can't be nil

[straight-shoota]

The improved error message for getters is good.

But I'm not in support of adding a custom error message to #not_nil!. Using this method should be avoided as much as possible and it's generally considered a code smell. There are rare occasions where it's necessary, but these should really be the exception. So adding too much convenience seems counter-productive, as it might encourage using it.

Besides, you can usually just raise a custom exception:

foo.not_nil! "because bar"
foo || raise "because bar"

This has the same size, but offers more flexibility because you can fully customize the exception being raised.
It doesn't work with falsey Bool values, though. But calling .not_nil! on a Bool? type is pretty rare.

[icy-arctic-fox]

Not trying to argue for the #not_nil! variant, but rather have a discussion and provide use cases. I'm totally fine with removing it and just improving the getter! error message.

I'm not much of a fan of #not_nil! either, since it is discouraged to explicitly check for nil. In its current state, #not_nil! is not very useful, especially for debugging. The only message provided is "Nil assertion failed," which doesn't tell what or why. It would be much more helpful to have a message explaining it. But to your point, you can just raise your own exception.

#not_nil! is more explicit that nil should not be given.

nil.not_nil!("Nil not expected here")

It's also shorter than writing:

nil || raise NilAssertionError.new("Nil not expected here")

Which is equivalent to what it does now. If you care about the exception type, then it gets lengthy. I prefer the "or raise" case myself. Or for a workaround, I used:

if (parent = @parent)
  parent.do_something
else
  raise NilAssertionError.new("Parent node can't be nil")
end

Where #not_nil! is beneficial over the "or raise" approach is in method chaining. When searching through the Crystal repository for usages, using #not_nil! in a method chain is quite common. Most of the time though, the value is not nil, so this is a quick way to remove nil from the type list. But in the case it is nil, a "Nil assertion failed" message and a stack trace is produced, then it's up to the developer to figure out why. This is especially bad, for example, in a recursive data structure, like JSON, expecting a specific layout. So add a custom message, right?

(foo.bar || raise "bar can't be nil").baz
# or...
if (b = foo.bar)
  b.baz
else
  raise "bar can't be nil"
end
# or even...
def bar_not_nil
  foo.bar || raise "bar can't be nil"
end
bar_not_nil.baz

It gets quite ugly. An alternative:

foo.bar.not_nil!("bar can't be nil").baz

This is shorter and cleaner, but still kind of weird having a message thrown in there.

[straight-shoota]

#not_nil! shouldn't be used for data validation. Its single use case IMO is for when a nilable variable can't be nil in a specific context, but the compiler can't prove it.

[icy-arctic-fox]

Update this PR to just have improved error messages for getter! and related macros? or create a new one?

[j8r]

Not needed, the block variant of getter can already be used for that:

class Klass
  getter str : String { raise "oops" }
end

Even if getter! is in certain (rather rare) cases handy, we could easily live without it – and even better because one will be encouraged to specify an error message.

[straight-shoota]

@j8r Improving the error message for getter! is a good thing. The macro knows about the context (the name of the variable), so this should be included in the error message.

@icy-arctic-fox New PR or this one, your choice.

[sdogruyol]

Thanks @icy-arctic-fox 🙏

[bcardiff]

@straight-shoota why you consider this a breaking-change? The exception type raised is the same. The message changes but not its type.

[asterite]

I think it would be better to somehow include the type name in the message, like "Foo#bar was nil". Because seeing "name was nil" in a trace isn't much better than "something was nil" because "name" can appear in multiple types.

[Sija]

sth along {{@type}}{{method_prefix}}\{{name.id}} would be IMO ideal.

[straight-shoota]

@bcardiff Maybe because Exception was changed to NilAssertionError in the specs. But that's not breaking, the actual type didn't change.