Skip to content

Annotate std/json.d to please dlang/dmd#12520#8088

Merged
RazvanN7 merged 1 commit intodlang:masterfrom
nordlow:fix-pure-scope-json
May 19, 2021
Merged

Annotate std/json.d to please dlang/dmd#12520#8088
RazvanN7 merged 1 commit intodlang:masterfrom
nordlow:fix-pure-scope-json

Conversation

@nordlow
Copy link
Contributor

@nordlow nordlow commented May 16, 2021

Part of #8076

@nordlow nordlow requested a review from CyberShadow as a code owner May 16, 2021 22:24
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @nordlow! 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

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.

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 + phobos#8088"

std/json.d Outdated
}

private void assign(T)(T arg)
private void assign(T)(scope T arg)
Copy link
Member

Choose a reason for hiding this comment

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

How can it be scope given it is stored in the object ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I'll check.

Copy link
Contributor

Choose a reason for hiding this comment

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

It can't, but the assignment happens in a @trusted lambda so the compiler doesn't complain.

Copy link
Contributor Author

@nordlow nordlow May 17, 2021

Choose a reason for hiding this comment

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

Deleted my two previous comments. Afaict, we have a catch 22 here; qualifying v as return in

@property string str(return string v) pure nothrow @nogc @safe
{
    assign(v);
    return v;
}

and reverting to non-scope arg in

private void assign(T)(T arg)

fails to be build on master (uses -dip1000) as

std/json.d(164): Error: scope variable `v` assigned to non-scope parameter `arg` calling std.json.JSONValue.assign!string.assign

because incorrect inference of scope on v because str is qualified as pure.

This passes with dlang/dmd#12520, though.

One way around this is to create two definitions of str; one that works with correct (non-)scope inference of parameters of pure functions (dlang/dmd#12520) and one that compiles with the incorrect (master).

A simpler approach I vote for is to temporarily qualify str, object and array as @trusted and revert these back to @safe as soon dlang/dmd#12520 has been merged.

What do you think, @Geod24, @thewilsonator, @dkorpel ?

Copy link
Contributor Author

@nordlow nordlow May 17, 2021

Choose a reason for hiding this comment

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

I just pushed a forced update that uses @trusted along with a TODO for now for the cases described above. This std/json.d builds locally with both master and dlang/dmd#12520.

@nordlow nordlow force-pushed the fix-pure-scope-json branch from 4aa1ea3 to 5f2e8fc Compare May 17, 2021 21:13
Copy link
Collaborator

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

I agree that this is a better alternative, however, we must not forget about it.

@RazvanN7 RazvanN7 merged commit 5131694 into dlang:master May 19, 2021
@nordlow
Copy link
Contributor Author

nordlow commented May 19, 2021

I agree that this is a better alternative, however, we must not forget about it.

Thank you!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants