Issue15702: prohibit implicit conversion to void[] in @safe code if array elements have indirection

[quickfur]

As a usability compromise, still allow explicit casting to void[] in @safe code. Not 100% sure about this. Maybe it's better to completely prohibit all such casts since they usually occur in I/O functions calling C APIs, which almost certainly means overwriting pointers with arbitrary external data.

Note that implicitly casting arrays with no indirections is still allowed, so any code breakage will only be in places where the code is already breaking @safe.

Also, not 100% sure this is the right place in the compiler to put this check, but it seems to be the place that needs minimal code changes.

[dlang-bot]

Fix	Bugzilla	Description
✗	15702	std.socket.Socket.receive breaks @safe

[yebblies]

I would rather "cannot implicitly convert expression (%s) of type %s to %s in @safe code because the type contains indirections".

And converting to const(void)[] should still work. Is there a test case?

[quickfur]

Fixed the error message.

And you're right, I missed the const(void)[] case. Fixed.

[yebblies]

As a usability compromise, still allow explicit casting to void[] in @safe code.

No. A cast doesn't make anything @safeer.

[JakobOvrum]

Indeed, allowing the cast leaves us in no better position. We won't be able to have Socket.receive @trusted if @safe code can cast pointers to void[].

[Geod24]

I would split the acceptance test in compilable.

Also, could you provide additional tests for an array of:

• struct with a static array (should be accepted);
• struct with a pointer member (rejected);
• struct with a dynamic array member (rejected);

[quickfur]

Makes sense. Will do that when I get home later.

[quickfur]

OK, will add code to reject explicit casts too. Gotta run now, will get to this later today.

[yebblies]

Don't forget it's not just casts to void[], but to any mutable array type.

[quickfur]

Updated code to reject all casts (explicit and implicit) from T[] to U[] in @safe code if T != U and U is mutable.

[quickfur]

Ran out of time to work on the test cases... will update tomorrow, maybe.

[yebblies]

You've removed the implicit conversion check. And I'm pretty sure the cast safety checking is done in CastExp.semantic.

[quickfur]

Implicit conversions are still caught because implicitConvTo calls castTo.

But would that still apply if I moved the code to CastExp.semantic?

[quickfur]

Updated code and rebased. The test cases still need some work, but will have to get to that later, gotta run now.

[quickfur]

Gah, looks like it's not as simple as I thought, since nested arrays like int[][] are also considered to have indirections, so even valid conversions like to double[][] are wrongly rejected. Will have to look into this a bit more.

[quickfur]

Reworked the fix. Haven't updated the tests yet, because the current code is rather ugly, particularly in expression.d lines 10743-10746. I couldn't make it work without preempting that call to implicitConvTo, but unfortunately that means duplicating the test for the array -> array case. I don't like this. Any tips to how to refactor this to be nicer?

[quickfur]

Not to mention, isSafeArrayConversion seems to be duplicating a lot of work done elsewhere. What's the best way to fold this into existing code?

[dnadlinger]

Wait, doesn't this fix the wrong end of the equation? Why exactly would it be the T* to void* conversion that is unsafe (or equivalently, T[] to void[])? Using void[] is the unsafe part, and the fact that we have a function in std.socket that's mistakenly marked @trusted doesn't change that (and neither the issue with assignments).

You've asked for a thorough look at @safe on the NG, so it would be a good idea to stay away from quick "fixes" that somehow seem to work around the issue, maybe in some cases, maybe in all, but certainly without having established a principled argument for the change. For example, why go with this instead of just allowing any using/passing around of mutable void[] in @safe code? (This is just for illustration purposes – I'm not suggesting the latter would be the better solution.)

[quickfur]

The original solution was to fix std.socket.Socket.receive to have a memory-safe API, but that didn't work out well because it interferes with method overloading and breaks existing code, plus it wasn't scalable (you have to implement the same workarounds for just about every I/O interface that handles void[]). @JakobOvrum proposed this solution instead.

You're right, though, that we probably need to find a more principled argument for (or against) this solution before committing to anything.