modularml / mojo

The Mojo Programming Language
https://docs.modular.com/mojo/manual/
Other
23.37k stars 2.6k forks source link

[BUG] Implicit conversion from `Boolable` to `Bool` lead to surprising behaviour #2860

Closed soraros closed 5 months ago

soraros commented 6 months ago

Bug description

As title. I think implicit conversion should be used much more sparingly.

@lattner @laszlokindrat, since you made the relevant changes.

Steps to reproduce

fn main():
  print(UInt8('test') == 1) # prints True

This is because SIMD has a constructor from Bool, and String/StringLiteral is Boolable. It's easy enough to find out how this works, but I find it difficult to justify why it should work at all in the first place.

System information

N/A
laszlokindrat commented 6 months ago

I agree that this behavior might be surprising, but I can't see the harm yet. At the same time, implicit conversion to Bool and Int is often useful and we don't always want to type Boolable/Indexer. This does make me wonder, though, if we are missing a different abstraction: similarly to how Int has Intable and Indexer, maybe we need two different traits for Bool as well?

To mitigate this particular case, we could just add an always-failing constructor from String/StringLiteral to SIMD.

bgreni commented 6 months ago

I agree that this behavior might be surprising, but I can't see the harm yet.

This could be annoying/cause bugs if I someone passes a string that "looks" like an integer since they might expect it to match the behaviour of int.

print(Int8('100')) # prints 1
print(int('100')) # prints 100

I believe these are the kinds of footguns we should aim to avoid in an nascent language like Mojo.

laszlokindrat commented 6 months ago

I agree that this behavior might be surprising, but I can't see the harm yet.

This could be annoying/cause bugs if I someone passes a string that "looks" like an integer since they might expect it to match the behaviour of int.

print(Int8('100')) # prints 1
print(int('100')) # prints 100

I believe these are the kinds of footguns we should aim to avoid in an nascent language like Mojo.

Again, this can be mitigated by an always-failing String constructor on SIMD and other numeric types. Once we have more powerful traits (e.g. conditional conformance or parametric traits), we could also just prevent non-bool SIMD types from being implicitly constructed from bool. That being said, we are working on improving the implicit conversion story in Mojo, so there might be functionality we can implement to help control these situations better.

JoeLoser commented 5 months ago

We discussed this in our team design meeting on 6/10/24. Here are some raw notes in case you're curious.

With this Boolable thing, there's sort of three things to consider:

Perhaps issue in discerning between these three things

  1. implicitly convertible or losslessly convertible
    1. Analogous to Indexer
  2. coercible to bool (can call bool(my_value))
    1. Like Intable
  3. can put it into an “if statement” (C++ calls this guy “contextually convertible to bool”)
    1. Don’t model separately, should we think of this as a different thing? Never thought about this; in Python, same as (2) — is that the right way to model that?

One approach to fix these issues is to introduce a new trait (ImplicitlyBoolConvertable)so we change the implicit conversion rules for bool. We can restrict it to numeric types for example to avoid problems with unintentional conversions. @laszlo is running with this I believe.

gabrieldemarmiesse commented 5 months ago

@JoeLoser Do we have a timeline for when the compiler will stop auto-converting everything when a constructor with one argument is available? That's a major pain right now.

soraros commented 5 months ago

Related, I think it's important that we pin down the semantics of e.g. if statements

if exp:
  ...
else:
  ...

and make it explicit that they work like

# exp: T where T: Boolable
if bool(exp):
  ...
else:
  ...