fix Issue 18143 - in/out contracts should be implicitly 'this' const by ibuclaw · Pull Request #7553 · dlang/dmd

ibuclaw · 2017-12-30T11:58:37Z

Actual error message should probably be ratified. I just borrowed the same format as the error above for modifying other parameters in contracts.

dlang-bot · 2017-12-30T11:58:38Z

Thanks for your pull request, @ibuclaw!

Bugzilla references

Auto-close	Bugzilla	Description
✓	18143	in/out contracts should be implicitly 'this' const

quickfur · 2018-01-02T18:43:25Z

src/dmd/semantic3.d

                sc2.flags = (sc2.flags & ~SCOPEcontract) | SCOPErequire;

                // BUG: need to error if accessing out parameters
-                // BUG: need to treat parameters as const


Hmm. This PR seems to only disable writing to this, but does it also prohibit modifying function parameters?

@quickfur that was implemented in #1569. Look at the whole of checkModify.

Ah, ok. Nevermind then. :-) I was just confused because deleting this comment in this PR makes it seem like this PR is fixing the entire issue. I suppose making this const is the last remaining issue as far as this comment is concerned?

quickfur · 2018-01-02T20:15:32Z

LGTM.

JinShil · 2018-01-02T23:50:06Z

Looks like this may have found a bug in Sociomantic's Ocean

./src/ocean/net/http/HttpResponse.d(466): Error: variable ocean.net.http.HttpResponse.HttpResponse.AppendHeaderLines.IncrementalValue.this cannot modify parameter 'this' in contract

cc @leandro-lucarella-sociomantic @mathias-lang-sociomantic

quickfur · 2018-01-02T23:57:09Z

Very nice! I love compiler changes that discover bugs in existing code!

ibuclaw · 2018-01-03T00:16:11Z

It certainly looks suspicious.
https://github.com/sociomantic-tsunami/ocean/blob/84612d09b3ede91ef27333be72b78836997e58e3/src/ocean/net/http/HttpResponse.d#L466

Contains fix for the bug which will be detected by dlang/dmd#7553

wilzbach · 2018-01-05T13:45:07Z

Looks like this may have found a bug in Sociomantic's Ocean

Thanks to the work of @iain-buclaw-sociomantic and @mihails-strasuns-sociomantic Ocean was fixed and upgraded:

sociomantic-tsunami/ocean#427
dlang/ci#108

-> Jenkins should be passing now :)

wilzbach

We should really stop documenting bugs in the source code where they are just forgotten or confuse people because it has been fixed. Should we add the fixed "BUGS" as test cases to the testsuite?

wilzbach · 2018-01-05T13:56:24Z

src/dmd/semantic3.d

                sc2.flags = (sc2.flags & ~SCOPEcontract) | SCOPEensure;

-                // BUG: need to treat parameters as const
                // BUG: need to disallow returns and throws


We shouldn't document bugs in the source code like this, they should be in Bugzilla:

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

Also return is already forbidden: https://run.dlang.io/is/A0JWgT

I didn't verify any of the comments, so no idea what the state is.

wilzbach · 2018-01-05T13:59:46Z

src/dmd/semantic3.d

@@ -875,7 +875,6 @@ extern(C++) final class Semantic3Visitor : Visitor
                sc2.flags = (sc2.flags & ~SCOPEcontract) | SCOPErequire;

                // BUG: need to error if accessing out parameters


Isn't this already forbidden?

https://run.dlang.io/is/NacqYo

But is that accessing the out parameter, or out parameters?

Actually, I've worked out what this means:

static int cache; int foo(out int bar) in { cache = bar; // This should be an error. somefunc(bar); // This should be an error. assert(bar == 0); // This should be an error. }

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

wilzbach · 2018-01-06T17:08:36Z

(rebased to hopefully fix Jenkins)

quickfur · 2018-01-06T22:07:42Z

Excellent!

ibuclaw · 2018-01-07T11:03:04Z

src/dmd/semantic3.d

                // BUG: need to error if accessing out parameters
-                // BUG: need to treat parameters as const
                // BUG: need to disallow returns and throws
                // BUG: verify that all in and ref parameters are read


This comment as seen today appeared in D1.011.

Before it used to be:

BUG: verify that all in and inout parameters are read

Which has been here since the dawn of ~~time~~ svn history.

It seems to allude to this:

int foo(in int a, ref int b) in { // Should emit error: 'a' is not read in contract. // Should emit error: 'b' is not read in contract. } do { return a + b; }

But I don't think that any of those are a compiler bug at all, and so this comment should be removed. @WalterBright do I understand the context correctly?

Why should an error be emitted if a parameter isn't read in the in-contract? How else would you express that any possible value is permitted for that parameter?

Some languages enforce that some parameter are used, e.g. C# forces you to assign out parameters. One way to express this in an in contract would be to do cast(void)a; for example. But that's the kind of enforcement which IMO goes against D's approach, as many times you'll end up just trying to satisfy the compiler.

I'm saying I don't think it should be considered as a bug. Also the comments doesn't say anything about all parameters. Just in and ref parameters.

verify that all in and ref parameters are read

It may also be implying that all input parameters should actually be used in the implementation.

ibuclaw requested a review from WalterBright as a code owner December 30, 2017 11:58

dlang-bot added the Severity:Bug Fix label Dec 30, 2017

ibuclaw force-pushed the issue18143 branch 2 times, most recently from a8fe66a to a90b717 Compare December 30, 2017 12:07

dlang-bot added Review:Needs Rebase and removed Review:Needs Rebase labels Jan 1, 2018

quickfur reviewed Jan 2, 2018

View reviewed changes

wilzbach mentioned this pull request Jan 5, 2018

Remove bogus out contract from IncrementalValue dtor. sociomantic-tsunami/ocean#427

Merged

mihails-strasuns-sociomantic added a commit to mihails-strasuns-sociomantic/ci that referenced this pull request Jan 5, 2018

Test ocean v4.0.0-alpha.4

4950e92

Contains fix for the bug which will be detected by dlang/dmd#7553

wilzbach approved these changes Jan 5, 2018

View reviewed changes

wilzbach force-pushed the issue18143 branch from a90b717 to aa5bc65 Compare January 6, 2018 17:08

wilzbach mentioned this pull request Jan 6, 2018

Updates sometimes don't propogate dlang/ci#112

Closed

ibuclaw force-pushed the issue18143 branch from aa5bc65 to 747377b Compare January 6, 2018 17:40

fix Issue 18143 - in/out contracts should be implicitly 'this' const

3d024cb

ibuclaw force-pushed the issue18143 branch from 747377b to 3d024cb Compare January 6, 2018 17:44

JinShil approved these changes Jan 6, 2018

View reviewed changes

JinShil added the Merge:auto-merge label Jan 6, 2018

dlang-bot merged commit 4c7ea17 into dlang:master Jan 6, 2018

ibuclaw deleted the issue18143 branch January 6, 2018 22:55

wilzbach mentioned this pull request Jan 7, 2018

Temporarily patch Ocean to workaround missing out contract patch dlang/ci#115

Merged

ibuclaw commented Jan 7, 2018

View reviewed changes

		@@ -875,7 +875,6 @@ extern(C++) final class Semantic3Visitor : Visitor
		sc2.flags = (sc2.flags & ~SCOPEcontract) \| SCOPErequire;

		// BUG: need to error if accessing out parameters

Uh oh!

Comments

Conversation

ibuclaw commented Dec 30, 2017

Uh oh!

dlang-bot commented Dec 30, 2017

Bugzilla references

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!

quickfur commented Jan 2, 2018

Uh oh!

JinShil commented Jan 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

quickfur commented Jan 2, 2018

Uh oh!

ibuclaw commented Jan 3, 2018

Uh oh!

wilzbach commented Jan 5, 2018

Uh oh!

wilzbach 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!

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!

ibuclaw Jan 7, 2018 • 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!

wilzbach commented Jan 6, 2018

Uh oh!

quickfur commented Jan 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw Jan 7, 2018 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

JinShil commented Jan 2, 2018 •

edited

Loading

ibuclaw Jan 7, 2018 •

edited

Loading

ibuclaw Jan 7, 2018 •

edited

Loading