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

fix #20644 cast(ref T) as shorthand for *cast(T*)& #20728

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

WalterBright
Copy link
Member

A simple rewrite.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

  • In preparation for migrating from Bugzilla to GitHub Issues, the issue reference syntax has changed. Please add the word "Bugzilla" to issue references. For example, Fix Bugzilla Issue 12345 or Fix Bugzilla 12345.(Reminder: the edit needs to be done in the Git commit message, not the GitHub pull request.)

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#20728"

@WalterBright
Copy link
Member Author

#20644

@thewilsonator thewilsonator added Review:Needs Changelog A changelog entry needs to be added to /changelog Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org labels Jan 18, 2025
Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

Needs Spec PR and changelog (and probably DIP?) but otherwise looks good

@JohanEngelen
Copy link
Contributor

JohanEngelen commented Jan 18, 2025

Edit: moved my comment to here: #20644 (comment)

@WalterBright
Copy link
Member Author

Spec PR: dlang/dlang.org#4164

@WalterBright WalterBright removed the Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org label Jan 18, 2025
@WalterBright WalterBright removed the Review:Needs Changelog A changelog entry needs to be added to /changelog label Jan 18, 2025
@WalterBright WalterBright force-pushed the castRef branch 2 times, most recently from 07d16f7 to 5e222bf Compare January 18, 2025 20:29
@dkorpel dkorpel linked an issue Jan 20, 2025 that may be closed by this pull request
@0xEAB 0xEAB changed the title fix #20644 cast(ref T) as shorthand for *cast(T*)& fix #20644 cast(ref T) as shorthand for *cast(T*)& Feb 2, 2025
@apz28
Copy link

apz28 commented Feb 8, 2025

Why not using this pattern: cast(*T*)

@TurkeyMan
Copy link
Contributor

LGTM... is this just hanging out?

@schveiguy
Copy link
Member

Just had this thought. What happens here?

void callback(void *arg)
{
    cast(ref int)arg = 5;
}

If this means what I think it means, then it's going to be very confusing. Is there any way to warn against this?

@TurkeyMan
Copy link
Contributor

Just had this thought. What happens here?

void callback(void *arg)
{
    cast(ref int)arg = 5;
}

If this means what I think it means, then it's going to be very confusing. Is there any way to warn against this?

I'm not clear what you think it means?

It looks like you're trying to assign an integer to that pointer.
You probably meant: arg = cast(void*)5, but the way you've written it shows a potential 32/64 bit mismatch. Change int for size_t and it's fine I guess... although the function doesn't show anything it does with that pointer afterwards... or why the function should receive a pointer in the first place if you're just overwriting it with a literal?

Maybe you're actually trying to show a situation where you intend to return something via the supplied pointer? But I don't know how that relates to ref cast? It's just a total mismatch of concepts.
*cast(int*)&arg = 5 doesn't assign through the pointer, it assigns TO the pointer.

So, I guess you actually want this:
*cast(int*)arg =5
You could alternately write:
cast(ref int)*arg = 5, except not in this case because it'svoid*and you obviously can't dereference void.

Anyway, safe to say that cast(ref) isn't really relevant to this situation. If the function received it's argument by ref, then cast(ref) might be a more fitting expression... but I suspect what you're showing is just a plain mismatch of concepts.

@schveiguy
Copy link
Member

I'm not clear what you think it means?

I'm pretty sure it means, set the integer-sized portion of the pointer bits to 5. But I didn't test it.

But many people reading it will think it's writing to whatever the pointer is pointing at.

It looks like you're trying to assign an integer to that pointer. You probably meant: arg = cast(void*)5, but the way you've written it shows a potential 32/64 bit mismatch. Change int for size_t and it's fine I guess... although the function doesn't show anything it does with that pointer afterwards... or why the function should receive a pointer in the first place if you're just overwriting it with a literal?

No. What I'm imagining is that someone writes this, expecting it to be the same as *(cast(int*)p) = 5;, but just a nicer way to do it with ref so you don't need the extra syntax.

The fact that it compiles and does something completely unexpected is violating the principle of least surprise.

Maybe you're actually trying to show a situation where you intend to return something via the supplied pointer? But I don't know how that relates to ref cast? It's just a total mismatch of concepts. *cast(int*)&arg = 5 doesn't assign through the pointer, it assigns TO the pointer.

I'm trying to show a situation where the feature is misunderstood, but looks like it does something it doesn't.

This reminds me of bad delegate syntax (which is no longer allowed):

auto dg = () => {return x + y;}

This was generally expected to be a delegate that returns x + y, but instead it's a delegate that returns a delegate that returns x + y.

Now, the compiler complains, because this syntax is too easy to get wrong:

onlineapp.d(6): Deprecation: using `(args) => { ... }` to create a delegate that returns a delegate is error-prone.
onlineapp.d(6):        Use `(args) { ... }` for a multi-statement function literal or use `(args) => () { }` if you intended for the lambda to return a delegate.

I'm anticipating a similar problem with this syntax. Perhaps there is a similar way to disallow bad usage.

So, I guess you actually want this: *cast(int*)arg =5 You could alternately write: cast(ref int)*arg = 5, except not in this case because it'svoid*and you obviously can't dereference void.

That is what the fictitious person who I imagine would write this was thinking.

Anyway, safe to say that cast(ref) isn't really relevant to this situation. If the function received it's argument by ref, then cast(ref) might be a more fitting expression... but I suspect what you're showing is just a plain mismatch of concepts.

It's very relevant, because it's a new syntax that has the potential to cause confusion. Indeed I am showing how someone might misunderstand how it works.

Note, I'm in favor of the cast(ref) change, I just am anticipating ways it can be confusing.

@TurkeyMan
Copy link
Contributor

TurkeyMan commented Mar 2, 2025

I would imagine the same user who is likely to experience that confusion to notice a mismatch between the function argument that appears to be *, and their expression which is ref, and I reckon they'd notice the mismatch... if not immediately, they'll crash and certainly look for their bug and become suspicious.
This isn't the sort of mistake that will lead to subtle change in behaviour that will go unnoticed; it's basically 100% guaranteed crash bug if you got this wrong.

I still maintain the suspicion that such a user who becomes confused in the way you suggest is likely to be a whole lot more confused when they encounter *cast(Thing*)&wow... put the 2 expressions beside each other and ask an unfamiliar user what they reckon each one does.

D has been moving towards ref for like a decade now; with escape analysis and @safe stuff in general; raw pointer arguments are less and less common in favour of ref. A ref cast on a ref parameter is much more reasonable and accessible. People aren't likely to mismatch ref and pointers, they're likely to choose the thing that matches the other thing that's already there.

@TurkeyMan
Copy link
Contributor

@WalterBright This seems to fail the tests... needs a rebase?

@schveiguy
Copy link
Member

they'll crash and certainly look for their bug and become suspicious.

The code above doesn't crash. It just does nothing.

I've spent hours on code that seems like it should do what I want but doesn't. It's not fun.

@schveiguy
Copy link
Member

People aren't likely to mismatch ref and pointers, they're likely to choose the thing that matches the other thing that's already there.

There is a problem though. This cast will always work (assuming you have an lvalue), whereas a pointer cast does not always work.

i.e.

struct Foo {}
Foo foo;
auto noWorky = cast(void*)foo; // Error: cannot cast expression `foo` of type `Foo` to `void*`

Code compiling but doing something completely unlike what you expect is bad. Having it happen and be coerced into the type you expect is worse.

Could we start with a requirement that the original type must not be a pointer? When people have a pointer, they are not thinking about reinterpreting the bits of the pointer most of the time. Yes there are cases where you would want to write or bits of the pointer, but there is always the original syntax, and let's face it -- if you are writing pointer bit patterns you should know what you are doing via reinterpreting pointers using the old casting syntax.

This seems like a great feature for reinterpreting bits of types, and I would like to see it added.

@TurkeyMan
Copy link
Contributor

My imagine just doesn't light up at all with respect to the concern you raise. I can kinda see your argument to prevent it applied to pointers and I'm sympathetic to a small extent, but a much higher weight in my soul hates unfounded non-uniformity like that, so I wouldn't do it if it were my thing. It's raising the complexity of the implementation (which is like 5 lines!); adding that sort of logic is probably way more implementation than the thing itself... I don't love that.

I still think fear surrounding this is basically hypothetical and no more concerning than casting anything in D in the normal case. Especially compared to *cast(T*)&val which is one of those things that any young programmer is well documented to freak out about; this is making a horrible and confusing thing better and more reasonable in a very real sense. Misuse of anything is possible; and D's cast is already the wild west! My feeling is that if we want to start putting guard rails on any cast, then we should overhaul cast completely and take the whole problem space into consideration wholistically.
C++ did an overhaul forever-ago to add a suite of casts and a whole bunch of guard rails, and that's been well received. I see no point in adding complexity unless we're going to make a wholistic attack at the problem the same way C++ did.
D's casts are brutal and raw, that's just how it is... this doesn't really add or subtract from that.

@schveiguy
Copy link
Member

The concern I have is driven by the implicit address taking of a local. This is an awkward and unnatural thing to realize, and not at all obvious from the proposed syntax. cast(ref T)x does not look like you are taking the address of x, even though you are. When the thing is a pointer, it's even less obvious.

I also sympathize with your point of view, and I understand the reluctance to add "special cases" here. My main concern is that there are legitimate but very few cases where you need to write the bits of a pointer via reinterpret cast. So if this feature is added, naturally, people will write code that uses it for that purpose. Later on, when it is discovered (my prediction) that this is so error prone it's not worth having, then removing that allowance will break legitimate code.

If, on the other hand, we disallow this kind of code at the beginning, then no valid code will use the feature to write bits to the pointer. If we then at some point decide it should be allowed and is probably not harmful, then allowing the usage does not break anything.

In other words, it's much easier to let the toothpaste out of the tube than it is to put it back in.

@TurkeyMan
Copy link
Contributor

TurkeyMan commented Mar 3, 2025

[...] then removing that allowance will break legitimate code.

If, on the other hand, we disallow this kind of code at the beginning, then no valid code will use the feature to write bits to the pointer. If we then at some point decide it should be allowed and is probably not harmful, then allowing the usage does not break anything.

In other words, it's much easier to let the toothpaste out of the tube than it is to put it back in.

Frankly, I'm bored of this talking point. It's my biggest frustration with the D community, and it's pretty much the single reason that I am always repelled from contributing time and energy as a core developer. It's almost impossible to make D better or fix things!

Particularly in cases like this hypothetical, where the correction in user code is clear and mechanical, and doesn't have any side effects or broader implications.

The language should be allowed to make breaking changes; common-sense and case-by-case determination should be employed to make sure such decisions are sane.

Release notes 2027:
* The cast(ref) expression rules were tightened; update your code to say `**&...`

It's frustrating that we refuse to fix mistakes that require users to update a self-contained line here and there.

I hear what you're saying, but I still think this concern is hypothetical. I think it's sad that there is a culture that promotes over-engineering because people are terrified to tighten a rule later...
I'm certainly not saying it's always inappropriate; experience, taste, and common-sense have a lot to answer for, but this talking point is so prevalent that it feels to me more like a reflexive instinct, and I just see it as over-engineering.

Obviously, it's easy to see how that develops in a culture where we refuse to ever fix anything... so the problem here would seem to me that we refuse to ever fix anything!

But like, yeah... I really don't mind either way. I wouldn't do it, but if you want to add that check you suggest, then I don't think anyone is likely to complain.
That said though, I doubt Walter is going to update this PR to add that guard; I notice that he tends value simplicity. My prediction is that this will sit here and die while nobody ever addresses that concern.

@schveiguy
Copy link
Member

I don't have any skills to change it, or set this as my hill to die on, so do whatever you like. If Walter wants to take that risk, it's up to him. Don't consider this point as a death knell or proof that this feature is invalid. But surely, we can discuss hypotheticals without getting all bent out of shape?

Some of the most painful and stupid changes we've done in this language are do to avoiding code breakage. So it's a real concern to think about when adding or changing features.

@TurkeyMan
Copy link
Contributor

TurkeyMan commented Mar 3, 2025

I wouldn't say I'm bent out of shape, it was more of a meta-commentary on a broad issue that influences decision making on a detail like this.

@WalterBright are you persuaded to implement the change Steve reckons?

@schveiguy
Copy link
Member

Could we start with a requirement that the original type must not be a pointer? When people have a pointer, they are not thinking about reinterpreting the bits of the pointer most of the time. Yes there are cases where you would want to write or bits of the pointer, but there is always the original syntax, and let's face it -- if you are writing pointer bit patterns you should know what you are doing via reinterpreting pointers using the old casting syntax.

@WalterBright this is the requested change.

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

Successfully merging this pull request may close these issues.

cast(ref T)... as shorthand for *cast(T*)&...
8 participants