-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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] Make trait CollectionElement
inherit from ExplicitlyCopyable
#3153
base: nightly
Are you sure you want to change the base?
[stdlib] Make trait CollectionElement
inherit from ExplicitlyCopyable
#3153
Conversation
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
…th_collectionelementnew
…_with_collectionelementnew
…_with_collectionelementnew
…h_collectionelementnew Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
…th_collectionelementnew
…_list_set_work_with_collectionelementnew
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
CollectionElement
inherit from ExplicitlyCopyable
CollectionElement
inherit from ExplicitlyCopyable
stdlib/src/builtin/format_int.mojo
Outdated
@@ -330,7 +332,7 @@ fn _try_write_int[ | |||
len=1, | |||
) | |||
fmt.write_str(zero) | |||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is an example of the None constructor of Optional not working anymore.
stdlib/src/python/object.mojo
Outdated
# TODO: re-enable when this is possible | ||
# fn __init__(inout self, value: Dict[Self, Self]): | ||
# """Initialize the object from a dictionary of PythonObjects. | ||
# | ||
# Args: | ||
# value: The dictionary value. | ||
# """ | ||
# var cpython = _get_global_python_itf().cpython() | ||
# self.py_object = cpython.PyDict_New() | ||
# for entry in value.items(): | ||
# var result = cpython.PyDict_SetItem( | ||
# self.py_object, entry[].key.py_object, entry[].value.py_object | ||
# ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uncommenting this gives "recursive reference to declaration"
…ementnew Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
@ConnorGray would that help if I rebase this PR or do you have an internal branch for this that you're taking care or? (in which case I can just close this PR then) |
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
…_collectionelementnew Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
@@ -151,10 +151,12 @@ def test_optional_equality(): | |||
assert_true(o == 10) | |||
assert_true(o != 11) | |||
assert_true(o != n) | |||
assert_true(o != None) | |||
# TODO: Find a way to allow "o != None" again. | |||
assert_true(o.__ne__(None)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the meantime we can recommend is None
which works well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will be fixed when we change the compiler to emit NoneType()
when it sees None
. (Currently it emits the #kgen.none
raw MLIR type, which means there's always one extra "hop" of conversion where NoneType
is currently used.)
@ConnorGray you can take a look at what this PR to get an idea of what it takes to make this work. But I'll likely split it further. |
stdlib/src/collections/optional.mojo
Outdated
@@ -193,6 +204,17 @@ struct Optional[T: CollectionElement]( | |||
return False | |||
return not rhs | |||
|
|||
fn __ne__(self, rhs: NoneType) -> Bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion I think opt == None
will work if you change this signature to:
# TODO(MSTDL-715):
# This method should not be necessary, we should need
# only the comparison from a `Optional(None)`.
fn __ne__(self, rhs: NoneType._mlir_type):
(See the similar comment and pattern on Optional.__init__(inout self, value: NoneType._mlir_type)
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried and got the following error:
/projects/open_source/mojo/stdlib/test/collections/test_optional.mojo:148:5: error: cannot bind MLIR type 'None' to trait 'CollectionElement'
def test_optional_equality():
^
/projects/open_source/mojo/stdlib/src/builtin/value.mojo:148:8: note: MLIR type cannot satisfy required trait function here
fn __init__(inout self, *, other: Self):
^
/projects/open_source/mojo/stdlib/test/collections/test_optional.mojo:148:5: error: cannot bind MLIR type 'None' to trait 'CollectionElement'
def test_optional_equality():
^
/projects/open_source/mojo/stdlib/src/builtin/value.mojo:148:8: note: MLIR type cannot satisfy required trait function here
fn __init__(inout self, *, other: Self):
^
Since I get the same error when I didn't do any modifications to Optional
I believe that it's a bug in the overload resolution.
# TODO: Pass directly a UnsafePointer when possible, instead of an Int. | ||
# Otherwise we get "argument #2 cannot be converted from 'UnsafePointer[List[Int], 0, 0]' to 'UnsafePointer[List[Int], 0, 0]'" | ||
# This bug also appears if we pass the list as borrow and grab the address in the constructor, in | ||
# which case we get "argument #2 cannot be converted from 'List[Int]' to 'List[Int]'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question Does this work if you use rebind[UnsafePointer[List[Int]]](ptr)
wherever the pointer is passed? That might be a simpler workaround (if it does work).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand where the rebind should go, but I tried this:
@value
struct ValueDestructorRecorder(CollectionElement):
var value: Int
var destructor_counter: UnsafePointer[List[Int]]
fn __init__(inout self, value: Int, destructor_counter: UnsafePointer[List[Int]]):
self.value = value
self.destructor_counter = rebind[UnsafePointer[List[Int]]](destructor_counter)
fn __init__(inout self, *, other: Self):
self.value = other.value
self.destructor_counter = other.destructor_counter
fn __del__(owned self):
self.destructor_counter[].append(self.value)
And I get the following error:
/projects/open_source/mojo/stdlib/test/memory/test_maybe_uninitialized.mojo:26:32: error: no matching function in initialization
ValueDestructorRecorder(
~~~~~~~~~~~~~~~~~~~~~~~^
/projects/open_source/mojo/stdlib/test/test_utils/types.mojo:120:8: note: candidate not viable: argument #2 cannot be converted from 'UnsafePointer[List[Int], 0, 0]' to 'UnsafePointer[List[Int], 0, 0]'
fn __init__(inout self, value: Int, destructor_counter: UnsafePointer[List[Int]]):
^
/projects/open_source/mojo/stdlib/test/test_utils/types.mojo:124:8: note: candidate not viable: missing 1 required keyword-only argument: 'other'
fn __init__(inout self, *, other: Self):
^
mojo: error: failed to parse the provided Mojo source module
and if I rebind an Int
, I just get
/projects/open_source/mojo/stdlib/src/builtin/rebind.mojo:42:52: error: invalid rebind between two unequal types: index to !kgen.pointer<none>
This pull request is part of #3153 and can be merged before. It's basically adding a few explicit copy constructors where necessary. I didn't add any traits, as `CollectionElement` will very soon require the explicit copy constructor. Co-authored-by: Gabriel de Marmiesse <gabriel.demarmiesse@datadoghq.com> Closes #3217 MODULAR_ORIG_COMMIT_REV_ID: 5593d75e811050ee28b5d981ab93ee6f789cdec2
!sync |
stdlib/src/python/object.mojo
Outdated
# self.py_object = cpython.PyDict_New() | ||
# for entry in value.items(): | ||
# var result = cpython.PyDict_SetItem( | ||
# self.py_object, entry[].key.py_object, entry[].value.py_object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this code be commented without todo / fixme etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't go into nightly, it's there to show what is needed to make the CI happy
stdlib/src/python/python.mojo
Outdated
# Returns: | ||
# The constructed empty Python dictionary. | ||
# """ | ||
# return PythonObject(Dict[PythonObject, PythonObject]()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question, is commenting that intentional?
!sync |
…ementnew Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
@ConnorGray I found a workaround for the I believe the PR is good enough to be merged. |
!sync |
Feel free to ping me when you have time for a review and want me to rebase the PR |
Waiting for the new nightly to be released so I can rebase this PR and shrink the diff
List of issues I have currently:
1 - Passing functions by parameter gives cryptic error message
Here is a minimal reproducer:
The error message is
The error message is not clear at all. What can we do to fix it? This issue prevents me from migrating over
sort.mojo
.2 - The
None
constructor ofOptional
cannot be used anymore. We must useOptional[...]()
orOptional[...](_NoneType)
It's still present in the code, but if one tries to use it, then the compiler says "cannot bind MLIR type None to trait CollectionElement". And the error message is often at the wrong place in the code (compiler bug?)
3 - Using
Dict[Self, Self]
inPythonObject
givesrecursive reference to declaration
So I had to comment out the method. I don't see how to reintroduce it.