Fix issue 19256: prevent JSONValue const conversion

[FeepingCreature]

trafficstars

Fix https://issues.dlang.org/show_bug.cgi?id=19256 Reorder JSONValue's union members to make dmd realize that JSONValue contains const references and disallows reassigning const(JSONValue) to JSONValue, which would break constness. This works around https://issues.dlang.org/show_bug.cgi?id=12885 .

Sample code illustrating the problem:

    const JSONValue innerObj = JSONValue(["foo": JSONValue(1)]);

    assert(innerObj["foo"] == JSONValue(1));

    // Why can I do this??
    JSONValue value = innerObj;

    value["foo"] = JSONValue(2);

    assert(innerObj["foo"] == JSONValue(1)); // fails

[dlang-bot]

Thanks for your pull request and interest in making D better, @FeepingCreature! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
• 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
✓	19256	normal	std.json: JSONValue allows violating constness

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + phobos#6716"

[CyberShadow]

Test, please? You can use static assert(!__traits(compiles, ...)) to test for accepts-invalid bugs.

[FeepingCreature]

Test added.

[FeepingCreature]

Regarding auto-tester failures, see https://github.com/dlang/druntime/pull/2304

[FeepingCreature]

druntime PR merged, retrying.

[ghost]

About buildkite i don't know if you have seen but this PR breaks a non trivial amount of projects (vibe, dls, dub, ...). Ask for an invite to wilzbach or Martin Nowak or andralex in order to see the logs.

[FeepingCreature]

I am not sure that things can be done. D allows you to write straight-up wrong code involving JSONValue right now. Presumably that's what they're doing.

Occasionally fixing a bug implies breaking code that used to work, if it worked in a way that defied the language spec.

[ghost]

Contacting people so that they change their code is what i had in mind.

[CyberShadow]

The problem could be in just 1 place in the code, with so many projects failing because said code is a dependency.

[ghost]

Almost. After looking more carefully at the logs it's always vibe-d (but several places) excepted for the project dls.

[CyberShadow]

Perhaps leaving this to a compiler fix would be preferred after all, as then it could go through a deprecation phase.

@WalterBright How likely are we to get a fix for https://issues.dlang.org/show_bug.cgi?id=12885 in DMD?

[FeepingCreature]

Ping? "Not very likely"?

[MartinNowak]

@WalterBright How likely are we to get a fix for https://issues.dlang.org/show_bug.cgi?id=12885 in DMD?

I'd say we should fix that issue (https://issues.dlang.org/show_bug.cgi?id=12885#c3), though it will take some deprecation period. So this looks like a reasonable workaround in the meantime.

[MartinNowak]

Build failure of vibe.d (https://buildkite.com/dlang/phobos/builds/397#dccb2203-9142-4358-9840-e650356a4ccb/99-140).

Error

vibe-core-1.4.3/vibe-core/source/vibe/core/args.d(68,27): Error: function `vibe.core.args.fromValue!bool.fromValue(JSONValue val)` is not callable using argument types `(const(JSONValue))`
vibe-core-1.4.3/vibe-core/source/vibe/core/args.d(68,27):        cannot pass argument `*pv` of type `const(JSONValue)` to parameter `JSONValue val`
vibe-core-1.4.3/vibe-core/source/vibe/core/args.d(298,12): Error: template instance `vibe.core.args.readOption!bool` error instantiating
vibe-core-1.4.3/vibe-core/source/vibe/core/core.d(692,70): Deprecation: `std.algorithm.setops.No` is not visible from module `core`
vibe-core-1.4.3/vibe-core/source/vibe/core/args.d(68,27): Error: function `vibe.core.args.fromValue!string.fromValue(JSONValue val)` is not callable using argument types `(const(JSONValue))`
vibe-core-1.4.3/vibe-core/source/vibe/core/args.d(68,27):        cannot pass argument `*pv` of type `const(JSONValue)` to parameter `JSONValue val`
vibe-core-1.4.3/vibe-core/source/vibe/core/core.d(1328,13): Error: template instance `vibe.core.args.readOption!string` error instantiating

[MartinNowak]

Here is a trivial PR to vibe.d https://github.com/vibe-d/vibe-core/pull/100, seems like a rare occurrence of sth. that breaks with this workaround, so it might still be reasonable to go ahead with this once a new vibe.d version is released.

[MartinNowak]

Should work now with the fixed vibe-d and good to go for 2.085.0 from my side. Seemed to risky to put in 2.084.0 without beta.

[FeepingCreature]

ping

[thewilsonator]

Ack, waiting on https://github.com/dlang/phobos/pull/7011 for green tests.

[thewilsonator]

Can you please rebase this so the circle error goes away?

[thewilsonator]

Hmm vibe.d still fails.

[schveiguy]

I think this PR should be closed. The only practical way forward is to deprecate and remove the behavior, and since the behavior is 100% compiler related (you can't hook an implicit cast away from const), it can't be done in a library.

Note, I found that SumType suffers from the same problem. And it happens in @safe code too.

https://forum.dlang.org/post/[email protected]

[FeepingCreature]

~~I don't follow why. Should D take the const-convertible-ness of a union from the first field? I guess not, but it works today, and then JSONValue behaves correctly.~~

oh wait I see, you mean get rid of that logic. Yeah, I guess. I'd rather see it codified as "well this sucks, but it's the best we can do."

SumType can "fix" this by just reordering its union by hasIndirections. It's not a good solution but it is a solution.

edit: I think fixing 12885 is viable. For the record, I said in 2019 that I need this behavior, but this is no longer the case; librebindable uses a totally different approach (involving a lot more casting).

[schveiguy]

I'd rather see it codified as "well this sucks, but it's the best we can do."

It's not memory safe in @safe code. If D is no longer serious about memory safety, then we can leave it. I guess another option is to disable using such types in @safe code.

SumType can "fix" this by just reordering its union by hasIndirections. It's not a good solution but it is a solution.

SumType will suffer exactly the same fate as JSONValue the more it is used. Eventually there will be critical projects that will break with your suggested change because they accidentally implicitly cast away const, and it will become untouchable.

And there is no way to deprecate it, unless we want to rename the type.

The only path forward is to deprecate the current memory-unsafe behavior, and then fix it.

librebindable uses a totally different approach (involving a lot more casting).

Rebindable in general is a horrendous hack, making up for a lack of tail-modifiers in the language. I'm not convinced it's sound type-wise.

[FeepingCreature]

I mean, if the bug is fixed, JSONValue and SumType will break regardless, surely?

Rebindable in general is a horrendous hack, making up for a lack of tail-modifiers in the language. I'm not convinced it's sound type-wise.

I believe that librebindable is typesafe, or rather, can be used to build typesafe abstractions if used correctly. Phobos Rebindable definitely wouldn't be typesafe, especially if it worked for structs, because it exposes its value by ref, leading to pointer leaks that allow observing immutable changes. My primary argument for librebindable being sound is that it does not do this.

[schveiguy]

I mean, if the bug is fixed, JSONValue and SumType will break regardless, surely?

Yes, but the big difference is that the compiler can deprecate the behavior first, notifying anyone using it for a (probably long-ish) period of time. A library cannot hook the implicit cast.

[schveiguy]

I believe that librebindable is typesafe, or rather, can be used to build typesafe abstractions if used correctly.

I honestly have not read much into it. The idea of deconstifying copied data is sound (i.e. deconstifying the head), but what I'm more worried about is if it's sound within the current model. That is, will amazingly obtuse optimizations suddenly happen because they are sound within the model, but not with the hacks performed here?

[FeepingCreature]

Yes, but the big difference is that the compiler can deprecate the behavior first, notifying anyone using it for a (probably long-ish) period of time. A library cannot hook the implicit cast.

Ah, now I get what you're saying. Yeah if there's hope of the bug being fixed, I agree that's better than this PR.