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

Autocasting from Bool? to Bool #12704

Open
straight-shoota opened this issue Oct 31, 2022 · 14 comments
Open

Autocasting from Bool? to Bool #12704

straight-shoota opened this issue Oct 31, 2022 · 14 comments

Comments

@straight-shoota
Copy link
Member

straight-shoota commented Oct 31, 2022

This idea is inspired by #12702: A lot of predicate methods are returning Bool | Nil when just Bool would suffice. Removing Nil makes the API more concise. Looking at the diff of that PR, it consists of prefixing !! to many return expressions for predicate methods. This double negation maps nil to false, thus removing the Nil type from the union.

What if instead of altering the implementations, we introduce an autocast from Bool? to Bool? At least for method return values?
Nil has a singleton value nil and it is falsey. Casting Nil to Bool is clearly defined.

Random example from that PR results in this method:

def has_def_without_parents?(name)
  !!defs.try(&.has_key?(name))
end

With Bool? autocasting, we could write it like this:

def has_def_without_parents?(name) : Bool
  defs.try(&.has_key?(name))
end

I think that would be more elegant.

Both examples are semantically equivalent. But the type restriction has other benefits (documentation and tying down the API). So considering we want to add return types in many places anyways, autocasting would avoid having to write !! as well.

@asterite
Copy link
Member

The main question is: why? What's the benefit of using Bool over Bool | Nil?

I think the answer might be around "it's more efficient to use Bool over Bool | Nil, and it might be faster to compile." But in my experiments changing methods to return Nil, or doing such simplifications, yielded to no visible benefit. So eventually I stopped caring about such micro-optimizations.

So I wouldn't introduce a semantic change without a proof that it has a meaningful impact.

@HertzDevil
Copy link
Contributor

Those changes were about correctness more than performance. A Bool has two states and a Bool? has three, yet those predicates never return a third state unlike, for example, BitArray#[]?. If I am debugging something and see a Bool? local variable I do not want to wonder why a certain predicate method implies the existence of a third state. Removing those Nils also cleans up the generated LLVM a bit.

We can cast anything to a Bool with !!. I wonder if it is too easy to make mistakes though:

def foo(x : Bool)
end

foo(true.as?(Bool?))
foo(nil)                # okay?
foo(Pointer(Void).null) # okay?
foo(1)                  # okay??

In particular it would be really odd if nil.as(Bool?) autocasts to false but nil doesn't.

Ruby RBS has a boolish type that is equivalent to the top type but indicates that only the object's truthiness is relevant. If we have something similar then maybe we could autocast anything to it (but then it is unclear why we need it to be distinct from the usual Bool).

@straight-shoota
Copy link
Member Author

I mentioned the motivation in the OP:

Removing Nil makes the API more concise.

If the purpose of a value is to express truthiness, having two values for representing falsey (false and nil) is redundant. As @HertzDevil mentioned in the previous comment Bool | Nil can represent three states where nil may have a distinct meaning from false. But if there's only two states (as with typical predicate methods), the Bool type should suffice. Using Bool | Nil makes the reader wonder about the significance of nil.

@straight-shoota
Copy link
Member Author

@HertzDevil In general, I don't think autocasting anything to Bool makes much sense. For a method argument, an explicit cast is probably always better.
My proposal is specifically for method return types, i.e. the use case presented here and in #12702.

@HertzDevil
Copy link
Contributor

HertzDevil commented Oct 31, 2022

Autocasting is currently never done for return values. This is entirely something new. I think it is a bad idea to have two sets of autocasting rules for return values and other restrictions. And my concern still stands:

def foo : Bool
  nil.as(Bool?)
end

def foo : Bool
  nil # okay?
end

def foo : Bool
  # okay?
end

Besides, at this point --error-trace should point to the Nil source if for whatever reason a method returns Bool?.

@asterite
Copy link
Member

I guess what I'm saying is that if you want to make sure to return Bool you can add a type annotation. If you only care about truthiness, then returning Bool | Nil works because nil and false are falsey, and true is truthy. It doesn't matter that we have three states. We don't. We just have two ways to refer to a falsey thing. I personally don't find it confusing. It's the same way in Ruby.

I would actually find it confusing if nil suddenly became false due to some autocasting rule, but nil doesn't become false in other places, etc.

I think introducing such a rule might cause more confusion than benefit.

@straight-shoota
Copy link
Member Author

Autocasting is currently never done for return values.

It's maybe not exactly autocasting, but the idea was inspired by the return type restriction Nil which enforces that the returned value is always nil, regardless of the body implementation.

@beta-ziliani
Copy link
Member

FWIW I would consider a good idea to have a method to do the equivalent to !! in a more idiomatic way. Object#to_b maybe?

@oprypin
Copy link
Member

oprypin commented Oct 31, 2022

I think @straight-shoota 's arguments are fully sound and I fully support the idea.
Nil and Bool are the only types that have falsey values (well, not exactly, but I wish they were), so it's probably not a coincidence that this conversion will ever be added only for them.
Let's not call this autocasting.
Let's just say that a : Bool annotation turns last_expression into !!last_expression just like a : Nil annotation turns it into last_expression; nil.

And this might be backwards compatible? Of course, other than the case when someone was actually comparing to nil 😕

@oprypin
Copy link
Member

oprypin commented Oct 31, 2022

It could be implemented as a syntax sugar transform, like the following:

def foo : Bool
    body
end
def foo : Bool
  !!begin
    body
  end
end

The fact that return x will not allow nils under this implementation can be considered a bonus. Anyway, what to do with explicit returns should definitely be clarified.

@straight-shoota
Copy link
Member Author

If we want explicit returns to be an error then it can't just be syntax sugar.

For the Nil annotation, explicit returns with non-Nil values are actually allowed:

def foo : Nil
  return 1
end

That's how it works for Nil currently. I don't think that's good, though.

@oprypin
Copy link
Member

oprypin commented Nov 1, 2022

If we want explicit returns to be an error then it can't just be syntax sugar.

I meant only a wrong explicit return - this certainly errors:

def foo : Bool
  !!begin
    return 6
    nil
  end
end

Anyway maybe it's not a good way to implement it.

And yes, I was fearing that part about : Nil, thanks for checking it.

@asterite
Copy link
Member

asterite commented Nov 1, 2022

I don't think the !!begin ... end trick works. Or, like, it works too much.

This:

def foo : Bool
  1
end

would get transformed to this:

def foo : Bool
  !!begin
    1
  end
end

which actually typechecks, but it's probably not what you want.

@oprypin
Copy link
Member

oprypin commented Nov 1, 2022

FWIW I would consider a good idea to have a method to do the equivalent to !! in a more idiomatic way. Object#to_b maybe?

That actually sounds concerning to me, depending on the approach, because people might start trying to give it special meanings

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

No branches or pull requests

5 participants