Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dlang-bot merged 1 commit into from
Jan 6, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/dmd/declaration.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,20 @@ extern (C++) abstract class Declaration : Dsymbol
}
}

if (e1 && e1.op == TOKthis && isField())
{
VarDeclaration vthis = (cast(ThisExp)e1).var;
for (Scope* scx = sc; scx; scx = scx.enclosing)
{
if (scx.func == vthis.parent && (scx.flags & SCOPEcontract))
{
if (!flag)
error(loc, "cannot modify parameter 'this' in contract");
return 2; // do not report type related errors
}
}
}

if (v && (isCtorinit() || isField()))
{
// It's only modifiable if inside the right constructor
Expand Down
2 changes: 0 additions & 2 deletions src/dmd/semantic3.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,7 +885,6 @@ private extern(C++) final class Semantic3Visitor : Visitor
sc2.flags = (sc2.flags & ~SCOPEcontract) | SCOPErequire;

// BUG: need to error if accessing out parameters
Copy link
Contributor

@wilzbach wilzbach Jan 5, 2018

Choose a reason for hiding this comment

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

Isn't this already forbidden?

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

Copy link

@iain-buclaw-sociomantic iain-buclaw-sociomantic Jan 5, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@ibuclaw ibuclaw Jan 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.
}

Copy link
Member Author

@ibuclaw ibuclaw Jan 7, 2018

Choose a reason for hiding this comment

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

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

// BUG: need to treat parameters as const
Copy link
Member

@quickfur quickfur Jan 2, 2018

Choose a reason for hiding this comment

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

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

Copy link

@iain-buclaw-sociomantic iain-buclaw-sociomantic Jan 2, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

@quickfur quickfur Jan 2, 2018

Choose a reason for hiding this comment

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

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?

// BUG: need to disallow returns and throws
// BUG: verify that all in and ref parameters are read
Copy link
Member Author

@ibuclaw ibuclaw Jan 7, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@ibuclaw ibuclaw Jan 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

@quickfur quickfur Jan 8, 2018

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@mathias-lang-sociomantic mathias-lang-sociomantic Jan 8, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@ibuclaw ibuclaw Jan 8, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@JinShil JinShil Jan 8, 2018

Choose a reason for hiding this comment

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

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.

freq = freq.statementSemantic(sc2);
Expand All @@ -908,7 +907,6 @@ private extern(C++) final class Semantic3Visitor : Visitor
sc2 = scout; //push
sc2.flags = (sc2.flags & ~SCOPEcontract) | SCOPEensure;

// BUG: need to treat parameters as const
// BUG: need to disallow returns and throws
Copy link
Contributor

@wilzbach wilzbach Jan 5, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@wilzbach wilzbach Jan 5, 2018

Choose a reason for hiding this comment

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

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

Copy link

@iain-buclaw-sociomantic iain-buclaw-sociomantic Jan 5, 2018

Choose a reason for hiding this comment

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

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


if (funcdecl.fensure && f.next.ty != Tvoid)
Expand Down
43 changes: 43 additions & 0 deletions test/fail_compilation/fail18143.d
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
TEST_OUTPUT:
---
fail_compilation/fail18143.d(20): Error: variable fail18143.S.a cannot modify parameter 'this' in contract
fail_compilation/fail18143.d(21): Error: variable fail18143.S.a cannot modify parameter 'this' in contract
fail_compilation/fail18143.d(25): Error: variable fail18143.S.a cannot modify parameter 'this' in contract
fail_compilation/fail18143.d(26): Error: variable fail18143.S.a cannot modify parameter 'this' in contract
fail_compilation/fail18143.d(35): Error: variable fail18143.C.a cannot modify parameter 'this' in contract
fail_compilation/fail18143.d(36): Error: variable fail18143.C.a cannot modify parameter 'this' in contract
fail_compilation/fail18143.d(40): Error: variable fail18143.C.a cannot modify parameter 'this' in contract
fail_compilation/fail18143.d(41): Error: variable fail18143.C.a cannot modify parameter 'this' in contract
---
*/

struct S
{
int a;

this(int n)
in { a = n; } // error, modifying this.a in contract
out { a = n; } // error, modifying this.a in contract
do { }

void foo(int n)
in { a = n; } // error, modifying this.a in contract
out { a = n; } // error, modifying this.a in contract
do { }
}

class C
{
int a;

this(int n)
in { a = n; } // error, modifying this.a in contract
out { a = n; } // error, modifying this.a in contract
do { }

void foo(int n)
in { a = n; } // error, modifying this.a in contract
out { a = n; } // error, modifying this.a in contract
do { }
}