mojo
mojo copied to clipboard
modularml
[BUG]: Mojo permits the use of any constructor for implicit conversions
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
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 👍🏽
@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.
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
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.
At least marking a constructor as
@implicitwould document that such a conversation is taking place.
And I think it will work well with @nonmaterializable too, which also relies on implicit conversion.
Agree, we need to make implicit conversions be opt-in with an @implicit sort of decorator or something
Agree, we need to make implicit conversions be opt-in with an
@implicitsort 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
It does seem counter-intuitive, that the numeric types don't win in that overload. But the docs say the precedence is:
- Candidates with the minimal number of implicit conversions (in both arguments and parameters).
@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?
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.
@JoeLoser Why do we need to_string when we have traits (for explicit conversion)?
One option for moving forward in the short-term would be to have an explicit
to_stringfunction on theInttype and remove theStringconstructor taking in anInt.
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.
@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 @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.
@bethebunny has a proposal
@bethebunny do you want to push on this some more or want @modularml/mojo-standard-library to run with it?
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))
recursively search for the path of least resistance from one type to another, and automatically follow it :]
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...)?
