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

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

Open
soraros opened this issue Nov 19, 2023 · 23 comments
Open
Labels
bug Something isn't working mojo-repo Tag all issues with this label mojo-stdlib Tag for issues related to standard library

Comments

@soraros
Copy link
Contributor

soraros commented Nov 19, 2023

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
@soraros soraros added bug Something isn't working mojo Issues that are related to mojo labels Nov 19, 2023
@soraros soraros changed the title [BUG]: Mojo allows implicit conversion from Int to String [BUG]: Mojo allows implicit conversion from Int/SIMD/... to String Nov 19, 2023
@guidorice
Copy link

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
Copy link
Contributor Author

soraros commented Nov 19, 2023

@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.

@soraros soraros changed the title [BUG]: Mojo allows implicit conversion from Int/SIMD/... to String [BUG]: Mojo permits the use of any constructor for implicit conversions Nov 19, 2023
@JoeLoser JoeLoser added the mojo-stdlib Tag for issues related to standard library label Nov 19, 2023
@scottamain scottamain added the documentation Improvements or additions to documentation label Nov 20, 2023
@helehex
Copy link
Contributor

helehex commented Nov 20, 2023

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

@helehex
Copy link
Contributor

helehex commented Nov 20, 2023

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
Copy link

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
Copy link
Contributor Author

soraros commented Nov 20, 2023

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
Copy link
Contributor

helehex commented Nov 20, 2023

@nonmaterializable allows an even impliciter conversion

@helehex
Copy link
Contributor

helehex commented Nov 20, 2023

i wonder if we'll ever get the implicitest conversion

@willghatch willghatch added mojo-lang Tag for all issues related to language. and removed mojo-stdlib Tag for issues related to standard library mojo-lang Tag for all issues related to language. labels Nov 21, 2023
@ematejska ematejska added the mojo-lang Tag for all issues related to language. label Nov 21, 2023
@lattner
Copy link
Collaborator

lattner commented Nov 27, 2023

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

@soraros
Copy link
Contributor Author

soraros commented Dec 1, 2023

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
Copy link

guidorice commented Dec 1, 2023

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
Copy link
Contributor Author

soraros commented Dec 1, 2023

@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
Copy link
Collaborator

JoeLoser commented Dec 5, 2023

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
Copy link
Contributor Author

soraros commented Dec 5, 2023

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

@ParadaCarleton
Copy link

ParadaCarleton commented Dec 7, 2023

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
Copy link
Collaborator

@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
Copy link
Collaborator

@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
Copy link
Collaborator

lattner commented Apr 9, 2024

@bethebunny has a proposal

@JoeLoser
Copy link
Collaborator

@bethebunny has a proposal

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

@linear linear bot added mojo-repo Tag all issues with this label test-sync-delete after and removed mojo Issues that are related to mojo mojo-lang Tag for all issues related to language. documentation Improvements or additions to documentation test-sync-delete after labels Apr 29, 2024
@ematejska ematejska added the mojo-stdlib Tag for issues related to standard library label May 7, 2024 — with Linear
@eric-vader
Copy link

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
Copy link
Contributor

helehex commented May 24, 2024

still waiting on the implicitest conversion

@helehex
Copy link
Contributor

helehex commented May 24, 2024

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

@gabrieldemarmiesse
Copy link
Contributor

gabrieldemarmiesse commented Jun 13, 2024

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...)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working mojo-repo Tag all issues with this label mojo-stdlib Tag for issues related to standard library
Projects
None yet
Development

No branches or pull requests