Add missing branches to make exhaustiveness check happy

[Sija]

Followup to #8678

[Sija]

Now when I'm looking at it there's still a when nil branch missing but compiler doesn't seem to complain...

Using compiled compiler at /Users/sija/Code/crystal/.build/crystal
In /Users/sija/Code/crystal/src/http/client.cr:132:5

 132 | {% if flag?(:without_openssl) %}
       ^
Warning: expanding macro


There was a problem expanding macro 'macro_4835757792'

Called macro defined in /Users/sija/Code/crystal/src/http/client.cr:132:5

 132 | {% if flag?(:without_openssl) %}

Which expanded to:

   1 |
 > 2 |       @tls = case tls
   3 |              when true
   4 |                OpenSSL::SSL::Context::Client.new
   5 |              when OpenSSL::SSL::Context::Client
   6 |                tls
   7 |              end
   8 |
Warning: case is not exhaustive.

Missing cases:
 - false

[asterite]

Is nil passed in your calling code? Otherwise the compiler won't complain. The compiler checks actual values. If you specify a default value but it's not used, the compiler doesn't use it.

[asterite]

I guess we need to handle nil too, even if the compiler (correctly) doesn't complain about it.

[asterite]

I wonder why we didn't see this warning before... I think a change to this file was merged after we merge the exhaustiveness check. When I compile the specs for http client I get this:

In src/http/client.cr:132:5

 132 | {% if flag?(:without_openssl) %}
       ^
Warning: expanding macro


There was a problem expanding macro 'macro_4640824944'

Called macro defined in src/http/client.cr:132:5

 132 | {% if flag?(:without_openssl) %}

Which expanded to:

   1 |
 > 2 |       @tls = case tls
   3 |              when true
   4 |                OpenSSL::SSL::Context::Client.new
   5 |              when OpenSSL::SSL::Context::Client
   6 |                tls
   7 |              end
   8 |
Warning: case is not exhaustive.

Missing cases:
 - Nil

In src/http/client.cr:132:5

 132 | {% if flag?(:without_openssl) %}
       ^
Warning: expanding macro


There was a problem expanding macro 'macro_4678149552'

Called macro defined in src/http/client.cr:132:5

 132 | {% if flag?(:without_openssl) %}

Which expanded to:

   1 |
 > 2 |       @tls = case tls
   3 |              when true
   4 |                OpenSSL::SSL::Context::Client.new
   5 |              when OpenSSL::SSL::Context::Client
   6 |                tls
   7 |              end
   8 |
Warning: case is not exhaustive.

Missing cases:
 - Nil

In src/http/client.cr:132:5

 132 | {% if flag?(:without_openssl) %}
       ^
Warning: expanding macro


There was a problem expanding macro 'macro_4678181648'

Called macro defined in src/http/client.cr:132:5

 132 | {% if flag?(:without_openssl) %}

Which expanded to:

   1 |
 > 2 |       @tls = case tls
   3 |              when true
   4 |                OpenSSL::SSL::Context::Client.new
   5 |              when OpenSSL::SSL::Context::Client
   6 |                tls
   7 |              end
   8 |
Warning: case is not exhaustive.

Missing cases:
 - false

In src/http/client.cr:132:5

 132 | {% if flag?(:without_openssl) %}
       ^
Warning: expanding macro


There was a problem expanding macro 'macro_4678825280'

Called macro defined in src/http/client.cr:132:5

 132 | {% if flag?(:without_openssl) %}

Which expanded to:

   1 |
 > 2 |       @tls = case tls
   3 |              when true
   4 |                OpenSSL::SSL::Context::Client.new
   5 |              when OpenSSL::SSL::Context::Client
   6 |                tls
   7 |              end
   8 |
Warning: case is not exhaustive.

Missing cases:
 - false

In src/http/client.cr:132:5

 132 | {% if flag?(:without_openssl) %}
       ^
Warning: expanding macro


There was a problem expanding macro 'macro_4713403040'

Called macro defined in src/http/client.cr:132:5

 132 | {% if flag?(:without_openssl) %}

Which expanded to:

   1 |
 > 2 |       @tls = case tls
   3 |              when true
   4 |                OpenSSL::SSL::Context::Client.new
   5 |              when OpenSSL::SSL::Context::Client
   6 |                tls
   7 |              end
   8 |
Warning: case is not exhaustive.

Missing cases:
 - false

In src/http/client.cr:132:5

 132 | {% if flag?(:without_openssl) %}
       ^
Warning: expanding macro


There was a problem expanding macro 'macro_4714974048'

Called macro defined in src/http/client.cr:132:5

 132 | {% if flag?(:without_openssl) %}

Which expanded to:

   1 |
 > 2 |       @tls = case tls
   3 |              when true
   4 |                OpenSSL::SSL::Context::Client.new
   5 |              when OpenSSL::SSL::Context::Client
   6 |                tls
   7 |              end
   8 |
Warning: case is not exhaustive.

Missing cases:
 - Nil

In src/http/client.cr:132:5

 132 | {% if flag?(:without_openssl) %}
       ^
Warning: expanding macro


There was a problem expanding macro 'macro_4734511408'

Called macro defined in src/http/client.cr:132:5

 132 | {% if flag?(:without_openssl) %}

Which expanded to:

   1 |
 > 2 |       @tls = case tls
   3 |              when true
   4 |                OpenSSL::SSL::Context::Client.new
   5 |              when OpenSSL::SSL::Context::Client
   6 |                tls
   7 |              end
   8 |
Warning: case is not exhaustive.

Missing cases:
 - Nil

In src/http/client.cr:132:5

 132 | {% if flag?(:without_openssl) %}
       ^
Warning: expanding macro


There was a problem expanding macro 'macro_4711526624'

Called macro defined in src/http/client.cr:132:5

 132 | {% if flag?(:without_openssl) %}

Which expanded to:

   1 |
 > 2 |       @tls = case tls
   3 |              when true
   4 |                OpenSSL::SSL::Context::Client.new
   5 |              when OpenSSL::SSL::Context::Client
   6 |                tls
   7 |              end
   8 |
Warning: case is not exhaustive.

Missing cases:
 - Nil

A total of 8 warnings were found.

It's a bit of a mess but it's fine: in the release after 0.34.0 you'll get an error for these, and you'll only see one of them (like all other errors). Then you'll have to add nil, then false and you'll be done.

[Sija]

@asterite I've added an else branch which handles both of those cases.

[asterite]

I think we should add the explicit cases false and nil. Then if we decide to add new cases the compiler will tell us. That's the benefit of the change. So we should try to avoid adding else if possible

[Sija]

@asterite ok, done

[j8r]

@asterite I rebased master yesterday, but I didn't notice the warnings since the CI was green.

[asterite]

@j8r It's fine :-)

I was worried for a minute that the check was working correctly, but it was just that this was merged after I merged my PR.