Skip to content
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.

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

Fix Issue 16492 - support @nogc in debug{} blocks #7882

Merged
merged 1 commit into from
Feb 14, 2018

Conversation

wilzbach
Copy link
Member

@wilzbach wilzbach commented Feb 13, 2018

debug already allows to use non-pure within pure scopes, imho it only makes sense to do the same for the rest.
If this gets approved, I am happy to do the same for @safe and nothrow.

Spec PR: dlang/dlang.org#2205

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

Bugzilla references

Auto-close Bugzilla Severity Description
16492 enhancement support @nogc in debug{} blocks

@TurkeyMan
Copy link
Contributor

Oh thank god!

auto b = [1, 2, 3];
b ~= 4;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This can be merged with other compilable tests. Merging them makes the autotester run faster.

src/dmd/nogc.d Outdated
if (e && e.op != TOK.error && f && sc.intypeof != 1 && !(sc.flags & SCOPE.ctfe)
&& (f.type.ty == Tfunction
&& (cast(TypeFunction)f.type).isnogc || (f.flags & FUNCFLAG.nogcInprocess) || global.params.vgc)
&& !(sc.flags & SCOPE.debug_))
Copy link
Member

@WalterBright WalterBright Feb 13, 2018

Choose a reason for hiding this comment

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

The code is good, but you're inventing another style of doing multiline conditionals.

if (e && e.op != TOK.error && f && sc.intypeof != 1 && !(sc.flags & SCOPE.ctfe) &&
    (f.type.ty == Tfunction &&
      (cast(TypeFunction)f.type).isnogc || (f.flags & FUNCFLAG.nogcInprocess) || global.params.vgc) &&
    !(sc.flags & SCOPE.debug_))

I.e. the dangling operator goes at the end, not the beginning, and subsequent lines should line up with the character after the opening (.

Copy link
Member

@WalterBright WalterBright left a comment

Choose a reason for hiding this comment

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

as already commented

@wilzbach
Copy link
Member Author

This can be merged with other compilable tests. Merging them makes the autotester run faster.

I forgot to use ARGS_SETS. The test needs to be run at least once with -debug and once without.

@wilzbach wilzbach force-pushed the fix-16492 branch 3 times, most recently from 8589415 to 25c503e Compare February 13, 2018 00:48
@JinShil
Copy link
Contributor

JinShil commented Feb 13, 2018

This feature leaves me feeling a little apprehensive. Is this truly in harmony with the intent of the debug statement, or are we looking to expedite a workaround to avoid the hard work of fixing some greater problem? Would this be an issue if our libraries and built-in types were less dependent on the GC? @andralex has hinted he and his staff are working on some revolutionary feature to be released soon™ that may remove reliance on the GC. Will that obsolete the need for this feature?

Note, I'm not against this, but I want to be sure there is deliberate intent behind it.

@timotheecour
Copy link
Contributor

timotheecour commented Feb 13, 2018

Wether it's debug or @!nogc or @gc we need an escape hatch at least for debugging. Trying to debug code inside @nogc @safe @nothrow is a pain otherwise.

@wilzbach
Copy link
Member Author

wilzbach commented Feb 13, 2018

There's a precendent:

As a concession to practicality, a pure function can also:
...
perform impure operations in statements that are in a ConditionalStatement controlled by a DebugCondition.

https://dlang.org/spec/function.html#pure-functions

debug shouldn't only be an escape hatch for pure, but for "turtles all the way down"

@marler8997
Copy link
Contributor

I forgot to use ARGS_SETS. The test needs to be run at least once with -debug and once without.

Ah...looks like that new option comes in handy :)

@TurkeyMan
Copy link
Contributor

@JinShil: writeln()

@dkgroot
Copy link
Contributor

dkgroot commented Feb 13, 2018

@wilzbach : '-debug' is a global compile flag, but @nogc / @pure are function local.

IMHO This could create some potentially unexpected results: Let's say you want to debug function X, so you switch on -debug, and now suddently function y.y.y.y (potentially in some library), suddenly could have (impure) side effects, require gc etc.
Switching on/off debugging should have as little as possible change in runtime, behaviour and hopefully no side effects.

@timotheecour
Copy link
Contributor

timotheecour commented Feb 13, 2018

This also worries me. Oftentimes you want just a local effect since you're just debugging one function.

Here's how nim does that:

https://nim-lang.org/docs/manual.html#pragmas-push-and-pop-pragmas

#here's an example with a particular compile option (`checks:off`) being locally applied
{.push checks: off.}
# compile this section without runtime checks as it is speed critical
# ... some code ...
{.pop.} # restore old settings

in D could look something like:

pragma(push, "-debug"); // could push other stuff eg "-noboundscheck, -O, -inline" etc
// compile this section with `-debug`
// ... some code ...
pragma(pop); // restore old settings

or perhaps more idiomatically, taking advantage of scoping rules of pragmas (3 types: single decl, all following decls, or inside {})

// could push other stuff eg "-noboundscheck, -O, -inline" etc
pragma(compilerOptions, "-debug"){
  // compile this section with `-debug`
  // ... some code ...
}
// old settings restored

@wilzbach
Copy link
Member Author

This could create some potentially unexpected results:
suddenly could have (impure) side effect

This has been part of the DMD compiler and DSpec for years for pure! It's even in Andrei's TDPL.
No one complained, raised concerns or even opened a Bugzilla issue about it - the issues that were opened are that you can't use this feature to escape other attributes - it pops up every three to six minutes late months. A quick excerpt - the list is longer:

debug already helps debugging, just not for all attributes.

@wilzbach
Copy link
Member Author

in D could look something like:

This is already possible today. You can trick the compiler into thinking something is @nogc:

void assumeNogc(alias Func, T...)(T xs) @nogc {
	import std.traits;
	static auto assumeNogcPtr(T)(T f) if (
		isFunctionPointer!T || isDelegate!T
	) {
  	    enum attrs = functionAttributes!T | FunctionAttribute.nogc;
    	    return cast(SetFunctionAttributes!(T, functionLinkage!T, attrs)) f;
	};
	assumeNogcPtr(&Func!T)(xs);
};

@dkgroot
Copy link
Contributor

dkgroot commented Feb 13, 2018

@timotheecour Do really like you potential solution, would allow for nicely localized debugging !
@wilzbach If it is the status quo, so be it.
Side effects are sometimes difficult to detect and doubly so, if they are caused by debugging. So it might take a while :-)

@TurkeyMan
Copy link
Contributor

@dkgroot

Switching on/off debugging should have as little as possible change in runtime, behaviour and hopefully no side effects.

I tend to expect switching on/off debug have enormous impact on runtime (ie, enable/disable all non-shipping debugging infrastructure; high runtime cost), and likely have loads of 'side effects' (like emitting data to logs, telemetry, etc.)

Right now, it's impossible to debug pure/@nogc/etc code. This patch is necessary.

@dkgroot
Copy link
Contributor

dkgroot commented Feb 14, 2018

@TurkeyMan I meant negative/unpredictable side effects. When debugging your application, it would be nice if you could finetune your debug only to the part that you are actually interested in, ie the part you are trying to debug. That's why i did like @timotheecour solution so much.
Note: I am not against this PR at all, just wanted to raise a potential issue that should be considered before accepting it.

@WalterBright WalterBright dismissed their stale review February 14, 2018 08:54

It has unique switches

@WalterBright
Copy link
Member

I pulled this because this solution is simple. The casting solution is just too awful, and the push/pop is just annoying. When I write debug code, I don't care about correctness, I just need to do some logging or extra checking, and the compiler should relax its usual strict pedantic checking for it. If you ship code with debug on, well, that's your choice and your problem :-)

@WalterBright WalterBright merged commit 5694053 into dlang:master Feb 14, 2018
@JinShil
Copy link
Contributor

JinShil commented Feb 14, 2018

Well, that addresses my concerns 😆

UPDATE: I think that comment could easily be misinterpreted. What I mean by the above comment is @WalterBright addressed my concerns completely and definitively in a single K-Pow! I was wondering about the intent behind debug {} and the inventor of the language made it abundantly clear.

@wilzbach wilzbach deleted the fix-16492 branch February 14, 2018 12:59
@TurkeyMan
Copy link
Contributor

Follow up for the other attributes expressing the same problem?

@wilzbach
Copy link
Member Author

Yes, but they are more complicated.
In the mean time there's https://issues.dlang.org/show_bug.cgi?id=18407, s.t. this doesn't get lost.

@andralex
Copy link
Member

@TurkeyMan what other attributes would be left?

cc @RazvanN7

@wilzbach
Copy link
Member Author

@TurkeyMan what other attributes would be left?

nothrow and @safe

@WalterBright
Copy link
Member

@JinShil thanks for the clarification. Think of debug like granting root privileges, or like attaching a symbolic debugger to the program. Debugging may need such access.

@JackStouffer
Copy link
Member

Oh man, thank you for pulling this an working on the other attributes. This is my biggest pain point on D.

I've been doing this in my projects, and I'm looking forward to deleting this cruft

version (noAttributes)
{
    unittest
    {
        runTests();
    }
}
else
{
    @safe pure unittest
    {
        runTests();
    }
}

@PetarKirov
Copy link
Member

@wilzbach awesome! Can you add a changelog and update the spec pls?

@wilzbach
Copy link
Member Author

@wilzbach awesome! Can you add a changelog and

It technically already appears on the changelog, but I doubt anyone will notice:

image

I think it makes sense to wait with a proper changelog entry until nothrow and @safe are done too, because otherwise the question "why not @safe and nothrow" will immediately come up.

update the spec pls?

Spec is already updated. See dlang/dlang.org#2205 and dlang/dlang.org#2209

@PetarKirov
Copy link
Member

Thanks Seb, I saw those PRs later than this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.