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 'unreachable!' method to express unreachable code explicitly #4779

Closed

Conversation

makenowjust
Copy link
Contributor

@makenowjust makenowjust commented Aug 2, 2017

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.

Copy link
Contributor

@RX14 RX14 left a comment

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


describe "unreachable!" do
it "raises UnreachableError" do
expect_raises(UnreachableError){ unreachable! }
Copy link
Contributor

@RX14 RX14 Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be covered by the below expect_raises.

@makenowjust
Copy link
Contributor Author

makenowjust commented Aug 2, 2017

@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
Copy link
Contributor

RX14 commented Aug 2, 2017

@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
Copy link
Contributor Author

makenowjust commented Aug 2, 2017

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

@makenowjust
Copy link
Contributor Author

makenowjust commented Aug 2, 2017

Done...

Add `ASTNode#unreachable!` and `Lexer#unreachable!`
Copy link
Member

@straight-shoota straight-shoota left a comment

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.

@@ -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}"
Copy link
Member

@straight-shoota straight-shoota Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@makenowjust makenowjust Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@straight-shoota straight-shoota Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then what is the purpose of this type check anyway?

Copy link
Contributor Author

@makenowjust makenowjust Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@straight-shoota straight-shoota Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

unless func
raise "BUG: #{name} is not defined"
end
unreachable! "#{name} is not defined" unless func
Copy link
Member

@straight-shoota straight-shoota Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This too looks more like a runtime assertion.

Copy link
Contributor Author

@makenowjust makenowjust Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@straight-shoota straight-shoota Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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)
Copy link
Member

@straight-shoota straight-shoota Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@makenowjust makenowjust Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@makenowjust
Copy link
Contributor Author

makenowjust commented Aug 2, 2017

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

@straight-shoota
Copy link
Member

straight-shoota commented Aug 2, 2017

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
Copy link
Contributor Author

makenowjust commented Aug 2, 2017

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
Copy link
Member

straight-shoota commented Aug 2, 2017

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
Copy link
Contributor Author

makenowjust commented Aug 2, 2017

@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
Copy link
Member

straight-shoota commented Aug 2, 2017

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
Copy link
Contributor Author

makenowjust commented Aug 2, 2017

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

@makenowjust
Copy link
Contributor Author

makenowjust commented Aug 2, 2017

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

@straight-shoota
Copy link
Member

straight-shoota commented Aug 2, 2017

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

@makenowjust
Copy link
Contributor Author

makenowjust commented Aug 5, 2017

@RX14 🏓

@konovod
Copy link
Contributor

konovod commented Aug 28, 2017

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

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

perhaps it can be replaced with unreachable ?

@RX14
Copy link
Contributor

RX14 commented Aug 28, 2017

Is this even wanted now, because of #4837?

src/kernel.cr Outdated
# 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}")
Copy link
Member

@asterite asterite Sep 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@makenowjust makenowjust Sep 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@makenowjust
Copy link
Contributor Author

makenowjust commented Sep 19, 2017

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

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

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
Copy link
Member

straight-shoota commented Dec 9, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants