Add 'unreachable!' method to express unreachable code explicitly #4779

makenowjust · 2017-08-02T10:27:58Z

This is an implementation of #4749.

This PR introduces a new error class UnreachableError and unreachable! method raises this error.

~~After merging this, I'll try to fix to use unreachable! instead of raise "BUG: ..." in stdlib and compiler source code.~~

RX14

I think it would be best to start using unreachable! in this PR, just in a seperate commit.

RX14 · 2017-08-02T10:41:25Z

spec/std/unreachable_spec.cr

+
+describe "unreachable!" do
+  it "raises UnreachableError" do
+    expect_raises(UnreachableError){ unreachable! }


This seems to be covered by the below expect_raises.

This is an implementation of crystal-lang#4749.

makenowjust · 2017-08-02T11:02:21Z

@RX14 I don't think it is best, however I'll try it...

There are many raise "BUG: ..." under src/compiler/crystal. However most these raise cannot be replaced to unreachable! because this raise is ASTNode#raise.

RX14 · 2017-08-02T11:04:07Z

@makenowjust Surely you'd add an ASTNode#unreachable! too to solve this? I don't think spreading changes over 2 PRs is beneficial, as long as it's separable into commits (add new method, refactor to use it) it's a single logical change and should be 1 PR.

makenowjust · 2017-08-02T11:05:33Z

@RX14 Yes. I am working now in this way.

makenowjust · 2017-08-02T11:36:28Z

Done...

Add `ASTNode#unreachable!` and `Lexer#unreachable!`

straight-shoota

I would characterize the use of unreachable! as more restrictive than you did in a few of your substitutions. I think it should mostly be used to declare entire methods or else branches of a type distinction as unreachable.
If an error is raised based on the non-existence or value of specific variables is not unreachable code but a runtime assertion. I didn't mark all of them but wanted to hear some other thoughts.

straight-shoota · 2017-08-02T12:34:10Z

src/compiler/crystal/codegen/debug.cr

@@ -186,7 +186,7 @@ module Crystal
      # any case, macro code *should* be simple so that it doesn't
      # need to be debugged at runtime (because macros work at compile-time.)
      unless filename.is_a?(String)
-        raise "BUG: expected debug filename to be a String, not #{filename.class}"
+        unreachable! "expected debug filename to be a String, not #{filename.class}"


I don't think this should be unreachable!. It looks more like an assertion.

I checked this code and all points calling file_and_dir, then I think here is unreachable surely. file_and_dir is called by 5 points: L12, L157, L226, L259, L270. L12 passes String to file_and_dir directly. L259 passes filename variable, which is typed String for now. L157, L226 and L270 pass location.filename, however this location comes from location.original_location. original_location.filename is always String.

Then what is the purpose of this type check anyway?

Unfortunately location.filename's type is String | VirtualFile although location is original_location.

Still, this is not unreachable code - it just checks for an invalid argument.

straight-shoota · 2017-08-02T12:34:43Z

src/compiler/crystal/codegen/fun.cr

-    unless func
-      raise "BUG: #{name} is not defined"
-    end
+    unreachable! "#{name} is not defined" unless func


This too looks more like a runtime assertion.

See #4779 (comment). I think it is related.

Yes, that's the same thing I am concerned about.

straight-shoota · 2017-08-02T12:37:43Z

src/compiler/crystal/semantic/ast.cr

@@ -13,6 +13,10 @@ module Crystal
      ::raise exception_type.for_node(self, message, inner)
    end

+    def unreachable!(message = "unreachable", inner = nil, exception_type = Crystal::TypeException)
+      ::raise exception_type.for_node(self, "BUG: #{message}", inner)


Shouldn't this raise an UnreachableError to be compatible with ::unreachable!? Otherwise it gets confusing.

It is difficult to raise UnreachableError with keeping node information. Perhaps we will feel confused in future in debug if disappear node trace.

makenowjust · 2017-08-02T12:52:14Z

Should Object#not_nil! use unreachable!? I think the function of not_nil! looks like unreachable!. How do you think?

