[stdlib] Fix allow implicit casting MutableOrigin -> ImmutableOrigin

[martinvuyk]

Fix allow implicit casting MutableOrigin -> ImmutableOrigin by adding implicit constructors to and fro for Span and StringSlice

[nmsmith]

Small note: If we end up modelling origin mutation as an effect that functions perform, rather than today's approach where structs and functions are parameterized by is_mutable parameters, this PR would end up being redundant, because "mutable slices" and "immutable slices" (etc.) would have the same type. (This idea has been in discussion for a while.)

That's not to say this PR shouldn't be accepted. Maybe this is solving some near-term problems that people have been experiencing. I'm out of the loop on this.

[martinvuyk]

Hi @nmsmith, I would love for this PR to become redundant. Parametric origins are a pain. This is mostly to facilitate work in function signatures and at call-site to allow using both types interchangeably on both ends and letting implicit construction do the job of casting

[ConnorGray]

Question: Could you expand on the design for implicit casting of immutable Span to a mutable Span? My gut instinct is that that would violate the compilers ability to reason about exclusivity of mutable origins. E.g. if you had:

fn read[O: ImmutableOrigin](imm_span: Span[Int, O]):
    var mut_span = imm_span.mut(imm_span)
    # ... mutate `mut_span` ...

then when the compiler reasons that read() doesn't mutate imm_span it is incorrect. I think I can see a path to an argument that that's actually fine (Mojo isn't Rust), but I haven't thought deeply about it, and I'm curious what your thoughts are 🙂

[martinvuyk]

I didn't put much thought into it either. Now that you mention it it seems like a possible foot-gun.

The only use case I can think of is when trying to work around the compiler's restriction to having 2 mutable references to the same origin inside a function. An example (for Pointer) would be sorting algorithms's inner functions where they'd need 2 references to be compared and then swapped, and if they pointed to the same memory location the compiler would enforce that they should be immutable refs. Currently the sort module just uses UnsafePointer to avoid such restrictions.

I'm still not sure though whether we should offer this constructor even as an explicit one, it seems like encouraging function signature "contract breaking".

The more I think about it the less I like the idea, I'll revert the changes to leave only Mutable -> Immutable and if we find a solid use case we could always implement the counter direction.

[ConnorGray]

I agree that we could conceivable want an "unsafe" way to cast an immutable origin to a mutable origin (we have that already in a less ergonomic form in _lit_mut_cast), but I agree its a special case, and we could leave it out of this PR for the time being.

Thanks for removing those for the time being Martin — I'll give this another look now! 🙂

[ConnorGray]

I know we've used the convention of having a value field in other structs that wrap MLIR values, but IMO I think we'd be better moving away from that. (value isn't very readable at a glance since its such a generic term.)

My preference is that we leave this field name as _mlir_origin for the time being. I'm open to reconsidering that though, perhaps as part of a holistic effort to revist all the MLIR-wrapping types in the stdlib to be more consistent about this in general.

[martinvuyk]

I can revert the change for this PR np. It was part of my attempt to do #3833, I don't have any string opinion on the name of the field itself either.

[ConnorGray]

Question Is there a way we might avoid the duplication here? I can see the potential value in having aliases for mutable/immutable variants of the current Span/StringSlice type, but in a case like this, a "normal" method call seems more readable at a glance.

[martinvuyk]

True, I removed the function mostly because I disliked the name. We could also change the method name to something like .to_immut() or directly .immut() (or leave that for another PR) WDYT?