Add Expression-Based Contract Syntax by tgehr · Pull Request #8155 · dlang/dmd

tgehr · 2018-04-09T19:00:07Z

Implementation of DIP 1009.

dlang-bot · 2018-04-09T19:00:11Z

Thanks for your pull request and interest in making D better, @tgehr! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
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 + dmd#8155"

thewilsonator · 2018-04-11T05:37:53Z

src/dmd/arraytypes.h


 typedef Array<class TemplateInstance *> TemplateInstances;

+typedef Array<class Ensure> Ensures;


Should this not be struct Ensure?

It does not make a difference in code semantics, but I guess using struct is better style.

ibuclaw

The most notable thing missing here is documentation, both for new code blocks added and ddoc entries for functions and types introduced.

Other than that, just a couple notes on implementation detail that are trivial or could be explained away by adding documentation for why it's needed.

ibuclaw · 2018-04-11T05:57:06Z

src/dmd/func.d

+                    o.push(new TypeIdentifier(rloc,Id.result));
+                    import dmd.cond: StaticIfCondition;
+                    auto c = new StaticIfCondition(rloc, new TraitsExp(loc, Id.compiles, o));
+                    auto result = new ConditionalStatement(rloc, c, new ExpStatement(rloc, a), null);


Why do we need an artificial static if here?

This was a hack to improve diagnostics. (__result should not appear in the output.)
The static if condition always passes in well-formed D programs, but will fail if out contracts of void functions specify a result identifier. I'd prefer to remove this. I can just not semantically analyze the out contracts if one of them specifies an output but the result is void, I guess.

ibuclaw · 2018-04-11T06:00:23Z

src/dmd/func.d

 /***********************************************************
 */
+
+extern (C++) struct Ensure


Wouldn't this be better as class EnsureStatement : Statement? Then it can go in the statement module.

It is not really a statement, it's just a way to create a tuple of result identifier and out contract.
I can of course declare it as a statement if that is better. (But I don't think it is.)

ibuclaw · 2018-04-11T06:02:07Z

src/dmd/func.d

    const(char)* mangleString;          /// mangled symbol created from mangleExact()

-    Identifier outId;                   /// identifier for out statement
+    @property Identifier outId(){       /// identifier for result


This would be dead code now, so remove?

It is not dead code, but I can turn it into a more appropriately-named bool function.

ibuclaw · 2018-04-11T06:04:20Z

src/dmd/parse.d

            }

        case TOK.in_:
+            auto loc = token.loc;


Add a couple comments presenting the supported in syntaxes as per invariant above.

ibuclaw · 2018-04-11T06:05:34Z

src/dmd/parse.d


        case TOK.out_:
            // parse: out (identifier) { statement }
+            auto loc = token.loc;


Likewise here, add comments for syntaxes that are now handled.

ibuclaw · 2018-04-11T06:06:08Z

src/dmd/statement.d

        assert(0);
    }

+    static Statements* arraySyntaxCopy(Statements* a){


ibuclaw · 2018-04-11T06:13:07Z

I think this could be ironed out within a week ready for inclusion.

ibuclaw · 2018-04-11T20:54:24Z

src/dmd/statement.d

+    /*************************************
+     * Do syntax copy of an array of Statement's.
+     */
+    static Statements* arraySyntaxCopy(Statements* a){


Curly braces on next line, check for this style everywhere.

ibuclaw · 2018-04-11T20:56:57Z

src/dmd/func.d

+        if (a)
+        {
+            b = a.copy();
+            foreach(i,e;*a)


Add spaces in foreach (i, e; *a) - check for this style everywhere as I've seen it more than once.

ibuclaw · 2018-04-11T21:12:21Z

src/dmd/func.d

+     *     `true` if the function has a return type that
+     *     is different from `void`.
+     */
+    final bool canBuildResultVar(){


I see that this is only used by the module its declared in. So make it private, and extern(D).

tgehr · 2018-04-24T09:14:20Z

@JinShil Probably not, but I can delete code. (I think most of what is not covered in hdrgen it is currently dead code.)

JinShil · 2018-04-24T09:27:10Z

I'm referring to coverage within this PR itself. The hdrgen code that isn't covered is new code in this PR. See https://codecov.io/gh/dlang/dmd/compare/b4d5005db0fd3581789d8a0942583f7c13efbc82...772309cbccb1c0d3e3cd9892f1ebc36024e5f7b7/diff

Ideally, we should have 100% coverage for all code in this PR.

tgehr · 2018-04-24T09:28:58Z

So am I. I know.

JinShil · 2018-04-24T09:30:44Z

If its dead code, why was it added?

tgehr · 2018-04-24T09:32:11Z

There is an obvious way to handle the case other than an assertion failure. But if coverage is more important than robustness to changes to invariants in other parts of the code, I can remove that.

JinShil · 2018-04-24T09:37:37Z

Sorry, I don't understand what you're implying. I think all that is required is a test to generate a .di file and verify that the result is correct. That should exercise the new code in hdrgen.d

tgehr · 2018-04-24T09:40:43Z

Currently, a contract is always either a ScopeStatement or an AssertStatement. The header generation code also handles the case when it is a different statement. But yes, you are right, the AssertStatement case can still be covered. I'll add a test and delete the dead code.

tgehr · 2018-05-04T17:26:29Z

@JinShil @ibuclaw @klickverbot @RazvanN7 @WalterBright Gentle reminder. (The remaining few lines of uncovered code cannot be covered in an end-to-end test as far as I can tell, but they should be there for completeness.)

andralex

Cool, thanks! Please follow with a spec PR.

andralex · 2018-05-05T15:35:42Z

cc @ibuclaw

wilzbach · 2018-05-05T15:36:29Z

@andralex FYI: there's already a PR to dlang.org: dlang/dlang.org#2339

wilzbach · 2018-05-04T02:00:23Z

changelog/expression-based_contract_syntax.dd

@@ -0,0 +1,22 @@
+Implement DIP 1009 - $(LINK2 https://github.com/dlang/DIPs/blob/master/DIPs/accepted/DIP1009.md, Add Expression-Based Contract Syntax)


The title is already a link. Thus it can't contain links.

ibuclaw

🙈

tgehr requested review from RazvanN7, WalterBright, dnadlinger and ibuclaw as code owners April 9, 2018 19:00

tgehr force-pushed the contract-syntax branch 2 times, most recently from c306ff4 to 357bfe8 Compare April 9, 2018 19:35

wilzbach mentioned this pull request Apr 9, 2018

dub run <package> should search the registry on failure dlang/dub#1428

Merged

wilzbach added Review:Needs Changelog A changelog entry needs to be added to /changelog Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org labels Apr 9, 2018

tgehr force-pushed the contract-syntax branch 2 times, most recently from c70941f to 34e6850 Compare April 9, 2018 20:23

thewilsonator reviewed Apr 11, 2018

View reviewed changes

ibuclaw requested changes Apr 11, 2018

View reviewed changes

tgehr force-pushed the contract-syntax branch 3 times, most recently from 57c8c90 to 0491d4c Compare April 11, 2018 18:27

dlang-bot added the Severity:Bug Fix label Apr 11, 2018

tgehr force-pushed the contract-syntax branch from 0491d4c to 9d8c577 Compare April 11, 2018 18:31

ibuclaw reviewed Apr 11, 2018

View reviewed changes

tgehr force-pushed the contract-syntax branch 8 times, most recently from 852982c to db54028 Compare April 17, 2018 13:39

tgehr force-pushed the contract-syntax branch 2 times, most recently from 7c09f5b to 312d16f Compare May 2, 2018 10:48

tgehr added 6 commits May 2, 2018 15:05

Implement new contract syntax.

067e5d6

Support for multiple contracts.

b0e4782

Implement new syntax for invariants.

0c001d8

Use new contract syntax for header generation.

144630e

Workaround for bug in older DMD frontends.

59b669d

Add changelog entry.

85cf246

tgehr force-pushed the contract-syntax branch 3 times, most recently from 1c556ea to 34b6986 Compare May 2, 2018 13:31

Full code coverage.

1a9de07

tgehr force-pushed the contract-syntax branch from 34b6986 to 1a9de07 Compare May 2, 2018 14:36

RazvanN7 approved these changes May 5, 2018

View reviewed changes

andralex approved these changes May 5, 2018

View reviewed changes

wilzbach reviewed May 5, 2018

View reviewed changes

ibuclaw approved these changes May 5, 2018

View reviewed changes

Fix link

23ee1c7

andralex added the Merge:auto-merge label May 5, 2018

dlang-bot merged commit 09d3845 into dlang:master May 5, 2018


		typedef Array<class TemplateInstance *> TemplateInstances;

		typedef Array<class Ensure> Ensures;

		@@ -0,0 +1,22 @@
		Implement DIP 1009 - $(LINK2 https://github.com/dlang/DIPs/blob/master/DIPs/accepted/DIP1009.md, Add Expression-Based Contract Syntax)

Uh oh!

Conversation

tgehr commented Apr 9, 2018

Uh oh!

dlang-bot commented Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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

tgehr Apr 11, 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!

Choose a reason for hiding this comment

Uh oh!

ibuclaw commented Apr 11, 2018

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!

tgehr commented Apr 24, 2018

Uh oh!

JinShil commented Apr 24, 2018

Uh oh!

tgehr commented Apr 24, 2018

Uh oh!

JinShil commented Apr 24, 2018

Uh oh!

tgehr commented Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JinShil commented Apr 24, 2018

Uh oh!

tgehr commented Apr 24, 2018

Uh oh!

tgehr commented May 4, 2018

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

andralex commented May 5, 2018

Uh oh!

wilzbach commented May 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

dlang-bot commented Apr 9, 2018 •

edited

Loading

tgehr Apr 11, 2018 •

edited

Loading

tgehr commented Apr 24, 2018 •

edited

Loading