straight-shoota · 2017-08-02T12:59:46Z

The purpose of unreachable is to mark unreachable code. Variables guarded by not_nil! are not unreachable this rather serves to detect and eventually raise in case of an invalid program state.

makenowjust · 2017-08-02T16:55:00Z

I want to revert ba47338 for now. I think all raise "BUG: in stdlib and the compiler source code are runtime assertion said by @straight-shoota. Perhaps, the compiler authors know the problem explained in #4749 and hide it by their technique. Maybe there is code in which using unreachable! is smarter, but it is another issue, and please open a new pull request if you found such a code, I think.

@RX14 Please close your "requested changes".

straight-shoota · 2017-08-02T18:42:14Z

Well looking closer at the individual instances I mostly agree: Almost all uses are actually not unreachable code. For example if a case statement switches over strings or other arbitrary values, raising in an else is just safeguarding against invalid arguments. Using unreachable! in this context makes only sense when switching over a closed set, i.e. an enum.
But I would suggest to use unreachable! where a method just exists to provide an interface but should never receive a call.

makenowjust · 2017-08-02T20:58:29Z

@straight-shoota

Using unreachable! in this context makes only sense when switching over a closed set, i.e. an enum.

Yes, and union type is also.

def string_or_int32(value : String | Int32)
  case value
  when String then :string
  when Int32  then :int32
  else             unreachable!
  end
end

But I would suggest to use unreachable! where a method just exists to provide an interface but should never receive a call.

Why not use abstract def?

straight-shoota · 2017-08-02T21:42:45Z

Yeah, union types are another usecase.

I mean methods that could from the compilers perspective technically receive a call and must exist for some reason, but should never actually be called.

This could be the case when a supertype has a method that makes no sense in the subtype - the subtype must still implement it (and default to parent definition).
You could argue this smells like a design failure but sometimes it is just a practical solution and not so bad at all. So it's similar to a "not implemented" method (for example #read in a write-only IO like String::Builder) but essentially impossible that the method could ever be called on that specific type.

makenowjust · 2017-08-02T22:20:24Z

Where is such a case in this repository? or is there such a case in this repository?

This reverts commit ba47338.

makenowjust · 2017-08-02T22:23:22Z

@straight-shoota You thought #4775 (comment)?

straight-shoota · 2017-08-02T22:27:26Z

For example. I am not sure if there is something similar in the compiler repo.

makenowjust · 2017-08-05T03:11:29Z

@RX14 🏓

konovod · 2017-08-28T10:47:34Z

There is a raise "" used to remove Iterator::Stop type from a union.

crystal/src/iterator.cr

Line 139 in 98af739

raise "" if e.is_a?(Iterator::Stop)

perhaps it can be replaced with unreachable ?

RX14 · 2017-08-28T18:57:35Z

Is this even wanted now, because of #4837?

asterite · 2017-09-10T15:06:41Z

src/kernel.cr

+# When it is called, it raises `UnreachableError` with given *message*.
+# However you will never see it if you use this method accurately.
+def unreachable!(message = "unreachable") : NoReturn
+  raise UnreachableError.new("BUG: #{message}")


I wouldn't hardcode "BUG: ..." in every unreachable message. Please move "Bug: " inside the default value.

makenowjust · 2017-09-19T04:44:53Z

@RX14 I think it is needed also after #4837. Because Rust has exhaustiveness check but also has unreachable.

ysbaddaden

I won't object if it eventually gets merged, but my vote goes to reject this.

This brings zero improvement over the the actual raise "unreachable". It's even encouraging to use an unhelpful generic message, whereas manually raising could push to have an helpful and explanatory message instead.

straight-shoota · 2020-12-09T19:58:11Z

Closing. This is mostly superseeded by exhaustive case.
cf. #9988

RX14 requested changes Aug 2, 2017

View reviewed changes

makenowjust force-pushed the feature/unreachable branch from afd168c to b50a12b Compare August 2, 2017 10:48

Add 'unreachable!' method to express unreachable code explicitly

