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
don't use @trusted functions nested in @safe functions #3077
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request, @WalterBright! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. |
This should probably specify "non- |
What is this "experience"? Immediately invoked |
@RazvanN7 this needs to be fixed in order to be technically correct according to non-static nested function behaviour and the rationale presented in this best practice. |
@ljmf00 Yes, that is why I did not merge it already. |
Where is the DIP for this? |
I don't think a "Best Practice" need a DIP. |
I don't think this suggestion is very well-motivated, though, particularly as it goes against what some others (including myself) would consider best practice for templated code. |
The growing consensus is that system code should only be done in This is being hammered out in a DIP, to which will hopefully show up soon. |
Well, that's then definitely a change of a magnitude that should be well-researched and motivated (i.e. go through the DIP process). (If I understand your point correctly, |
Yes, and because of that, it is far too easy to whitewash your code with This thread provides some additional context. https://forum.dlang.org/thread/dmfoucfvatrzzgpzxoza@forum.dlang.org |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one place where you do not want to obey this: high-quality templates. There are often cases where you want to do something @trusted
, but if you marked the whole function that way it'd also be @trusted
when the function uses unsafe member function from types it's instanced with. And trying to design an internal @safe
interface for a library like Phobos to avoid the @trusted
nested function pattern would like double the code size.
I'm afraid that this advice in the spec would mislead Phobos / Vibe.D / Mir developers. Please be more specific.
Again, the plan is for |
Illustration of the problem: import std.range;
auto right(Range)(Range r)
if(isInputRange!Range && is(ElementType!Range : int))
{ import core.stdc.stdlib;
auto ptr = (() @trusted => malloc(10))();
scope(exit) () @trusted {ptr.free;}();
int result;
foreach(e; r)
{ /+ some calculations using mallocated buffer... +/
}
return result;
}
auto wrong(Range)(Range r) @trusted
if(isInputRange!Range && is(ElementType!Range : int))
{ import core.stdc.stdlib;
auto ptr = malloc(10);
scope(exit) ptr.free;
int result;
foreach(e; r)
{ /+ some calculations using mallocated buffer... +/
}
return result;
}
struct UnsafeRange
{ private int* ptr;
ref front(){return *ptr;}
auto popFront(){ptr++;}
auto empty(){return *ptr == 0;}
}
@safe void main()
{ int goodThisCompiles = [1,2,3,4].right;
int goodAlsoCompiles = [1,2,3,4].wrong;
//int goodDoesNotCompile = UnsafeRange(new int(5)).right;
int uhOh = UnsafeRange(new int(5)).wrong;
} |
Okay, this works, albeit it causes a lot of need to migrate code with relatively little benefit. But this is not a very public plan yet - It's the first time I even heard of it, and it isn't accepted yet. Having an advice like the one discussed given on spec is a bit early IMO. |
That code is invalid. You're effectively just marking There is obviously a desire (maybe a need) to use |
This is a problem with the language spec, not the code. An immediately-invoked (The same logic applies to all nested functions: since all of their call sites are known at compile time, the memory safety of those call sites can be proven by exhaustion.) |
That's fine. But someone has to do the work and write that down in the spec (and get it past Walter). Before then, the language doesn't allow it. The rules for Also, I'm not convinced that treating |
You may well be right with regular functions, perhaps even with your average template. But for Phobos, or other such high-quality template libraries, the alternatives are worse: 1: no |
You forgot the one true alternative even though you quoted it: A new, safer You don't like |
In the long run I agree. I'd hope for a more backwards-compatible change than the one they're currently planning, but otherwise the system block idea sounds good. But since it's still just a plan, I think in the short run it's just best to say that the safe interface requirement applies only for non-local symbols. That's how And Walter is backing this PR up with experience, not with spec compliance. This is why I don't want this change to be merged in the present form, at least not before the |
I'm afraid that the devil is in the details, and even a quick fix would need significant effort to make any sense. That effort would be better spent on the long run. But I wouldn't mind seeing a spec PR that proves me wrong. If allowing the current usage of
I think I get what you're saying: This "best practice" is basically telling everyone that they're doing But as far as I'm concerned, we're already there. The spec already forbids all the nested |
I agree, and in fact I am working on writing up an alternative formulation of the rules which handles both nested and non-nested functions sensibly. I'll post something on the forums when I have a complete draft ready. |
Yes, that is roughly what I meant.
I think you are right about that, unfortunately.
Hmm. It's rarely bad to advocate for spec compliance in best practices. But it increases the risk that people mark their templates So as dirty as the conclusion is, the spec must advocate against itself here, not for. Until Paul's changes and/or |
Experience with this has led to the conclusion it is not a good practice.