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

Sema: allow @ptrCast of slices changing the length #22706

Merged
merged 1 commit into from
Feb 23, 2025

Conversation

mlugg
Copy link
Member

@mlugg mlugg commented Feb 1, 2025

Also, refactor Sema.ptrCastFull to avoid emitting AIR int_from_ptr on slices. That was actually the original point of this branch :P

--

cc @jacobly0 -- double check that you're okay with the new ptrcast AIR?

@mlugg mlugg added the release notes This PR should be mentioned in the release notes. label Feb 1, 2025
@mlugg mlugg force-pushed the ptrcast-slice branch 4 times, most recently from 10fee1c to 62b4abf Compare February 1, 2025 20:16
MasonRemaley added a commit to Games-by-Mason/ZCS that referenced this pull request Feb 9, 2025
@ifreund
Copy link
Member

ifreund commented Feb 10, 2025

How does this handle sentinel terminated slices? From the code I think this PR matches the current behavior of mem.sliceAsBytes() when, for example, casting a [:0]u32 to a []u8.

However, I think #19984 makes a strong case that it is more useful and less bug-prone to include the reinterpreted memory of the sentinel in the resulting slice.

@mlugg
Copy link
Member Author

mlugg commented Feb 11, 2025

I think it makes sense for the builtin to exclude sentinels. As is, Zig-the-language never implicitly moves a sentinel to a slice's main length; I think that's a reasonable property to maintain.

In fact, the behaviour you propose would be incredibly weird. Consider these cases:

  • [:0]u8 to []u8
  • [:0]u8 to []i8
  • [:0]u32 to []u8

The first one doesn't even require a @ptrCast, but currently this is just equivalent to dropping the sentinel, and I think that's clearly the sane thing in this case. The cast explicitly says "I am not interested in this sentinel".

The second one is a slight variation on the first which can no longer be done via simple coercion. Again, it would be incredibly surprising if it included the sentinel in the result length.

The last one is the kind of thing this PR enables. If it included the sentinel while the other two didn't, the behavior would feel incredibly inconsistent; for instance, @as([]u8, @ptrCast(some_slice)) would include the sentinel of some_slice unless it was a slice of u8. So, I think we would clearly need to go all-or-nothing here, and "all" seems incredibly unintuitive to me.

This doesn't mean sliceAsBytes should necessarily do the same.

@ifreund
Copy link
Member

ifreund commented Feb 11, 2025

I think it makes sense for the builtin to exclude sentinels. As is, Zig-the-language never implicitly moves a sentinel to a slice's main length; I think that's a reasonable property to maintain.

Yes, after further consideration I tend to agree that it makes the most sense to always use foo.len (which is defined to exclude the sentinel) for the calculations involved in this cast.

The reason I ask is that @andrewrk expressed hope that we could delete mem.sliceAsBytes()/mem.bytesAsSlice() entirely after this feature lands, which I also think would be a reasonable thing to do.

I'm not sure it would be a good idea to have the semantics of @ptrCast() and mem.sliceAsBytes() differ subtly here.

Perhaps a better alternative would be to add a mem.absorbSentinel() function which, when passed a [:0]T returns a []T with len incremented by 1. I think this would be quite readable without being overly verbose:

const foo: [:0]u32 = whatever;
const bytes: []const u8 = @ptrCast(mem.absorbSentinel(foo));

@david-vanderson
Copy link
Contributor

I agree with both of you, and dropping mem.sliceAsBytes()/mem.bytesAsSlice() sounds good. @ifreund is right - we need something to include the sentinel if there is one (name maybe mem.includeSentinel()). For dvui this would be a clear improvement for storing/retrieving arbitrary slices.

Based on my experience with #19984 there will be bugs with people forgetting to include mem.includeSentinel(), but I can't think of a better solution.

@rohlem
Copy link
Contributor

rohlem commented Feb 11, 2025

there will be bugs with people forgetting to include mem.includeSentinel(), but I can't think of a better solution.

Fwiw, one potential solution would be to make dropping the sentinel require an explicit slicing operation slice[0..len:] / slice[0..:] / slice[:].
This behavior was brought up as a footgun in #17668, this solution was suggested in #17668 (comment),
and then the idea was rejected and the issue closed (imo without convincing reasoning; I'd still like this change in the language).

Also, refactor `Sema.ptrCastFull` to not be a horrifying hellscape.
@mlugg mlugg enabled auto-merge (rebase) February 22, 2025 20:58
@mlugg mlugg merged commit 5e20e9b into ziglang:master Feb 23, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes This PR should be mentioned in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants