Fix Bugzilla 24434 - Casting away const with cast() is not a @safe lv… #16315

ntrel · 2024-03-16T12:36:40Z

…alue

dlang-bot · 2024-03-16T12:36:46Z

Thanks for your pull request and interest in making D better, @ntrel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	23530	critical	casting immutable away allowed in safe
✓	24434	normal	Casting away const with cast() should not produce an lvalue

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

Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

To target stable perform these two steps:

Rebase your branch to upstream/stable:

git rebase --onto upstream/stable upstream/master

Change the base branch of your PR to stable

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#16315"

compiler/src/dmd/expressionsem.d

ntrel · 2024-03-16T15:52:14Z

src/core/internal/array/duplication.d(38):        cast from `ArrElem` to `immutable(ArrElem)` cannot be used as an lvalue in @safe code
src/object.d(3871):        `object.idup!(ArrElem).idup` is declared here

That is triggered in _dupCtfe:

        U[] res;
        foreach (ref e; a)
            res ~= e; // this line

So I'm not sure how to fix that. Also the error should maybe be a deprecation.

dkorpel · 2024-03-16T16:45:31Z

Also the error should maybe be a deprecation.

Yes, before we have editions, you can use setUnsafePreview(FeatureState.default_, ... for that

@safe

See dlang/dmd#16315 With that pull, using a type qualifier cast is not @safe to use as an lvalue.

@safe

See dlang/dmd#16315 With that pull, using a type qualifier cast is not @safe to use as an lvalue.

thewilsonator · 2024-03-17T13:30:13Z

../buildkite-ci-build/.dub/packages/libdparse/0.19.4/libdparse/src/dparse/lexer.d(158,23): Deprecation: cast from `const(string)` to `string` cannot be used as an lvalue in @safe code
../buildkite-ci-build/.dub/packages/libdparse/0.19.4/libdparse/src/dparse/parser.d(188,42): Error: template instance `dparse.parser.Parser.attachCommentFromSemicolon!(AliasDeclaration)` error instantiating
../dmd/generated/linux/release/64/dmd -conf= -I../dmd/druntime/import  -w -de -preview=dip1000 -preview=dtorfields -preview=fieldwise -m64 -fPIC -O -release -unittest -version=StdUnittest -c -ofgenerated/linux/release/64/unittest/std/format/package.o std/format/package.d
../buildkite-ci-build/.dub/packages/libdparse/0.19.4/libdparse/src/dparse/lexer.d(150,23): Deprecation: cast from `const(string)` to `string` cannot be used as an lvalue in @safe code
Error /buildkite/builds/geod24-main-1/dlang/dmd/dmd/generated/linux/release/64/dmd failed with exit code 1.

Looks like libdparse needs to be updated? (or not compiled with -de)

RazvanN7 · 2024-03-18T16:20:07Z

I find it icky that the fix is located in a function that is queried for finding the lvalueness of an expression. There is a function called isSafeCast and I think that's where the fix should be located.

On a different note: casting away a qualifier returns an lvalue irrespective of the fact if we are in safe code or not. I think that we should just consider banning casting away qualifiers in @safe code. That will put as to safety with regards to code such as this:

void main() @safe
{
    const int i = 2;
    cast()i = 3;
}

This currently compiles, and I don't think this patch solves this case (athough, IMO it should).

ntrel · 2024-03-20T11:20:23Z

There is a function called isSafeCast and I think that's where the fix should be located.
...
I think that we should just consider banning casting away qualifiers in @safe code

@RazvanN7 isSafeCast doesn't know about lvalues. So I tried making even lvalue QualCast unsafe:

std/utf.d(3172): Deprecation: qualifier cast from `immutable(char)` to `char` not allowed in @safe code
...
std/math/algebraic.d(979):        which wouldn't be `@safe` because of:
std/math/algebraic.d(979):        qualifier cast from `const(ulong)` to `ulong` not allowed in @safe code

utf.d has lots of typeof(cast() ..., which still trigger the deprecation. I'm guessing there's some way of saying to ignore safety inside typeof, which would fix those.

The algebraic.d one is return cast() cast(T) ... in a private function that returns an rvalue. So things like this which are safe (when T is POD) will be broken if you deprecate/error for rvalues as well.

There is an advantage that this doesn't need the duplication.d change of this pull, which creates a copy instead of ref which is probably wrong.

I don't think this patch solves this case (athough, IMO it should).

It actually does detect that. I'll add it to the test later.

Needed for dlang/dmd#16315.

ntrel · 2024-03-23T16:57:31Z

There is an advantage that this doesn't need the duplication.d change of this pull, which creates a copy instead of ref which is probably wrong.

I undid that change, it is not necessary now I'm doing the check for a qualifier cast correctly.

ntrel · 2024-03-23T17:20:55Z

Now broken - somehow the mod field is 0/1 (for mutable/const in the test) as expected when CastExp is visited for semantic analysis, but then by the time toLvalue is called, the mod field has become 255, and I can't detect the qualifier cast anymore. Maybe the CastExp got replaced with another one, because nowhere modifies mod.

Fixes Bugzilla 23530 - casting immutable away allowed in safe.

ntrel · 2024-03-24T20:19:17Z

As I can't detect a qualifier cast in toLvalue, I decided to also fix the non-qualifier unsafe lvalue cast bug too. I found the CatAssignExp semantic code that calls castTo for a safe append and added a workaround.

ntrel · 2024-03-24T21:35:03Z

buildkite is failing on phobos because libdparse 0.19.4 doesn't have the upstream fix:

../buildkite-ci-build/.dub/packages/libdparse/0.19.4/libdparse/src/dparse/lexer.d(158,23): Deprecation: cast from `const(string)` to `string` cannot be used as an lvalue in @safe code
../buildkite-ci-build/.dub/packages/libdparse/0.19.4/libdparse/src/dparse/parser.d(188,42): Error: template instance `dparse.parser.Parser.attachCommentFromSemicolon!(AliasDeclaration)` error instantiating
../buildkite-ci-build/.dub/packages/libdparse/0.19.4/libdparse/src/dparse/lexer.d(150,23): Deprecation: cast from `const(string)` to `string` cannot be used as an lvalue in @safe code

ntrel · 2024-03-27T10:36:33Z

Ping. So does this mean we have to wait for libdparse to be released with the fix before this can be merged?

RazvanN7 · 2024-03-27T14:28:50Z

Yes, we need to fix libdparse, however, the fix seems trivial.

ntrel · 2024-03-27T16:14:28Z

@RazvanN7 The fix is already merged: dlang-community/libdparse#514

Does the phobos buildkite test require libdparse to be the last released version?

@safe

See dlang/dmd#16315 With that pull, using a type qualifier cast is not @safe to use as an lvalue.

thewilsonator · 2024-03-28T00:27:27Z

Does the phobos buildkite test require libdparse to be the last released version?

Currently it is testing against libdparse 0.19.4. The current release of libdparse is 0.23.2 released July 2023.
We will need to tag a new release and then update the version that we test against.

RazvanN7 · 2024-04-26T07:58:51Z

@ntrel Phobos seems to continue failing. Does this need a rebase?

ntrel · 2024-04-27T14:25:39Z

@RazvanN7 There's been a libdparse v0.24.0 release with the fix but I'm not sure where the file is that specifies 0.19.4 for buildkite - @thewilsonator any idea?

dkorpel · 2024-04-27T14:42:29Z

I think it's dlang/tools#471, and dlang/dlang.org#3815 might also be necessary

ntrel · 2024-04-28T11:56:33Z

buildkite now passing!

Fix Bugzilla 24434 - Casting away const with cast() is not a @safe lv…

2b65bcc

…alue

dlang-bot added the Bug Fix label Mar 16, 2024

ntrel marked this pull request as draft March 16, 2024 12:36

ntrel commented Mar 16, 2024

View reviewed changes

compiler/src/dmd/expressionsem.d Outdated Show resolved Hide resolved

Use pointerTo

8a1f901

ntrel added 2 commits March 16, 2024 16:19

Use cast to hide lvalue append error

7b0b63e

Fix unsafe shared increment

d9bcab4

ntrel added a commit to ntrel/phobos that referenced this pull request Mar 16, 2024

[std.socket] Fix cast assign to getaddrinfoPointer

932a4ab

See dlang/dmd#16315 With that pull, using a type qualifier cast is not @safe to use as an lvalue.

ntrel mentioned this pull request Mar 16, 2024

[std.socket] Cast assign to getaddrinfoPointer is not @safe dlang/phobos#8949

Merged

dlang-bot pushed a commit to dlang/phobos that referenced this pull request Mar 16, 2024

[std.socket] Fix cast assign to getaddrinfoPointer

03c3681

See dlang/dmd#16315 With that pull, using a type qualifier cast is not @safe to use as an lvalue.

Use setUnsafePreview

d150a02

Add test for lvalue assign

b5e2b16

ntrel added a commit to ntrel/libdparse that referenced this pull request Mar 20, 2024

Use @trusted qualifier cast

de760f5

Needed for dlang/dmd#16315.

ntrel mentioned this pull request Mar 20, 2024

Use @trusted qualifier cast dlang-community/libdparse#514

Merged

WebFreak001 pushed a commit to dlang-community/libdparse that referenced this pull request Mar 20, 2024

Use @trusted qualifier cast

cc368ca

Needed for dlang/dmd#16315.

Fix qualifier cast check

4e5e9cc

Workaround for safe append

c2eac7c

Fixes Bugzilla 23530 - casting immutable away allowed in safe.

ntrel mentioned this pull request Mar 24, 2024

Add CastExp.toLvalue dip1000 error if implicit pointer conversion would fail #15597

Closed

Update header

db93e18

ntrel marked this pull request as ready for review March 24, 2024 21:35

ntrel requested a review from ibuclaw as a code owner March 24, 2024 21:35

MrcSnm pushed a commit to MrcSnm/phobos that referenced this pull request Mar 27, 2024

[std.socket] Fix cast assign to getaddrinfoPointer

abcec80

See dlang/dmd#16315 With that pull, using a type qualifier cast is not @safe to use as an lvalue.

ntrel mentioned this pull request Apr 14, 2024

New release needed for dmd PR dlang-community/libdparse#519

Closed

dkorpel approved these changes Apr 15, 2024

View reviewed changes

RazvanN7 approved these changes Apr 18, 2024

View reviewed changes

RazvanN7 added the auto-merge-squash label Apr 18, 2024

dkorpel mentioned this pull request Apr 27, 2024

tests_extractor.d: Upgrade libdparse dlang/tools#471

Merged

Trigger tests

6d72091

dlang-bot removed the auto-merge-squash label Apr 28, 2024

thewilsonator added the auto-merge-squash label Apr 28, 2024

thewilsonator merged commit e60bfd1 into dlang:master Apr 28, 2024
46 checks passed

ntrel deleted the unsafe-qual-cast branch November 20, 2024 12:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Bugzilla 24434 - Casting away const with cast() is not a @safe lv… #16315

Fix Bugzilla 24434 - Casting away const with cast() is not a @safe lv… #16315

ntrel commented Mar 16, 2024

dlang-bot commented Mar 16, 2024 •

edited

Loading

ntrel commented Mar 16, 2024

dkorpel commented Mar 16, 2024

thewilsonator commented Mar 17, 2024

RazvanN7 commented Mar 18, 2024

ntrel commented Mar 20, 2024 •

edited

Loading

ntrel commented Mar 23, 2024

ntrel commented Mar 23, 2024 •

edited

Loading

ntrel commented Mar 24, 2024 •

edited

Loading

ntrel commented Mar 24, 2024

ntrel commented Mar 27, 2024

RazvanN7 commented Mar 27, 2024

ntrel commented Mar 27, 2024

thewilsonator commented Mar 28, 2024

RazvanN7 commented Apr 26, 2024

ntrel commented Apr 27, 2024

dkorpel commented Apr 27, 2024

ntrel commented Apr 28, 2024

Fix Bugzilla 24434 - Casting away const with cast() is not a @safe lv… #16315

Fix Bugzilla 24434 - Casting away const with cast() is not a @safe lv… #16315

Conversation

ntrel commented Mar 16, 2024

dlang-bot commented Mar 16, 2024 • edited Loading

Bugzilla references

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

Testing this PR locally

ntrel commented Mar 16, 2024

dkorpel commented Mar 16, 2024

thewilsonator commented Mar 17, 2024

RazvanN7 commented Mar 18, 2024

ntrel commented Mar 20, 2024 • edited Loading

ntrel commented Mar 23, 2024

ntrel commented Mar 23, 2024 • edited Loading

ntrel commented Mar 24, 2024 • edited Loading

ntrel commented Mar 24, 2024

ntrel commented Mar 27, 2024

RazvanN7 commented Mar 27, 2024

ntrel commented Mar 27, 2024

thewilsonator commented Mar 28, 2024

RazvanN7 commented Apr 26, 2024

ntrel commented Apr 27, 2024

dkorpel commented Apr 27, 2024

ntrel commented Apr 28, 2024

dlang-bot commented Mar 16, 2024 •

edited

Loading

ntrel commented Mar 20, 2024 •

edited

Loading

ntrel commented Mar 23, 2024 •

edited

Loading

ntrel commented Mar 24, 2024 •

edited

Loading