Implement DIP1006 - more selective control over run-time checks #7980

MartinNowak · 2018-03-03T16:24:12Z

allow to disable only some run-time checks e.g. with -release=in,out,invariant
cli was proposed in DIP1006 review discussion
https://forum.dlang.org/post/rsafosvkhxddkxptaziy@forum.dlang.org
but hasn't yet been added to https://github.com/dlang/DIPs/blob/master/DIPs/DIP1006.md

dlang-bot · 2018-03-03T16:24:14Z

Thanks for your pull request, @MartinNowak!

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#7980"

MartinNowak · 2018-03-03T16:43:58Z

Regarding -release=body,in,out,invariant vs. -release=assert,in,out,invariant, see https://forum.dlang.org/post/zgybhumcwrxmejjntemp@forum.dlang.org.
There is no fine-grained switch to disable asserts only in some contexts and it would be quite some effort to add that, so -release=assert still runs contracts and invariants, but disables assertions within those. That doesn't seem too big a use-case and some replacement for assert can be used (e.g. enforce). So I don't think it warrants the complexity to enable assert dependening on contexts.

WalterBright · 2018-03-03T23:17:10Z

cli was proposed in DIP1006 review discussion
https://forum.dlang.org/post/rsafosvkhxddkxptaziy@forum.dlang.org

All on one page: https://digitalmars.com/d/archives/digitalmars/D/DIP_1006_-_Preliminary_Review_Round_1_299766.html

WalterBright · 2018-03-03T23:22:20Z

I appreciate the quick implementation of this, but it does not match the DIP, and one or the other needs to be changed.

Geod24

First, thanks for taking care of this @MartinNowak !
I made a few comments.

I don't care much about the name itself, and as they stand they look good enough to me. However, as I mentioned a few times, I strongly feel that disabling assert should disable in/out/invariant, because if assert are disabled, those code blocks are useless, and IMO it would be confusing to have them on.

Geod24 · 2018-03-04T19:12:11Z

src/dmd/mars.d

+                    auto tail = arg["-release=".length .. $];
+                    while (true)
+                    {
+                        auto delim = strchr(tail.ptr, ',');


I thought we were not supporting CLI with comma ? After #7863

I'm not familiar with the approved CLI interface for DIP1006, whether commas were approved. If commas were approved then we should establish a convention on when and when not to support commas, and then use a common implementation for comma support. If it were approved, I would think it would also apply to options like -i and -version...but this is all moot if commas are not approved

I picked commas because it allows to override previous occurences of that argument, sometimes useful when using other peoples projects and Makefiles. Less of an argument for switches like include paths that are expected to be additive.

Geod24 · 2018-03-04T19:14:48Z

src/dmd/mars.d

+
+                // switch without default is now a compile-time error anyhow
+                if (params.useSwitchError == CHECKENABLE._default)
+                    params.useSwitchError = CHECKENABLE.off;


Do I understand correctly that those 2 get affected even if I just provide -release=invariant ? That sounds odd.

Switch fallthrough errors are dead, they're a compile-time error nowadays.

Geod24 · 2018-03-04T19:18:24Z

changelog/dip1006.dd

@@ -0,0 +1,4 @@
+allow to disable only specific run-time checks


Should this be capitalized ? ("Allow")

Apparently yes

WebDrake · 2018-03-05T19:01:50Z

@WalterBright

I appreciate the quick implementation of this, but it does not match the DIP, and one or the other needs to be changed.

Just to note: I would have written up a DIP for the -release=... idea, but was advised to wait for a formal decision on DIP1006: https://forum.dlang.org/post/cudgblkodvsagcrtdpbu@forum.dlang.org

The major advantage of this approach over DIP1006 is that the options presented are not hierarchical, so it gives more control to the developer over how it can be used.

@Geod24

I strongly feel that disabling assert should disable in/out/invariant, because if assert are disabled, those code blocks are useless

This isn't true. We don't know, and can't know, what people are doing inside their contracts (which might still be useful even with assert disabled). @MartinNowak even mentions a use-case.

It's also simpler to understand if -release=assert just disables assert calls, rather than contracts and invariants.

WebDrake · 2018-03-05T19:08:00Z

@MartinNowak

Regarding -release=body,in,out,invariant vs. -release=assert,in,out,invariant, see https://forum.dlang.org/post/zgybhumcwrxmejjntemp@forum.dlang.org.

This is a shame, because one very nice aspect to -release=body,in,out,invariant is that it keeps the sub-options completely non-overlapping.

Acknowledging that it would be quite some effort, what do you see as the main difficulties/obstacles in implementing the fine-grained switch?

Geod24 · 2018-03-05T21:17:25Z

This isn't true. We don't know, and can't know, what people are doing inside their contracts

Someone can use assert to check user input, it doesn't make it any more valid.

@MartinNowak : Have you actually seen someone throwing exceptions from invariants / contracts ?

WebDrake · 2018-03-05T21:29:53Z

Have you actually seen someone throwing exceptions from invariants / contracts ?

Who said anything about throwing exceptions? Using enforce != throwing exceptions.

MartinNowak · 2018-03-06T20:08:13Z

@MartinNowak even mentions a use-case.

It's also simpler to understand if -release=assert just disables assert calls, rather than contracts and invariants.

We have a few D users that are extensively using contracts and might want to do so even in a production environment.

This is a shame, because one very nice aspect to -release=body,in,out,invariant is that it keeps the sub-options completely non-overlapping.

Acknowledging that it would be quite some effort, what do you see as the main difficulties/obstacles in implementing the fine-grained switch?

In order to enable asserts in some contexts but keep it disabled in others, we'd have to correctly infer the current context, which is not too trivial given the existing complexity in contracts and considering language features such a nested functions and mixin templates.
For example we already distinguish _d_unittest_msg and _d_assert_msg, but the former only is called if the direct parent is a unittest (but not for asserts in nested functions, nested types, or nested/mixin templates). This is confusing as it breaks which is confusing since most of those are lexically contained in unittests.
If deemed necessary it's definitely sth. that could be implemented, but it'd be a lot more work to implement correctly.
Also -release=in,out,invariant would still need to disable contracts and invariants alltogether for performance reasons, not just disable asserts in those, so -release=body doesn't really fit that category.

I appreciate the quick implementation of this, but it does not match the DIP, and one or the other needs to be changed.

Yip, this PR was intended to push the discussion/design towards a decision dlang/DIPs#64 (comment). Let me mark it as WIP though.

MartinNowak · 2018-03-06T20:19:28Z

I strongly feel that disabling assert should disable in/out/invariant, because if assert are disabled, those code blocks are useless, and IMO it would be confusing to have them on

That might be another option, then we could just have -release=in,out,invariant and leave disabling any run-time checks to -release.
Let me try to get @lindt into the discussion as well, b/c Funkwerk is one of the biggest contract users. It's generally true though that contracts are supposed to be side-effect free and primarily use assertions.
Leaving the option to explicity enforce!AssertError(val >= 0) in contracts while disabling assertions, might be useful though.

WebDrake · 2018-03-06T21:02:35Z

That might be another option, then we could just have -release=in,out,invariant and leave disabling any run-time checks to -release.

That's a fair point, and one that occurred to me too: I didn't follow the difference between @Geod24's proposal for -release=assert and just the plain -release flag.

It would certainly be feasible (and reasonable) to start with just -release=in,out,invariant and leave other options for future discussion.

WebDrake · 2018-03-06T21:12:56Z

Also -release=in,out,invariant would still need to disable contracts and invariants alltogether for performance reasons, not just disable asserts in those, so -release=body doesn't really fit that category.

I'd have assumed that would be a simplifying factor, because in,out,invariant can just 100% strip out the relevant contract sections, so the context of an assert only needs to be a concern for body, and that could be any function body ... but I accept that could have some edge cases where nested functions are concerned (e.g. nested functions in unittests or in contracts).

Are these edge cases your primary concern or is there inherently a difficulty with checking if the context of an assert is inside a function body?

Anyway,

Leaving the option to explicity enforce!AssertError(val >= 0) in contracts while disabling assertions, might be useful though.

... this is what I assumed you had in mind when talking about enforce, and this is why I would rather not have a -release=assert option that also strips out contracts and invariants.

MartinNowak · 2018-03-06T22:35:38Z

in,out,invariant

Just adding -release=in,out,invariant seems to be an agreeable subset, so we could move forward with that and wait for a use-case to selectively disable assertions only in function bodies but not in contracts.

linkrope · 2018-03-07T12:33:01Z

Let me try to get @lindt into the discussion as well, b/c Funkwerk is one of the biggest contract users. It's generally true though that contracts are supposed to be side-effect free and primarily use assertions.

In contracts, we only use assert. Our prime requirement is to keep all contracts enabled in production. We have a few invariants, which are too slow. We turn them off selectively with version conditions.

It could be helpful, however, to keep only in contracts enabled in production. Violating in contracts is a major source of issues.

So, adding -release=in,out,invariant would be great!

mathias-lang-sociomantic · 2018-03-07T12:56:45Z

-release=in,out,invariant (or probably -release=in -release=out -release=invariant) sounds good to me

WebDrake · 2018-03-07T20:14:17Z

Great -- glad we are able to come to a consensus here, as this is a nice feature to be able to move forward with.

wilzbach · 2018-05-07T20:12:52Z

Just adding -release=in,out,invariant seems to be an agreeable subset,

FWIW Walter previously objected to adding a comma parser as the 80 lines needed for its generic support for all arguments "wasn't justified" :/
See #7863
(though I still hope that we might be able to re-evaluate this decision)

- allow to disable only some run-time checks e.g. with -release=in,out,invariant - cli was proposed in DIP1006 review discussion https://forum.dlang.org/post/rsafosvkhxddkxptaziy@forum.dlang.org but hasn't yet been added to https://github.com/dlang/DIPs/blob/master/DIPs/DIP1006.md

wilzbach

It's really a shame that such good work dies in the PR queue. Also the DIP has been submitted more than a year ago and is still in "the final review" stage.
In my viewpoint this helps a lot of users and apparently @andralex and @WalterBright have not a huge interest in this. So I'm ready to pull this.
-> I added the "72h no objections -> merge".
If you have any final concerns, you should speak now!

leandro-lucarella-sociomantic · 2018-07-02T10:19:59Z

This is on me now, I've been contacted a month or so to take over the DIP from @Geod24 's hands and we are talking in private with W&A for the final review and they are requesting changes to accept it (basically to use -version=invariant / etc. to control this). We are still negotiating the details and seeing if it would work.

Sorry for the delay.

thewilsonator · 2018-12-24T14:04:59Z

@MartinNowak why close this?

leandro-lucarella-sociomantic · 2019-01-04T15:50:36Z

Probably because of #8972.

MartinNowak requested review from WalterBright, mathias-lang-sociomantic and rainers as code owners March 3, 2018 16:24

MartinNowak force-pushed the dip1006 branch from 2b8923a to bad68d6 Compare March 3, 2018 16:40

MartinNowak force-pushed the dip1006 branch from bad68d6 to e2b9435 Compare March 3, 2018 16:46

MartinNowak mentioned this pull request Mar 3, 2018

DIP1006: Apply feedback from review thread dlang/DIPs#64

Merged

Geod24 reviewed Mar 4, 2018

View reviewed changes

MartinNowak changed the title ~~implement DIP1006 - more selective control over run-time checks~~ [WIP] implement DIP1006 - more selective control over run-time checks Mar 6, 2018

dlang-bot added the Review:WIP Work In Progress - not ready for review or pulling label Mar 6, 2018

MartinNowak mentioned this pull request Mar 6, 2018

enable assertions in CI tests #7344

Closed

wilzbach mentioned this pull request Apr 5, 2018

Point users to Bugzilla on ICEs #8138

Merged

wilzbach force-pushed the dip1006 branch from e2b9435 to e1b10dd Compare July 2, 2018 00:21

Changelog improvements

2a487c2

wilzbach approved these changes Jul 2, 2018

View reviewed changes

wilzbach removed the Review:WIP Work In Progress - not ready for review or pulling label Jul 2, 2018

wilzbach changed the title ~~[WIP] implement DIP1006 - more selective control over run-time checks~~ Implement DIP1006 - more selective control over run-time checks Jul 2, 2018

wilzbach added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Jul 2, 2018

leandro-lucarella-sociomantic added Review:Needs Approval and removed Merge:72h no objection -> merge The PR will be merged if there are no objections raised. labels Jul 2, 2018

MartinNowak mentioned this pull request Jul 8, 2018

Build DMD, Druntime + Phobos in debug mode dlang/ci#185

Open

wilzbach mentioned this pull request Nov 19, 2018

add -check command line switch #8972

Merged

MartinNowak closed this Dec 24, 2018

		@@ -0,0 +1,4 @@
		allow to disable only specific run-time checks

Uh oh!

Implement DIP1006 - more selective control over run-time checks #7980

Implement DIP1006 - more selective control over run-time checks #7980

Uh oh!

Conversation

MartinNowak commented Mar 3, 2018

Uh oh!

dlang-bot commented Mar 3, 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!

MartinNowak commented Mar 3, 2018

Uh oh!

WalterBright commented Mar 3, 2018

Uh oh!

WalterBright commented Mar 3, 2018

Uh oh!

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

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!

WebDrake commented Mar 5, 2018

Uh oh!

WebDrake commented Mar 5, 2018

Uh oh!

Geod24 commented Mar 5, 2018

Uh oh!

WebDrake commented Mar 5, 2018

Uh oh!

MartinNowak commented Mar 6, 2018

Uh oh!

MartinNowak commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WebDrake commented Mar 6, 2018

Uh oh!

WebDrake commented Mar 6, 2018

Uh oh!

MartinNowak commented Mar 6, 2018

Uh oh!

linkrope commented Mar 7, 2018

Uh oh!

mathias-lang-sociomantic commented Mar 7, 2018

Uh oh!

WebDrake commented Mar 7, 2018

Uh oh!

wilzbach commented May 7, 2018

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

leandro-lucarella-sociomantic commented Jul 2, 2018

Uh oh!

thewilsonator commented Dec 24, 2018

Uh oh!

leandro-lucarella-sociomantic commented Jan 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

dlang-bot commented Mar 3, 2018 •

edited

Loading

MartinNowak commented Mar 6, 2018 •

edited

Loading