Replace `in` with `const scope` for object.d rt/dmain2.d and rt/typeinfo #2685

JinShil · 2019-07-17T10:45:43Z

About This PR

Followup to
#2676
dlang/phobos#7110
#2680
#2684

This one of several PRs I intend to submit, breaking up #2677 into multiple PRs.

This PR only addresses object.d, rt/dmain2.d, and files in the rt/typeinfo folder.

Background

This PR is in support of dlang/dmd#10179

in as a parameter storage class is defined as scope const. However in has not yet
been properly implemented so its current implementation is equivalent to const. Properly
implementing in now will likely break code, so it is recommended to avoid using in, and
explicitly use const or scope const instead, until in is properly implemented.

The use of in as a parameter storage class is already discouraged in the documentation. See https://dlang.org/spec/function.html#parameters

dlang-bot · 2019-07-17T10:45:45Z

Thanks for your pull request and interest in making D better, @JinShil! 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 fetch digger
dub run digger -- build "master + druntime#2685"

JinShil · 2019-07-17T10:47:44Z

src/object.d

+    private void* _aaGetY(AA* paa, const TypeInfo_AssociativeArray ti, const size_t valsz, const scope void* pkey) pure nothrow;
+    private void* _aaGetX(AA* paa, const TypeInfo_AssociativeArray ti, const size_t valsz, const scope void* pkey, out bool found) pure nothrow;
+    // inout(void)* _aaGetRvalueX(inout AA aa, const scope TypeInfo keyti, const size_t valsz, const scope void* pkey);
+    inout(void[]) _aaValues(inout AA aa, const size_t keysz, const size_t valsz, const TypeInfo tiValueArray) pure nothrow;


I only used const for size_t as I believe scope is not applicable to value types.

Yes, it is ignored to make generic code agnostic to valueness.

thewilsonator · 2019-07-17T11:06:15Z

src/object.d(2515): Error: none of the overloads of `toString` are callable using argument types `(void)`, candidates are:
src/object.d(2512):        `object.Throwable.toString()`
src/object.d(2525):        `object.Throwable.toString(scope void delegate(scope const(char[])) sink)`

PetarKirov

There are various places where I think it's incorrect to assume const/scope correctness:
TypeInfo_Enum, TypeInfo_Array, TypeInfo_StaticArray, TypeInfo_AssociativeArray, TypeInfo_Class, TypeInfo_Interface, TypeInfo_Struct, TypeInfo_Const
and ultimately because of that I think we can't assume this for the base TypeInfo class as well.

That is because all these classes call directly or indirectly other code through a function pointer or virtual function interface, which we can't know if is const/scope correct. Perhaps we can ignore this for now, but if we want to migrate to a fully template-driven TypeInfo generation, we're going to have a lot of trouble then, when the compiler would be able to verify the true signatures of opEquals and opCmp.

PetarKirov · 2019-07-17T11:51:10Z

src/rt/typeinfo/ti_C.d

    }

-    override bool equals(in void* p1, in void* p2)
+    override bool equals(const scope void* p1, const scope void* p2)


This calls object.opEquals, which in turn calls object.Object.opEquals (if both p1 and p2 are different and != null) on arbitrary object of an arbitrary class type, whose opEquals(Object o) implementation may store a reference to o (however unlikely this may be).

PetarKirov · 2019-07-17T11:52:08Z

src/rt/typeinfo/ti_C.d

    }

-    override int compare(in void* p1, in void* p2)
+    override int compare(const scope void* p1, const scope void* p2)


Ditto - potentially calls object.Object.opCmp(Object o) and can't be scope.

src/rt/dmain2.d

PetarKirov · 2019-07-17T11:59:17Z

src/object.d

     * toString) method to customize the error message.
     */
-    void toString(scope void delegate(in char[]) sink) const
+    void toString(scope void delegate(const scope char[]) sink) const


Could be a breaking change for callers, but that should only affect their @safe code, when they compile with -preview=dip1000.

PetarKirov · 2019-07-17T12:03:42Z

src/object.d

-    override bool equals(in void *p1, in void *p2) const { return base.equals(p1, p2); }
-    override int compare(in void *p1, in void *p2) const { return base.compare(p1, p2); }
+    override bool equals(const scope void *p1, const scope void *p2) const { return base.equals(p1, p2); }
+    override int compare(const scope void *p1, const scope void *p2) const { return base.compare(p1, p2); }


base can be the TypeInfo for a class or struct which implements opEquals|opCompare without having the scope attribute on the parameter.

What do you recommend?

PetarKirov · 2019-07-17T12:21:14Z

src/object.d

+        size_t   function(const scope void*)           xtoHash;
+        bool     function(const scope void*, const scope void*) xopEquals;
+        int      function(const scope void*, const scope void*) xopCmp;
+        string   function(const scope void*)           xtoString;


Pedantically speaking, I don't think those function parameters can be neither scope nor const :(

Only when the TypeInfo generation is fully template-driven we can have true guarantees about const/scope/insert your favorite function attribute here correctness.

What do you recommend?

There are two options:

Use honest function signature and change those parameters to void* as in reality even the const-ness can't be guaranteed. This means that TypeInfo_Struct.{equals,compare} must also take void* parameters which is likely a breaking change.

Continue with the deceit and deal with problems somehow later down the line, when we can be technically honest.

@jmdavis @schveiguy @atilaneves What's your point of view?

Not sure. I agree we can't make them scope or const though.

@ZombineDev Why can't these be const? They shouldn't be modifying p.

@JinShil Because they are function pointers to arbitrary user code. The pointer itself may not be modified (as it will be copied), but the object pointed by p may be. And it can also escape p without any issues.

Here's the sort of techniques that we need to use in order to be type safe:
https://gist.github.com/ZombineDev/ac19e93dff4f7b251be0c7af8e8d150d#file-proper_type_info-d-L62-L73

Live demo: https://run.dlang.io/gist/ZombineDev/ac19e93dff4f7b251be0c7af8e8d150d?compiler=dmd&args=-dip1000

JinShil · 2019-09-02T09:59:21Z

A decision about in has finally been made: dlang/dmd#10382 (comment)

JinShil requested review from MartinNowak, WalterBright, andralex and wilzbach as code owners July 17, 2019 10:45

JinShil commented Jul 17, 2019

View reviewed changes

thewilsonator approved these changes Jul 17, 2019

View reviewed changes

PetarKirov reviewed Jul 17, 2019

View reviewed changes

Replace in with const scope for object.d rt/dmain2.d and rt/typeinfo

0f08398

JinShil mentioned this pull request Jul 26, 2019

Replace some usages of in with const scope for object.d and rt/dmain2.d #2693

Merged

JinShil mentioned this pull request Sep 2, 2019

Add -transition=in compiler flag to help users transition away from using in dlang/dmd#10382

Closed

JinShil closed this Sep 2, 2019

JinShil deleted the discourage_in_2c branch September 2, 2019 09:59

Uh oh!

Replace in with const scope for object.d rt/dmain2.d and rt/typeinfo #2685

Replace in with const scope for object.d rt/dmain2.d and rt/typeinfo #2685

Uh oh!

Conversation

JinShil commented Jul 17, 2019

About This PR

Background

Uh oh!

dlang-bot commented Jul 17, 2019

Bugzilla references

Testing this PR locally

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thewilsonator commented Jul 17, 2019

Uh oh!

PetarKirov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetarKirov Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JinShil commented Sep 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Replace `in` with `const scope` for object.d rt/dmain2.d and rt/typeinfo #2685

Replace `in` with `const scope` for object.d rt/dmain2.d and rt/typeinfo #2685

PetarKirov Jul 17, 2019 •

edited

Loading