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

Explosive birth of 'case!' #4870

Conversation

makenowjust
Copy link
Contributor

case! looks like case, but it checks exhaustiveness like Scala and Rust.

case! spec:

  1. case! checks exhaustiveness for enums, union types, Bool (true or false) and nil.

    foo = true ? 42 : "foo"
    
    case! foo
    when  Int32
      p :int32
    end
    
    # Error in foo.cr:3: found non-exhaustive pattern: String
  2. Not such a type (e.g. Int32, String) cannot check exhaustiveness, but case! raises a compilation error in this case. It means:

    foo = 42
    
    case! foo
    when  42
      p :foo
    end
    
    # Error in foo.cr:3: found non-exhaustive pattern: Int32

    But, this case can be compiled:

    foo = 42
    
    case! foo
    when  Int32
      p :foo
    end
  3. Of course tuple conditions can be checked:

    foo = true ? 42 : "foo"
    bar = true ? 3.14 : :bar
    
    case! {foo, bar}
    when  {String, _}
      p :left_string
    when  {_, Symbol}
      p :right_symbol
    end
    
    # Error in foo.cr:4: found non-exhaustive pattern: {Int32, Float64}
  4. case! must have a condition, it's a syntax error:

    case!
    when  true
    end
    
    # Syntax error in foo.cr:2: unexpected token: when ('case!' must have a condition)
  5. case! must not have else clause, and can use when _ instead:

    # foo = true ? 42 : "foo"
    #
    # case! foo
    # when  Int32
    #   p :int32
    # else
    #   p :others
    # end
    #
    # # Syntax error in foo.cr:6: unexpected token: else ('case!' cannot have 'else', use 'when _' instead)
    
    foo = true ? 42 : "foo"
    
    case! foo
    when  Int32
      p :int32
    when  _
      p :others
    end
  6. crystal tool format appends 2 spaces after when of case! to align when conditions. For example:

    foo = true ? 42 : "foo"
    case! foo
    when  Int32
      p :int32
    when  String
      p :string
    end

    It is my preference, but I believe this is more beautiful than that:

    foo = true ? 42 : "foo"
    case! foo
    when Int32
      p :int32
    when String
      p :string
    end

@makenowjust
Copy link
Contributor Author

Goddamn!! Why this p-r number is not 4869!?!? 4869 is the special number for me, so I desire to get this. Ah...

@makenowjust makenowjust force-pushed the feature/crystal/optional-exhaustiveness-check branch from e206566 to e249e75 Compare August 22, 2017 15:48
@@ -927,6 +927,10 @@ describe "Parser" do
assert_syntax_error "case {1, 2}; when {3}; 4; end", "wrong number of tuple elements (given 1, expected 2)", 1, 19
assert_syntax_error "case 1; end", "unexpected token: end (expecting when or else)", 1, 9

it_parses "case! a\nwhen Int32\n1\nend", Case.new("a".call, [When.new(["Int32".path] of ASTNode, 1.int32)], check_exhaustiveness: true)
assert_syntax_error "case!\nwhen Int32\n1\nend", "unexpected token: when ('case!' must have a condition)", 2, 1
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the error for this should be: unexpected token: when ('case!' must have an expression) (not necessarily a condition).

@@ -108,4 +108,9 @@ describe "ASTNode#to_s" do
expect_to_s %(1 <= 2 <= 3)
expect_to_s %((1 <= 2) <= 3)
expect_to_s %(1 <= (2 <= 3))

expect_to_s %(case\nwhen 1\nwhen 2\nend)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering, what is the use of a case not followed by an expression, shouldn't this be disallowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's case, not case!. There is no case case in to_s_spec.cr, so I added them also.

Copy link
Contributor

Choose a reason for hiding this comment

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

A case without an expression is an if elseif elseif but with conditions at the same indentation level:

if x
elsif y
elsif z
end

case
when x
when y
when z
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @ysbaddaden

@RX14
Copy link
Contributor

RX14 commented Aug 22, 2017

Please, let's merge #4837 instead.

property! parent_visitor : MainVisitor

# `nil` means 'not checked'. `true` means 'this case is exhaustive'. `false` means 'it cannot check exhaustiveness'.
property? exhaustive : Bool? = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use an Enum here?

@makenowjust makenowjust force-pushed the feature/crystal/optional-exhaustiveness-check branch from e249e75 to 7cc59b6 Compare August 22, 2017 16:35
@makenowjust
Copy link
Contributor Author

@RX14 Yes, I think so too, but I can't (I don't have the logic to) convince others.

@akzhan
Copy link
Contributor

akzhan commented Sep 20, 2017

I really think that's area for after 1.0 after wide accepting of community.

#4837 looks better, but I'm lazy to write else branch, so 👎 for #4837.

@makenowjust makenowjust force-pushed the feature/crystal/optional-exhaustiveness-check branch from 7cc59b6 to b0c33ba Compare April 30, 2018 03:43
@asterite asterite mentioned this pull request Jul 26, 2019
@asterite asterite mentioned this pull request Nov 2, 2019
4 tasks
@Blacksmoke16
Copy link
Member

This can probably be closed now that #8424 is merged?

@bcardiff bcardiff closed this Mar 30, 2020
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

7 participants