modularml / mojo

The Mojo Programming Language
https://docs.modular.com/mojo
Other
22.1k stars 2.54k forks source link

[BUG]: Mojo permits the use of any constructor for implicit conversions #1310

Open soraros opened 7 months ago

soraros commented 7 months ago

Bug description

As title. Maybe it's working as intended, but I consider it a bug, for Mojo is not JavaScript. Is the behaviour there to circumvent the traitless-ness of print?

Steps to reproduce

I think the following code shouldn't type check.

fn main():
  let s: String = 1

System information

Mojo 0.5.0 on Docker, Intel Mac
guidorice commented 7 months ago

I was thinking that it is simply because String has a constructor that takes an Int value- however , now I am not so sure about how implicit conversions really work, and could not find in the programming manual where this is explained. Good question 👍🏽

soraros commented 7 months ago

@guidorice But the constructors are not called explicitly. Hard coding implicit type conversion for these types (counter example do exist, like FloatLiteral to FloatXX) also feels like very bad api design.

Actually, I think the problem is more fundamental, as the following will also work. Too surprising at least for me.

fn f(s: String):
  print(s)

fn main():
  f(1)

Things can get more confusing really easily, if we use this kind of conversion pervasively:

@value
@register_passable("trivial")
struct A:
  ...

@register_passable("trivial")
struct B:
  fn __init__(a: A) -> Self:
    return B {}

@register_passable("trivial")
struct C:
  fn __init__(a: A) -> Self:
    return C {}

fn main():
  fn f(b: B): print("b")
  fn f(c: C): print("c")

  f(A())

The error says error: ambiguous call to 'f', each candidate requires 1 implicit conversion, disambiguate with an explicit cast. I wish we can control which constructor can be used for implicit conversion with, say, a decorator like @implicit.

helehex commented 7 months ago

I like it, but an @explicit decorator would be nice

helehex commented 7 months ago

also, if you want to control this in the mean time, I've been wrapping the argument in a single tuple and it usually works. shouldn't affect performance, although I'm still not sure I trust my benchmarks

ivellapillil commented 7 months ago

Implicit conversions are going to be headscratchers in the future. At least marking a constructor as @implicit would document that such a conversation is taking place.

soraros commented 7 months ago

At least marking a constructor as @implicit would document that such a conversation is taking place.

And I think it will work well with @nonmaterializable too, which also relies on implicit conversion.

helehex commented 7 months ago

@nonmaterializable allows an even impliciter conversion

helehex commented 7 months ago

i wonder if we'll ever get the implicitest conversion

lattner commented 7 months ago

Agree, we need to make implicit conversions be opt-in with an @implicit sort of decorator or something

soraros commented 7 months ago

Agree, we need to make implicit conversions be opt-in with an @implicit sort of decorator or something

And fix the String type. Look at this monstrosity:

fn f(a: Int, b: String):
  ...

fn f(a: Float64, b: Float64):
  ...

f(1, 1)  # calls the first one
guidorice commented 7 months ago

It does seem counter-intuitive, that the numeric types don't win in that overload. But the docs say the precedence is:

  1. Candidates with the minimal number of implicit conversions (in both arguments and parameters).
soraros commented 7 months ago

@guidorice I don't find it counter-intuitive, as the overload resolution rules are clear. I see this as an example of a foot-gun enabled by questionable library design, or more precisely, String's misuse of the implicit conversion mechanism. With traits soon coming, we could We can have a String constructor using the str protocol, that must be called explicitly, and then remove the problematic Int to String constructor. Of cause, the exclusion does depend on the proposed @implicit decorator.

fn __init__[T: Stringable](x: T) -> String:
  return x.__str__()

After a bit more thinking, I actually do find it counter-intuitive. The fn(Int, String) -> None function is called with two integer literals, doesn't IntLiteral to Int conversion count in the # of ICs? Or maybe @nonmaterializable conversions don't count at all?

JoeLoser commented 7 months ago

One option for moving forward in the short-term would be to have an explicit to_string function on the Int type and remove the String constructor taking in an Int.

soraros commented 7 months ago

@JoeLoser Why do we need to_string when we have traits (for explicit conversion)?

ParadaCarleton commented 7 months ago

One option for moving forward in the short-term would be to have an explicit to_string function on the Int type and remove the String constructor taking in an Int.

This seems like it's going along the right lines, but special-casing String would be a pain.

The standard solution for this in strongly-typed languages with automatic promotion is to distinguish construction (takes an argument and creates a new object) from conversion (returns an alternative representation of the same number). For example, convert(String, 4) throws an error (because strings and integers are different things), whereas String(4) will create a new string, "4".

The rule is that convert(Type, x) == x for all x, but Type(x) does not necessarily equal x. e.g.

>>> 4 == "4"
False
>>> 4 == 4.0
True

So convert(String, 4) should error.

arthurevans commented 6 months ago

@scottamain @JoeLoser did tagging this issue documentation remove the mojo-stdlib label? There is probably more we could do in the docs, but I think this is mostly a stdlib issue (for specific conversions) and/or a Mojo issue (adding a way to separate implicit conversions from other constructor types.

scottamain commented 6 months ago

@scottamain @JoeLoser did tagging this issue documentation remove the mojo-stdlib label? There is probably more we could do in the docs, but I think this is mostly a stdlib issue (for specific conversions) and/or a Mojo issue (adding a way to separate implicit conversions from other constructor types.

No, Will removed the stdlib tag, maybe because this is primarily a language behavior in terms of how implicit conversion happens. Also, this issue was reported before we published this documentation about implicit conversion, which describes the current behavior but without acknowledging this issue. I think we should wait until something changes before updating those docs, but maybe we should add a note with a link to this issue.

lattner commented 3 months ago

@bethebunny has a proposal

JoeLoser commented 2 months ago

@bethebunny has a proposal

@bethebunny do you want to push on this some more or want @modularml/mojo-standard-library to run with it?

eric-vader commented 1 month ago

Yea, consider this to be a bug too.. Strange case using vectors; I thought its the type checker but found this thread and realised its the implicit cast.

from tensor import Tensor
fn print_tensor[dtype: DType](x:Tensor[dtype]):
    print(x)

fn main() raises:

    print_tensor[DType.float64](Int(1))
    print_tensor[DType.float64](1)
    print_tensor[DType.float64](Tensor[DType.float64](1))
helehex commented 1 month ago

still waiting on the implicitest conversion

helehex commented 1 month ago

recursively search for the path of least resistance from one type to another, and automatically follow it :]

gabrieldemarmiesse commented 3 weeks ago

I have a more general question about this issue. Is any implicit conversion needed in the language? Would getting rid of it impact the usability of the language if we have automatic materialization (IntLiteral -> Int, StringLiteral -> String, etc...)?