[Feature Request] Add unsafe_ptr and unsafe_cstr_ptr to StringRef and migrate the stdlib to use them

[soraros]

Review Mojo's priorities

• I have read the roadmap and priorities and I believe this request falls within the priorities.

What is your request?

As title. In the process, maybe also rename field data to _data.

What is your motivation for this change?

They are needed in e.g. Python interop.

Any other details?

N/A

[martinvuyk]

AFAIK we are migrating away from StringRef in favor of StringSlice which has a lifetime, so the goal is to actually replace ._strref_dangerous() usage with .as_string_slice() by migrating the methods.

[soraros]

@martinvuyk Indeed, we should migrate most things to StringSlice.

However, I suspected StringRef won't go away in the short term for it's an interface type to C++/CPython. For instance, the type passed to get_function isn't exactly right since PyImport_AddModule takes char* (UnsafePointer[Int8]) but str_ref.data has type UnsafePointer[UInt8].

mojo/stdlib/src/python/_cpython.mojo

Lines 847 to 851 in f4bc170

	fn PyImport_AddModule(inout self, name: StringRef) -> PyObjectPtr:
	var value = self.lib.get_function[
	fn (UnsafePointer[UInt8]) -> UnsafePointer[Int8]
	]("PyImport_AddModule")(name.data)
	return PyObjectPtr {value: value}

[martinvuyk]

Couldn't we just add a .unsafe_cstr_ptr() to StringSlice then ? It is literally just return self.unsafe_ptr().bitcast[Int8]()

I suspected StringRef won't go away in the short term

FWIW I'm quite aggressively trying to do just that in:

• .strip already does not depend on StringRef though the implementation is still there since the PR where I added _isspace()
• preparing for also moving String.split() (and deleting the unused just merged StringRef.split()) to StringSlice with PR [stdlib] Make StringSlice CollectionElement #3597

and the latest and most impactful

• PR [stdlib] Move StringRef find() implementation to Span #3548 moves .find() to Span, replaces uses of StringRef for StringSlice for .count, .startswith, .endswith which are literally all of the functions left for StringRef except the helpers for slicing. The biggest dependence to StringRef is really the .find() function.

[soraros]

Couldn't we just add a .unsafe_cstr_ptr() to StringSlice then ?

Yeah, we should probably do that as well.

I suspected StringRef won't go away in the short term

This doesn't even have much to do with the available API. Moving away from StringRef also means we need to figure out the correct lifetime for each of the external_calls, which isn't as mechanical as in user-facing code.

In any case, I think migrating to StringSlice while making QoL improvements to StringRef are not mutually exclusive.

[martinvuyk]

Moving away from StringRef also means we need to figure out the correct lifetime for each of the external_calls, which isn't as mechanical as in user-facing code.

If you mean interfacing with C then my answer is that you'd simply use UnsafePointer directly. Your above example would become: (or, pass a String and have null terminator guarantees since this debug_assert still leaves the door open for memory vulnerabilities.)

fn PyImport_AddModule(inout self, name: StringSlice) -> PyObjectPtr:
    var ptr = name.unsafe_cstr_ptr()
    debug_assert(ptr[name.byte_length()] == 0, "slice must be null terminated")
    var value = self.lib.get_function("PyImport_AddModule")(ptr) 
    return PyObjectPtr {value: value} 

and we still benefit from borrowed argument lifetime guarantees (what you taught me :D)

[soraros]

It all technically can™ be done. It's just the said function is an internal function that I don't care to fix all the call-site right now. One problem though is that fn f(s: StringSlice = "lit") doesn't work.

If you are interested, maybe you can look into having the methods on Python to take Strings instead of StringRef (I don't know if it's the right design though).

[martinvuyk]

One problem though is that fn f(s: StringSlice = "lit") doesn't work.

StringSlice has a constructor for StringLiteral, I have a test here where I'm doing that. Or is it an issue specifically about it not working for function signatures?

If you are interested, maybe you can look into having the methods on Python to take Strings instead of StringRef (I don't know if it's the right design though).

I'm totally up for changing it to StringSlice/String, but I'm already overloading the team with my 8+ open 100+ line PRs 🤣

[martinvuyk]

I've been working on some ffi stuff in PR #3632 and I've introduced a helper function that I think makes more sense than adding public APIs to types that are used only for low level things (I'd actually like to deprecate any use of .unsafe_cstr_ptr() in a follow up PR) :

trait _UnsafePtrU8:
    fn unsafe_ptr(self) -> UnsafePointer[UInt8]:
        ...


@always_inline
fn char_ptr[T: _UnsafePtrU8](item: T) -> UnsafePointer[c_char]:
    """Get the C.char pointer.

    Parameters:
        T: The type.

    Args:
        item: The item.

    Returns:
        The pointer.
    """
    return item.unsafe_ptr().bitcast[c_char]()


@always_inline
fn char_ptr[T: AnyType](ptr: UnsafePointer[T]) -> UnsafePointer[c_char]:
    """Get the C.char pointer.

    Parameters:
        T: The type.

    Args:
        ptr: The pointer.

    Returns:
        The pointer.
    """
    return ptr.bitcast[c_char]()

[soraros]

@martinvuyk I like this approach. Could you make the change into its own PR (and maybe rename to c_char_ptr since we can afford to have ugly names for these things).

[martinvuyk]

Could you make the change into its own PR (and maybe rename to c_char_ptr since we can afford to have ugly names for these things).

Yeah I think you're right this will be better to separate to another PR, also on the name since this is in ffi I just copy pasted from my WIP c ffi package which I'd like to bring over to the stlib once we have the top level ffi package