Fix Issue 18295 - `scope class` check too conservative under -dip1000 #7771

JinShil · 2018-01-24T21:20:27Z

See issue for the details. This PR also adds coverage for previously uncovered case.

FYI: This is in direct support of dlang/dlang.org#2027

dlang-bot · 2018-01-24T21:20:30Z

Thanks for your pull request, @JinShil!

Bugzilla references

Auto-close	Bugzilla	Description
✓	18295	[Scope][dip1000] `scope class` check too conservative under -dip1000

MartinNowak · 2018-01-24T23:23:45Z

test/compilable/test18295.d

+
+scope class C { int i; }    // Notice the use of `scope` here
+
+C increment(C c)            // Should not emit error here


As this is a function there is no inferrence, and if the scope parameter is not return, it cannot be returned.

MartinNowak

Right, all scope class means is enforcing the usage of
scope var = new Clazz, it has no implications on the scope checks besides that.
In particular you still need to annotate parameters as return scope. Remember, scope is a storage class, not a type qualifier, so it doesn't simply carry to every usage of that type.
I'm inclined to say that forcing a particular storage on users of a class is a weird use-case and unprecedented for other storage options (GC, RC). Maybe we should deprecate scope class C {} declarations in the long-run? Not sure why this mechanism was introduced? To catch unwanted GC allocations?

MartinNowak · 2018-01-24T23:27:18Z

test/compilable/test18295.d

+
+scope class C { int i; }    // Notice the use of `scope` here
+
+C increment(C c)            // Should not emit error here


Yes it should error, you need return scope on the parameter for this to pass.

Added return scope to the parameter

MartinNowak · 2018-01-24T23:31:38Z

test/compilable/test18295.d

+void main()
+{
+    scope C c = new C();
+    c.increment();


Should error here, cannot pass scope to non-scope parameter c.

bug.d(10): Error: scope variable `c` assigned to non-scope parameter `c` calling bug.increment

Added return scope to the parameter, there should be no error.

MartinNowak · 2018-01-24T23:31:57Z

test/compilable/test18295.d

+// https://issues.dlang.org/show_bug.cgi?id=18295
+
+// See test/fail_compilation/test18295.d for the non-`-dip1000` version`
+


Without @safe: there are no scope checks.

Added @safe attributes

MartinNowak · 2018-01-24T23:33:01Z

test/fail_compilation/test18295.d

+void main()
+{
+    scope C c = new C();
+    c.increment();


That simply won't catch the escaping of a scope variable, checks are only enabled with -dip1000.

That's not the point of this test. The point is to test the current behavior for which there currently is no test. See https://codecov.io/gh/dlang/dmd/src/master/src/dmd/typesem.d#L800...801 If you believe that is an error, please file a bug in bugzilla and submit a fix in a separate PR.

wilzbach · 2018-01-24T23:40:13Z

Maybe we should deprecate scope class C {} declarations in the long-run? Not sure why this mechanism was introduced? To catch unwanted GC allocations?

FYI: scope A a = new A(1); is already deprecated:

https://dlang.org/deprecate.html#scope%20for%20allocating%20classes%20on%20the%20stack

JinShil · 2018-01-24T23:54:57Z

Maybe we should deprecate scope class C {} declarations in the long-run?
FYI: scope A a = new A(1); is already deprecated:

The de-deprecation has been approved. See dlang/dlang.org#2027

Remember, scope is a storage class, not a type qualifier, so it doesn't simply carry to every usage of that type.

When scope is attributed to the class declaration, it serves as a type qualifier and does carry to every usage of that type, which is why the following emits an error:

scope class A { }

void main()
{
    A a = new A();   // Error: variable main.a reference to scope class must be scope
}

wilzbach · 2018-01-24T23:58:53Z

Jenkins failure is unrelated and should be gone soon (-> dlang/ci#132)

JinShil · 2018-01-25T01:21:13Z

I'm inclined to say that forcing a particular storage on users of a class is a weird use-case and unprecedented for other storage options (GC, RC). Not sure why this mechanism was introduced?

I find it to be rather useful as a way of enforcing that all instances of the class are stack allocated. It's consistent with constraints such as final in that it places restrictions on how the class is employed.

MartinNowak · 2018-01-25T13:55:14Z

Well, final allows you to restrict any compatibility-relevant usage.
What's use-case for restricting heap allocation of classes?

/+
dub.sdl:
dependency "automem" version="~>0.0.10"
+/
import automem, std.typecons : scoped;

scope class C {}

void main()
{
    auto c1 = scoped!C();
    auto c2 = RefCounted!C();
}

If there was a clear need for local only classes, that restriction might be warranted, I just don't see any.

MartinNowak · 2018-01-25T13:56:46Z

test/fail_compilation/test18295.d

+
+scope class C { int i; }    // Notice the use of `scope` here
+
+C increment(C c)            // Not compiling under -dip1000, so this should emit an error


The comment seems to be wrong.

Right or wrong, that's the way it currently works. All this test is doing is adding coverage to the existing implementation. The compilable/test18295.d file is the test for the new functionality in this PR.

This test has been merged in a separate PR (#7791), so we can get off this tangent.

MartinNowak

Looks OKish. Though I don't think anyone wants to define those half-implemented scope classes and their interaction with DIP1000 while we're working on @safe manual memory management and the whole topic is still in flux.

MartinNowak · 2018-01-25T14:06:20Z

test/compilable/test18295.d

+
+// See test/fail_compilation/test18295.d for the non-`-dip1000` version`
+
+scope class C { int i; }    // Notice the use of `scope` here


scope class is irrelevant for this test, please remove the misleading comment.
Only the fact that c is a scoped class reference matters for the return scope check.

It's not irrelevant, and in fact is the whole point of this PR. Prior to this PR, due to the scope class attribution, an error would be emitted at the function declaration C increment(scope return C c) @safe, which is too conservative. After this PR, there is no error.

MartinNowak · 2018-01-25T14:11:19Z

test/compilable/test18295.d

+
+scope class C { int i; }    // Notice the use of `scope` here
+
+C increment(return scope C c) @safe      // Should not emit error here


Ideally we'd want to add this -dip1000 test as well.

scope class C { int i; } C increment(return ref C c) @safe // check implicit scope from class type { c.i++; return c; } void main() @safe { scope C c = new C(); c.increment(); }

Doesn't work though cause scope class isn't a real class reference.

bug.d(3): Error: variable bug.increment.c globals, statics, fields, manifest constants, ref and out parameters cannot be `scope`

Then I suggest merging this PR and filing a bug for that example.

JinShil · 2018-01-28T02:27:31Z

What's use-case for restricting heap allocation of classes?

That's a good question for the folks at Sociomantic who have clearly found a use for it in Ocean. @mathias-lang-sociomantic?

mihails-strasuns-sociomantic · 2018-01-29T10:11:30Z

(answering @mathias-lang-sociomantic summons)

@JinShil it is a legacy of D1 which doesn't have struct constructors/destructors, which means scope class was the only form of non-trivial stack-allocated aggregates available. Because we have to keep compatibility with both language versions it was not possible to switch to structs.

In the long term there is nothing preventing deprecation of scope classes and switching to structs instead, but that is both non-trivial and somewhat low priority (there are more important problems with D2 migration that are still unresolved).

Important to note that we only care about actual scope instance = new Thing allocation - none of dip1000 constraints will matter because we never use @safe which enables them.

JinShil · 2018-01-29T12:29:41Z

In the long term there is nothing preventing deprecation of scope classes and switching to structs instead

So be it. Then I leave it to @WalterBright and @andralex to decide the fate of the scope class in dlang/dlang.org#2027

JinShil · 2018-03-20T07:12:00Z

@WalterBright has made his view known at dlang/dlang.org#2027 (comment)

Deprecate scope class C { }. It's kind of a bizarre, neglected quirk. It forces all allocations of C to go on the stack. It's hard to see what the point of it is these days.

Retain scope C c = new C();. The C gets allocated on the stack instead of the heap, but pedantically this is an optimization. The -dip1000 checks ensure it works in that c is not allowed to escape the lifetime of its stack frame.

Therefore, instead of fixing this issue (Issue 18295), I'll move ahead deprecating scope-as-a-type-modifier, which will eventually make the issue irrelevant. Closing.

dlang-bot added the Bug Fix label Jan 24, 2018

JinShil changed the title ~~Fix Issue 18295 - check too conservative under -dip1000~~ Fix Issue 18295 - scope class check too conservative under -dip1000 Jan 24, 2018

MartinNowak reviewed Jan 24, 2018

View reviewed changes

MartinNowak reviewed Jan 25, 2018

View reviewed changes

This was referenced Jan 27, 2018

Clarify deprecation of scope as a type constraint dlang/dlang.org#2027

Merged

Add test coverage for scope class #7791

Merged

Fix Issue 18295 - scope class check too conservative under -dip1000

863b49a

JinShil closed this Mar 20, 2018

JinShil deleted the scope_class_too_tight branch April 24, 2018 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Issue 18295 - `scope class` check too conservative under -dip1000 #7771

Fix Issue 18295 - `scope class` check too conservative under -dip1000 #7771

JinShil commented Jan 24, 2018 •

edited

Loading

dlang-bot commented Jan 24, 2018

MartinNowak Jan 24, 2018

MartinNowak left a comment

MartinNowak Jan 24, 2018

JinShil Jan 25, 2018

MartinNowak Jan 24, 2018

JinShil Jan 25, 2018

MartinNowak Jan 24, 2018

JinShil Jan 25, 2018

MartinNowak Jan 24, 2018

JinShil Jan 25, 2018

wilzbach commented Jan 24, 2018

JinShil commented Jan 24, 2018 •

edited

Loading

wilzbach commented Jan 24, 2018

JinShil commented Jan 25, 2018 •

edited

Loading

MartinNowak commented Jan 25, 2018

MartinNowak Jan 25, 2018

JinShil Jan 25, 2018

JinShil Jan 28, 2018

MartinNowak left a comment

MartinNowak Jan 25, 2018

JinShil Jan 28, 2018 •

edited

Loading

MartinNowak Jan 25, 2018

JinShil Jan 28, 2018 •

edited

Loading

JinShil commented Jan 28, 2018 •

edited

Loading

mihails-strasuns-sociomantic commented Jan 29, 2018

JinShil commented Jan 29, 2018

JinShil commented Mar 20, 2018


		scope class C { int i; } // Notice the use of `scope` here

		C increment(C c) // Should not emit error here

		// https://issues.dlang.org/show_bug.cgi?id=18295

		// See test/fail_compilation/test18295.d for the non-`-dip1000` version`


		scope class C { int i; } // Notice the use of `scope` here

		C increment(C c) // Not compiling under -dip1000, so this should emit an error


		scope class C { int i; } // Notice the use of `scope` here

		C increment(return scope C c) @safe // Should not emit error here

Fix Issue 18295 - scope class check too conservative under -dip1000 #7771

Fix Issue 18295 - scope class check too conservative under -dip1000 #7771

Conversation

JinShil commented Jan 24, 2018 • edited Loading

dlang-bot commented Jan 24, 2018

Bugzilla references

Choose a reason for hiding this comment

MartinNowak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach commented Jan 24, 2018

JinShil commented Jan 24, 2018 • edited Loading

wilzbach commented Jan 24, 2018

JinShil commented Jan 25, 2018 • edited Loading

MartinNowak commented Jan 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinNowak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinShil Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinShil Jan 28, 2018 • edited Loading

Choose a reason for hiding this comment

JinShil commented Jan 28, 2018 • edited Loading

mihails-strasuns-sociomantic commented Jan 29, 2018

JinShil commented Jan 29, 2018

JinShil commented Mar 20, 2018

Fix Issue 18295 - `scope class` check too conservative under -dip1000 #7771

Fix Issue 18295 - `scope class` check too conservative under -dip1000 #7771

JinShil commented Jan 24, 2018 •

edited

Loading

JinShil commented Jan 24, 2018 •

edited

Loading

JinShil commented Jan 25, 2018 •

edited

Loading

JinShil Jan 28, 2018 •

edited

Loading

JinShil Jan 28, 2018 •

edited

Loading

JinShil commented Jan 28, 2018 •

edited

Loading