a9b6e56

This is an implementation of crystal-lang#4749.

makenowjust force-pushed the feature/unreachable branch from b50a12b to a9b6e56 Compare August 2, 2017 10:54

Replace 'raise "BUG: ' with 'unreachable!'

ba47338

Add `ASTNode#unreachable!` and `Lexer#unreachable!`

makenowjust force-pushed the feature/unreachable branch from 2f7fc61 to ba47338 Compare August 2, 2017 12:20

straight-shoota reviewed Aug 2, 2017

View reviewed changes

Revert "Replace 'raise "BUG: ' with 'unreachable!'"

ef24559

This reverts commit ba47338.

asterite requested changes Sep 10, 2017

View reviewed changes

Remove "BUG:" prefix from unreachable message

05f8068

ysbaddaden reviewed Oct 6, 2017

View reviewed changes

straight-shoota closed this Dec 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'unreachable!' method to express unreachable code explicitly #4779

Add 'unreachable!' method to express unreachable code explicitly #4779

makenowjust commented Aug 2, 2017 •

edited

Loading

RX14 left a comment

RX14 Aug 2, 2017

makenowjust commented Aug 2, 2017

RX14 commented Aug 2, 2017 •

edited

Loading

makenowjust commented Aug 2, 2017 •

edited

Loading

makenowjust commented Aug 2, 2017

straight-shoota left a comment •

edited

Loading

straight-shoota Aug 2, 2017

makenowjust Aug 2, 2017

straight-shoota Aug 2, 2017

makenowjust Aug 2, 2017

straight-shoota Aug 2, 2017

straight-shoota Aug 2, 2017

makenowjust Aug 2, 2017

straight-shoota Aug 2, 2017

straight-shoota Aug 2, 2017

makenowjust Aug 2, 2017

makenowjust commented Aug 2, 2017 •

edited

Loading

straight-shoota commented Aug 2, 2017 •

edited

Loading

makenowjust commented Aug 2, 2017

straight-shoota commented Aug 2, 2017

makenowjust commented Aug 2, 2017

straight-shoota commented Aug 2, 2017

makenowjust commented Aug 2, 2017

makenowjust commented Aug 2, 2017

straight-shoota commented Aug 2, 2017

makenowjust commented Aug 5, 2017

konovod commented Aug 28, 2017

RX14 commented Aug 28, 2017

asterite Sep 10, 2017

makenowjust Sep 19, 2017

makenowjust commented Sep 19, 2017

ysbaddaden left a comment

straight-shoota commented Dec 9, 2020

Add 'unreachable!' method to express unreachable code explicitly #4779

Add 'unreachable!' method to express unreachable code explicitly #4779

Conversation

makenowjust commented Aug 2, 2017 • edited Loading

RX14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makenowjust commented Aug 2, 2017

RX14 commented Aug 2, 2017 • edited Loading

makenowjust commented Aug 2, 2017 • edited Loading

makenowjust commented Aug 2, 2017

straight-shoota left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makenowjust commented Aug 2, 2017 • edited Loading

straight-shoota commented Aug 2, 2017 • edited Loading

makenowjust commented Aug 2, 2017

straight-shoota commented Aug 2, 2017

makenowjust commented Aug 2, 2017

straight-shoota commented Aug 2, 2017

makenowjust commented Aug 2, 2017

makenowjust commented Aug 2, 2017

straight-shoota commented Aug 2, 2017

makenowjust commented Aug 5, 2017

konovod commented Aug 28, 2017

RX14 commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makenowjust commented Sep 19, 2017

ysbaddaden left a comment

Choose a reason for hiding this comment

straight-shoota commented Dec 9, 2020

makenowjust commented Aug 2, 2017 •

edited

Loading

RX14 commented Aug 2, 2017 •

edited

Loading

makenowjust commented Aug 2, 2017 •

edited

Loading

straight-shoota left a comment •

edited

Loading

makenowjust commented Aug 2, 2017 •

edited

Loading

straight-shoota commented Aug 2, 2017 •

edited

Loading