fix Issue 16747 - Cannot have stack allocated classes in @safe code by WalterBright · Pull Request #6279 · dlang/dmd

WalterBright · 2016-11-24T12:32:51Z

Stack allocated classes with no destructors do not need to call delete.

dlang-bot · 2016-11-24T12:32:54Z

Fix	Bugzilla	Description
✓	16747	[Reg 2.072] Cannot have stack allocated classes in @safe code

mathias-lang-sociomantic · 2016-11-24T12:59:17Z

test/fail_compilation/test16747.d

+void fwd() @safe
+{
+    scope o = new Object();
+    scope c = new C();


If the dtor is safe, why wouldn't we allow destruction ?

I think it's because delete uses a C interface and destroys using TypeInfo.

But surely at this point, we know the destruction is @safe ?

MartinNowak

Please add a separate error message (e.g. "deleting classes with destructors is unsafe atm.") for the hasDtor case, it seems like an internal limitation of the dmd/druntime API, and it would be very hard for people to understand why their code breaks by adding a dtor.

MartinNowak · 2016-11-26T07:23:09Z

Non-risky bugfixes and in particular all regression fixes should always go into stable.
It's unacceptable that you're constantly ignoring this simple rule.
We've already missed several regression fixes in previous releases due to this not being followed. We could use the dlang-bot to catch this.

I've changed the target branch to stable, please rebase your PR.
See http://stackoverflow.com/a/10853956 for how to do that.

MartinNowak · 2016-11-27T12:15:50Z

Ping

MartinNowak · 2016-11-27T12:18:57Z

Something along the line of git rebase --onto upstream/stable upstream/master fix16747 should do the trick. Make sure to fetch upstream beforehand.

mihails-strasuns · 2016-11-28T02:42:02Z

@MartinNowak FYI: I have rebased it and would highly recommend everyone to take similar approach. Wait time on back-and-forth requests for technical trivialities is quite a productivity killer - it is much more practical to make such adjustments directly and move on.

mathias-lang-sociomantic · 2016-11-28T10:21:53Z

test/fail_compilation/test16195.d

 * TEST_OUTPUT:
 ---
-fail_compilation/test16195.d(13): Error: delete p is not @safe but is used in @safe function test
+fail_compilation/test16195.d(13): Error: cannot use 'delete' in @nogc function 'test16195.test'


delete is not @nogc ?

new/delete are part of the gc.

Makes sense. I was under the impression that it would affect scope class, but it doesn't.
However, the following now compiles with the P.R. (commit 43ddb33):

@safe void dl (int* v) { delete v; }

Which is not the intent of this P.R.

mathias-lang-sociomantic · 2016-11-28T18:52:07Z

test/fail_compilation/fail14486.d

 TEST_OUTPUT:
 ---
-fail_compilation/fail14486.d(44): Error: delete c0 is not @safe but is used in @safe function test1a
+fail_compilation/fail14486.d(44): Error: cannot use 'delete' in @nogc function 'fail14486.test1a'


My comments applied to all the change to the test suite.

void main () @safe { auto f = new Object(); delete f; }

Compiles with your P.R., and obviously shouldn't

mathias-lang-sociomantic · 2016-11-28T19:01:42Z

What about the following ?

void c () @safe
{
    scope o = new Object;
    delete o;
    auto x = o.toHash();
}

I'd suggest we just prohibit explicit destruction of scope instance

mihails-strasuns · 2016-11-29T12:17:59Z

This definitely requires adding distinction between compiler-injected delete call and manual one (i.e. define __delete_automatic which does the same as delete but is allowed in @safe.

WalterBright · 2016-11-30T07:03:37Z

@Dicebot yes, I think you're right. But I don't have time to work on this at the moment, it'll have to wait a bit.

MartinNowak · 2016-12-10T05:31:53Z

@MartinNowak FYI: I have rebased it and would highly recommend everyone to take similar approach. Wait time on back-and-forth requests for technical trivialities is quite a productivity killer - it is much more practical to make such adjustments directly and move on.

I explicitly didn't do it, b/c I need Walter to learn how to target the stable branch. It doesn't help if you or I constantly have to rebase PRs from other people. Maybe we should reconsider a few github workflows to make this smoother, but the fact that people already have problems w/ only 2 branches is fairly frustrating.
And the rule is really trivial, if you want to fix sth. that has already been released, you branch from stable and target stable, always.

WalterBright · 2016-12-12T22:17:56Z

ping @MartinNowak

mathias-lang-sociomantic

LGTM

mihails-strasuns · 2016-12-13T16:15:49Z

src/expression.h


 class DeleteExp : public UnaExp
 {
+    bool isRIAA;


mihails-strasuns · 2016-12-14T08:53:17Z

FYI @MartinNowak this is the PR we have mentioned yesterday that most likely needs to be moved to scope branch. Please sanity check me.

mathias-lang-sociomantic · 2016-12-14T09:42:45Z

Why ? It's a regression in 2.072.0. It is valid code which stopped to compile.

mihails-strasuns · 2016-12-14T14:53:22Z

It opens an infinite hole in @safe without lifetime checks like ones implememted in scope branch.

mathias-lang-sociomantic · 2016-12-14T14:58:53Z

It opens an infinite hole in @safe without lifetime checks like ones implememted in scope branch.

This hole was already there before 2.072.0. What 2.072.0 did was breaking code that could be valid (but could not be verified by the compiler). And breaking valid code is not something we should ever do.

MartinNowak · 2016-12-15T00:32:25Z

Yes, but thanks for mentioning this anyhow, it's not obvious from the PR and prolly wasn't considered.

Disallowing the use of DeleteExp in @safe code (#5887) did incidentally fix escaping of references to stack allocated classes, by bluntly breaking all usage of scope classes.
Usage of scope classes in @safe code was a safety hole before 2.072.0 and should be addressed by the scope/DIP1000 work.
While this PR will reopen safety holes w/ scope classes in 2.072.2, it's much more important to not have an intermediate (until scope/DIP1000 work) breakage of all stack allocated classes in @safe code.

MartinNowak · 2016-12-15T01:36:04Z

Auto-merge toggled on

MartinNowak · 2016-12-15T12:39:12Z

There is one failing test on OSX (even after repeating it).
https://auto-tester.puremagic.com/show-run.ghtml?projectid=14&runid=2312526&isPull=true
https://auto-tester.puremagic.com/show-run.ghtml?projectid=14&runid=2311102&isPull=true
It rather looks like an issue with the OSX auto tester machine (D-Autotester.local (98.229.209.219)) @braddr.
So I'm still merging this because the same OSX tests on Travis-CI do pass and it fixes a major regression.

@safe

fix Issue 16747 - Cannot have stack allocated classes in @safe code

@safe

fix Issue 16747 - Cannot have stack allocated classes in @safe code

Merge pull request #6279 from WalterBright/fix16747

WalterBright force-pushed the fix16747 branch from 51141eb to 9a21dd6 Compare November 24, 2016 12:55

mathias-lang-sociomantic reviewed Nov 24, 2016

View reviewed changes

MartinNowak requested changes Nov 26, 2016

View reviewed changes

MartinNowak changed the base branch from master to stable November 26, 2016 07:23

mihails-strasuns-sociomantic force-pushed the fix16747 branch from 9a21dd6 to 2b1246a Compare November 28, 2016 02:40

WalterBright force-pushed the fix16747 branch 3 times, most recently from f44da90 to 43ddb33 Compare November 28, 2016 09:49

mathias-lang-sociomantic reviewed Nov 28, 2016

View reviewed changes

WalterBright force-pushed the fix16747 branch 2 times, most recently from 5ce65bc to 66d15ca Compare November 28, 2016 12:37

mathias-lang-sociomantic reviewed Nov 28, 2016

View reviewed changes

WalterBright force-pushed the fix16747 branch 5 times, most recently from 7439d2b to 4ac3fbe Compare December 2, 2016 10:54

mathias-lang-sociomantic approved these changes Dec 12, 2016

View reviewed changes

mihails-strasuns reviewed Dec 13, 2016

View reviewed changes

fix Issue 16747 - Cannot have stack allocated classes in @safe code

a07034f

WalterBright force-pushed the fix16747 branch from 4ac3fbe to a07034f Compare December 14, 2016 00:36

MartinNowak approved these changes Dec 15, 2016

View reviewed changes

mihails-strasuns approved these changes Dec 15, 2016

View reviewed changes

MartinNowak merged commit 6936a2e into dlang:stable Dec 15, 2016

MartinNowak added a commit to MartinNowak/dmd that referenced this pull request Dec 19, 2016

Merge pull request dlang#6279 from WalterBright/fix16747

84d9e78

fix Issue 16747 - Cannot have stack allocated classes in @safe code

MartinNowak added a commit to MartinNowak/dmd that referenced this pull request Dec 19, 2016

Merge pull request dlang#6279 from WalterBright/fix16747

0117027

fix Issue 16747 - Cannot have stack allocated classes in @safe code

WalterBright added a commit that referenced this pull request Dec 19, 2016

Merge pull request #6339 from MartinNowak/fix16747

1dc8c17

Merge pull request #6279 from WalterBright/fix16747

Uh oh!

Conversation

WalterBright commented Nov 24, 2016

Uh oh!

dlang-bot commented Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

MartinNowak left a comment

Choose a reason for hiding this comment

Uh oh!

MartinNowak commented Nov 26, 2016

Uh oh!

MartinNowak commented Nov 27, 2016

Uh oh!

MartinNowak commented Nov 27, 2016

Uh oh!

mihails-strasuns commented Nov 28, 2016

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!

mathias-lang-sociomantic commented Nov 28, 2016

Uh oh!

mihails-strasuns commented Nov 29, 2016

Uh oh!

WalterBright commented Nov 30, 2016

Uh oh!

MartinNowak commented Dec 10, 2016

Uh oh!

WalterBright commented Dec 12, 2016

Uh oh!

mathias-lang-sociomantic 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!

mihails-strasuns commented Dec 14, 2016

Uh oh!

mathias-lang-sociomantic commented Dec 14, 2016

Uh oh!

mihails-strasuns commented Dec 14, 2016

Uh oh!

mathias-lang-sociomantic commented Dec 14, 2016

Uh oh!

MartinNowak commented Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MartinNowak commented Dec 15, 2016

Uh oh!

MartinNowak commented Dec 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dlang-bot commented Nov 24, 2016 •

edited

Loading

MartinNowak commented Dec 15, 2016 •

edited

Loading