Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add name to error message in non-nil asserting getters #8200

Merged
merged 2 commits into from Oct 8, 2019

Conversation

@icy-arctic-fox
Copy link
Contributor

icy-arctic-fox commented Sep 19, 2019

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

This comment has been minimized.

Copy link
Member

straight-shoota commented Sep 19, 2019

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

This comment has been minimized.

Copy link
Contributor Author

icy-arctic-fox commented Sep 19, 2019

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

This comment has been minimized.

Copy link
Member

straight-shoota commented Sep 19, 2019

#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

This comment has been minimized.

Copy link
Contributor Author

icy-arctic-fox commented Sep 19, 2019

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

@j8r

This comment has been minimized.

Copy link
Contributor

j8r commented Sep 19, 2019

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

This comment has been minimized.

Copy link
Member

straight-shoota commented Sep 19, 2019

@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.

@icy-arctic-fox icy-arctic-fox force-pushed the icy-arctic-fox:not-nil-error-message branch from 982cc0f to 9eb78eb Sep 19, 2019
@icy-arctic-fox icy-arctic-fox changed the title Add variant of not_nil! that takes an error message Add name to error message in non-nil asserting getters Sep 19, 2019
@jhass
jhass approved these changes Sep 19, 2019
@RX14
RX14 approved these changes Sep 19, 2019
Copy link
Member

sdogruyol left a comment

Thanks @icy-arctic-fox 🙏

@RX14 RX14 added this to the 0.32.0 milestone Sep 20, 2019
@bcardiff bcardiff merged commit a6813e7 into crystal-lang:master Oct 8, 2019
6 checks passed
6 checks passed
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
ci/circleci: test_preview_mt Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Oct 8, 2019

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

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Oct 8, 2019

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

This comment has been minimized.

Copy link
Contributor

Sija commented Oct 8, 2019

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

@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Oct 9, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.