[stdlib] Make StringRef ctor from UnsafePointer[Byte] keyword only

[soraros]

This issues was reported by @aurelian(@diocletiann) on Discord.
This prevents accidental UnsafePointer[Byte] to String implicit conversion through String ctor from StringRef.

@JoeLoser Should we make the constructor from pointer+length keyword only as well, following String?

[gryznar]

I am curious why implicit conversions are present in Mojo at all? It seems that it is source of a lot of bugs

[soraros]

@gryznar Even though I'm not a fan of implicit conversions, they seem to be necessary to model things like literals and mimic Python behaviour in the dynamic portion. However, I do agree that we should be able to control them, and only opt-in to implicit conversion when creating a constructor (#1310).

[gryznar]

This issue should be addressed first IMHO, because switching to keyword-only treats symptoms, not cause

[JoeLoser]

@JoeLoser Should we make the constructor from pointer+length keyword only as well, following String?

Can you elaborate here? For String, it's implicit I think - see this constructor.

[soraros]

@JoeLoser I was always under the impression that it worked this way. I must have mixed it up with another struct.

That said, the unsafe constructors from pointers are really a mess, and I find myself having to look up the keyword names every time. Could we adopt a convention like fn __init__(inout self, *, ptr: UnsafePointer, len: Int)?

• String: fn __init__(inout self, ptr: UnsafePointer[UInt8], len: Int):
• StringRef: fn __init__(inout self, ptr: UnsafePointer[c_char], len: Int):
• StringSlice: fn __init__(inout self, *, unsafe_from_utf8_ptr: UnsafePointer[UInt8], len: Int):
• Span: fn __init__(inout self, *, unsafe_ptr: UnsafePointer[T], len: Int):
• List: fn __init__(inout self, *, unsafe_pointer: UnsafePointer[T], size: Int, capacity: Int):

[JoeLoser]

@JoeLoser I was always under the impression that it worked this way. I must have mixed it up with another struct.

That said, the unsafe constructors from pointers are really a mess, and I find myself having to look up the keyword names every time. Could we adopt a convention like fn __init__(inout self, *, ptr: UnsafePointer, len: Int)?

• String: fn __init__(inout self, ptr: UnsafePointer[UInt8], len: Int):
• StringRef: fn __init__(inout self, ptr: UnsafePointer[c_char], len: Int):
• StringSlice: fn __init__(inout self, *, unsafe_from_utf8_ptr: UnsafePointer[UInt8], len: Int):
• Span: fn __init__(inout self, *, unsafe_ptr: UnsafePointer[T], len: Int):
• List: fn __init__(inout self, *, unsafe_pointer: UnsafePointer[T], size: Int, capacity: Int):

Agreed, they're wildly inconsistent. +1 to your proposed convention - feel free to open a PR. I'm also +1 for making them all (including String kw-only for the ptr + len constructor until we fix implicit constructor issue you linked).

[soraros]

@JoeLoser I think this one is ready. The CI failure is caused by some Conda issues.

[JoeLoser]

!sync

[JoeLoser]

Looks good still, we'll just need to fix up our internal usage before this lands/merges internally. I'm on PTO tomorrow, but can look on Monday likely.

[modularbot]

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

[modularbot]

Landed in 8767f4d! Thank you for your contribution 🎉