-
-
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
remove the destination type parameter from all cast builtins #5909
Comments
Why are |
## My understanding of the original proposal
### Problem
If you change the type of a value, casts may give unintended results,
causing subtle bugs.
### Proposed solution
Remove the explicit type given to casts, and use the destination type
instead.
## Counter-proposal
I fully agree that in the examples given, changing the value's type will
be a likely source of subtle bugs. This is an issue we should be
solving. However, I do not agree with the proposed solution.
The problem with this proposal, to me, is that it makes some usages less
explicit, and makes it more difficult to explicitly specify the target
type.
```zig
// Before the proposal
// I want to use whatever type foo specifies. If it changes, I want to
// change the type we cast to.
foo(@intcast(@typeinfo(foo).args..., bar));
// I want to use u8. If the arg type to baz changes, I don't care.
baz(@intcast(u8, bar));
// After the proposal
// This is clearly much cleaner.
foo(@intcast(bar));
// This is much clunkier, even if it's not a big deal for simpler cases.
baz(@as(u8, @intcast(bar));
```
The only way to know the type of `@intCast(bar)` in the argument to foo
is to find the definition of `foo`. This is true both before and after
the proposal. The proposal clearly makes it much cleaner to write (and
much easier to read, as there's less weird type lookup to sift
through). The cast to a u8, on the other hand, is clunkier than before,
and unpleasant both to write and to read.
Sobeston brought up on Discord the idea of using an @ResultType()
builtin instead, and I think this is a good idea.
The example would change to the following:
```zig
foo(@intcast(@ResultType(), bar));
baz(@intcast(u8, bar));
```
Now, both cases are both explicit *and* clean. This gets rid of clunky
type lookups for cases where we want to use the destination's type,
without making cases with explicit types more annoying to write. As a
bonus, both cases look nearly identical, and it's easier to read.
|
Maybe we should allow this:
In this case |
I like that idea.
|
Consistency across casting primitives. Note that callsites where this is inconvenient are very uncommon, because of this pattern:
|
One thing i don't like about both @andrewrk and @michal-z's proposals is that it adds functionality especially tailored for these built ins. I believe that the latter proposal is more promising, provided this functionality is extended to be valid for all generic functions. I do think though that it could generate problems with functions where multiple I'm not sure whether i'm a fan of this type if inference, but in my opinion it beats @andrewrk's initial proposal as it is both more orthagonal (and doesn't provide a feature specific to these builtin calls), and it is clearer to the reader that the destination type is inferred. Anyway, as a hypothetical situation, that functionality could again be extended to return types of functions, where the following is valid code: fn add(a: _, b: _) _ {
return a + b;
} This would also allow for example calculation of constants for a certain type: fn pi() _ {
const T = @TypeOf(_); // TODO: Improve
...
return calculated_pi_value;
} As i type this i realize this is more in line with the original proposal though, so the latter paragraph may need some thinking. |
I think the original proposal is a great idea. With this, type coercion is the only thing that changes types, and casts define how that change happens and which things are allowed or UB when performing the conversion. Type coercion tends to occur in only three places: moving values between structs and locals, passing values as parameters, and interacting with There's a fourth case where you want to convert to some intermediate type and then coerce to a known target type, but if the target type is different from the cast type you are performing two coercions and IMO the syntax should make that clear. |
I also think |
Doesn't this fix one error but open up (or increase the likeliness) a whole set of other errors? Eg, What happens above if the data field type is changed from The explicit type parameter is often a double check (at least for me) and makes it more clear what is going on without having to dig through more code. @michal-z 's syntax makes the most sense to me because it doesn't lose the explicit intent of the cast (where |
You have In most cases you should be able to accomplish this by using the form |
@andrewrk This proposal could be made more ergonomic by borrowing C's coercion syntax: |
So maybe I'm being stupid here but it seems like the problem is that implicit upcasting is masking the bug. It feels like maybe we could just remove the implicit upcasting? That way we keep the current cast syntax and we prevent this bug from being masked. It might be a little in a few places but it does let us just punt on the problem. |
It is not yet time to implement #5909.
I sometimes have to turn a
Would this then look like the following in the future?
I'm having a bit of a hard time thinking about what this code will look like post-proposal. |
Yes that's correct, assuming the return type of the function is i32 |
There are a bunch more concepts like |
I understand ... I wrote it that way to make a point. The most important fundamental rule in software development is DRY. Each policy decision that needs to be made should be made in as few places as possible, preferably one ... otherwise you invariably get mismatches as the code evolves.
My claim is that this is never what you want. There are two reasons to limit the size of an int: be be compatible with something else, or for performance--memory or speed. That decision should be made in one place (Zig makes this easy with a type assignment, rather than special alias or typedef keywords). Adding explicit types throughout the code makes it less readable because it's not important or useful information. There's no need to know the specific size of an integer, and every cast gives reason to a code reader to wonder why it's there and why that size in particular is there. There's no need to know that it's a concrete value like u64, rather than whatever abstract type it represents--which should be done through well-chosen names. Zig makes it easy to pick the right sizes--I make liberal use of Anyway, I'm getting too much into philosophy--although it's quite relevant here because language design decisions about how much type inference to do depend on it. On the other matter: there's no semantic difference between inline functions and non-inline functions and there should not be--it's strictly a performance issue. Neither |
@as
is unchanged. These are affected:@floatCast
@intCast
@alignCast
@bitCast
@errSetCast
@ptrCast
@truncate
The motivation here is to prevent bugs. Here's an example case:
Here, we obtain
y
, ausize
but for whatever reason is guaranteed to fit into thebits
field ofData
. So we use@intCast
here to cast it.Now, we rework the code, and actually we need to add some more bits, making it a
u16
:We've now introduced a bug into the code. The
bits
field is updated, but the@intCast
is still casting too small. The lack of type inference here has actually made the code buggier.The original code theoretically should have used
data.bits = @intCast(@TypeOf(data.bits), y)
however this is so cumbersome that it's rarely done in practice, especially whendata.bits
is a more complicated expression.With this proposal, the original code would look like this:
And the updated code (which only changes the type of the
bits
field tou16
) remains correct.With this proposal, it is still possible to get the older semantics:
A cast operation with an inferred result location type would be a compile error:
The text was updated successfully, but these errors were encountered: