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

[stdlib] Fix allow immutable UnsafePointer and adjust .unsafe_ptr() methods #3734

Draft
wants to merge 12 commits into
base: nightly
Choose a base branch
from

Conversation

martinvuyk
Copy link
Contributor

@martinvuyk martinvuyk commented Nov 3, 2024

Fix allow immutable UnsafePointer and adjust .unsafe_ptr() methods. Fixes MSTDL-956

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
@martinvuyk
Copy link
Contributor Author

@JoeLoser All the tests are passing but the packaging of test_utils is giving problems, the linker keeps grabbing nightly stdlib even though the script sets MODULAR_MOJO_NIGHTLY_IMPORT_PATH=$BUILD_DIR and I've also tried adding mojo package "${TEST_UTILS_PATH}" -I ${BUILD_DIR}.

Copy link
Collaborator

@JoeLoser JoeLoser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so exciting to see coming together! I appreciate you tackling this! 🎉

@@ -290,6 +290,12 @@ struct _lit_mut_cast[
]


struct _is_mutable_origin[
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question Can you help me understand why this is needed? You should be able to directly access the is_mutable from the call sites without any issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not on type signatures where all you have is __origin_of() and you want to extract the mutability of that:

    @staticmethod
    @always_inline("nodebug")
    fn address_of(
        ref [_, address_space._value.value]arg: type
    ) -> UnsafePointer[
        type,
        address_space,
        is_mutable = _is_mutable_origin[__origin_of(arg)].result,
        origin = __origin_of(arg),
    ] as result:
        """Gets the address of the argument.

        Args:
            arg: The value to get the address of.

        Returns:
            An UnsafePointer which contains the address of the argument.
        """
        return __type_of(result)(
            __mlir_op.`lit.ref.to_pointer`(__get_mvalue_as_litref(arg))
        )

Though if there is a cleaner way to extract an attribute from __origin_of() I'm all ears

stdlib/src/builtin/string_literal.mojo Show resolved Hide resolved
stdlib/src/builtin/string_literal.mojo Show resolved Hide resolved
stdlib/src/memory/unsafe_pointer.mojo Outdated Show resolved Hide resolved
stdlib/test/collections/test_string.mojo Show resolved Hide resolved
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
@@ -624,6 +628,7 @@ struct UnsafePointer[
offset: The offset to store to.
val: The value to store.
"""
constrained[is_mutable, _must_be_mut_err]()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we turn these into conditional conformance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically the compiler won't even let you mutate the origin. I added them for convenience so that users get a clear error message. Conditional Conformance doesn't give you that, only a cryptic error that some parameters can't be inferred.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants