Fix for issue# 8362.

[jmdavis]

"std.traits.isSafe doesn't work with unsafe lamdba functions"

std.traits.isSafe seems to have been fairly broken. It's first branch was done completely incorrectly and showed a lack of understanding of how FunctionAttribute works, and the second half didn't take into account that the function wouldn't compile if the argument was @system. Templatizing the function that it used to test would have fixed it, but I just fixed the first part so that it should work for everything.

The original problem was that something like

import std.traits;

struct S{ auto fun() { return 1; } }

void main()
{
  auto s1 = S();
  static assert(!isSafe!({auto r = s1.fun();}));
}

didn't compile, since the latter half of isSafe didn't work with @system at all.

[alexrp]

The changes look reasonable to me.

[andralex]

Not a fan of the fact that we have @safe and @trusted but isSafe is a disjunction of them (instead of isSafe meaning safe and isTrusted meaning trusted). Could we change things such that we have isSafe, isTrusted, and isSafelyCallable? The latter would implement the functionality of the currently proposed isSafe.

[jmdavis]

I'd argue against it based on the fact that it could break code except for the fact that isSafe is pretty much completely broken right now anyway, and I tend to agree with your dislike for isSafe evaluating to true for @trusted as well. So, I'll make the changes.

[jmdavis]

On the downside, the whole naming scheme was built around the idea that isSafe would be true for @trusted: isSafe, isUnsafe, areAllSafe, and for the most part, you only care whether something is @system or not... So, I don't know. Having isX and areAllX for all of them seems a bit much. Maybe I can up with a good templatization scheme using enums or something. I'll have to think about this.

[dnadlinger]

Actually, I'm not so sure if we should change this in the first place. There is no valid reason whatsoever for a caller to check if a function is @safe or @trusted. Introducing this primitives just makes it easier for users to write wrong code.

Additionally, if @trusted goes away in the future – I hope it is changed to just annotate the function @safe and mark all of its body @trusted –, having those two templates, which would probably be changed to mean the same thing, would be confusing.

[dnadlinger]

Also, not introducing isSafe and isTrusted would provide a clean migration path to the fixed implementation and naming: Just schedule the isSafe and areAllSafe for deprecation, while adding isSafelyCallable as a replacement.

[dnadlinger]

Your change doesn't just fix isSafe, it changes its semantics (as requested by Andrei, but that doesn't make it less of a breaking change). This is why I think we should be clear about what we are doing…

[jmdavis]

Your change doesn't just fix isSafe, it changes its semantics

I never said otherwise.

This is why I think we should be clear what we are doing…

I don't see changing isSafe as being a problem even with your proposal, since it would just end up doing what it does now anyway, since @trusted would be identical from the perspective of the function signature. The problem is that it introduces isSafelyCallable, which currently is better to use than isSafe due to how @safe and @trusted work but which becomes completely pointless if we go with your proposal. So, we'd end up adding a function, essentially telling you to use it instead of isSafe, then turn around to deprecate it and tell you to use isSafe again, because @trusted will now mean @safe as far as function signatures are concerned. That's the only real problem that I see here. But if we don't end up going with your proposal, then these changes are exactly what we want.

[dnadlinger]

The @trusted debate is one thing, but what I'm worried about here is that this a breaking change to isSafe for non-functions, for questionable benefits. We have recently been very careful about the way to handle breaking changes. Yes, isSafelyCallable is currently the right choice for virtually all applications. But there is probably code out there which uses isSafe…

[jmdavis]

What do you mean a breaking change for isSafe for non-functions? And any code which uses isSafe is inherently broken anyway, since isSafe has never worked.

[dnadlinger]

Okay, seems like I underestimated how broken isSafe currently is (or, well, was), but this compiles, and won't with the changes folded in:

import std.traits;

alias void function() @safe SafeFn;
static assert(isSafe!(SafeFn.init));
alias void function() @trusted TrustedFn;
static assert(isSafe!(TrustedFn.init));
alias void delegate() @safe SafeDg;
static assert(isSafe!(SafeDg.init));
alias void delegate() @trusted TrustedDg;
static assert(isSafe!(TrustedDg.init));

I just grepped through my sources, and apparently I have actually written code which uses isSafe in template constraints (and RefRange seems to use it as well, btw) – I must have been incredibly lucky, because I can't recall noticing any problems with it. Yes, my code probably works out of coincidence given the bugs in the isSafe implementation, but it is supposed to work given the Phobos docs.

Changing the two instance of isSafe to isSafelyCallable would be trivial, of course, but I'm not sure if it's worth introducing a breaking change, given that – and this is where the argument detailed in the forum thread comes in, regardless of whether the proposal is accepted or not – it doesn't really make sense to use isSafe and isTrusted in just about any situation. And if somebody really needs that information, they can always use FunctionAttributes directly.

By the way, SetFunctionAttributes actually distinguishes between @safe and @trusted for consistency, but this, i.e. pretty printing function types, is about the only use case for isSafe/isTrusted I can think of. And if you are doing that, chances are you are using all of the other FunctionAttributes anyway.

[andralex]

waiting

[andralex]

FWIW isSafelyCallable could be a possibility.

[jmdavis]

Okay. Updated. It has isSafe, isTrusted, isUnsafe, isSafelyCallable, areAllSafe, and areAllSafelyCallable, where the safely callable ones are true for both @trusted and @safe, and the others are only for exactly what their names say.

[dnadlinger]

areSafelyCallable(a, b, c) == allSatisfy!(isSafelyCallable, a, b, c)?

[jmdavis]

Ah. Good point. Will fix.

[jmdavis]

Okay. areAllSafelyCallable has been removed. I made it because that's essentially what allAreSafe was doing before I changed isSafe, but since we have allSatisfy, it is rather pointless - as is areAllSafe, but we can decide later whether we want to get rid of it.

[dnadlinger]

If we want to remove areAllSafe, we should do it right now (well, at the same time the pull request is merged, after 2.060), because its behavior would be silently changed anyway, which is arguably worse than removing it.

Besides, I can't imagine that much external code uses it in its broken state. And it isn't used in Phobos anymore either – originally, it was included as part of the std.datetime.benchmark implementation, but attribute inference is used for it now.

[jmdavis]

My concern was removing it in the middle of a beta, and this pull request may get pulled in as part of that in order to fix bug# 8450.

However, what we can do cleanly is change it back to really being the same as allSatisfy(isSafelyCallable, a, b, c) and schedule it for deprecation. That would probably be better. It's less disruptive and gets us started on fixing the problem.

[jmdavis]

Okay. areAllSafe is back to the way that it was and marked as scheduled for deprecation. So, this pull request should make isSafe, isTrusted, isUnsafe, and isSafelyCallable all do what you'd expect from their names and sets us up to get rid of the poorly named and unnecessary areAllSafe. The one downside to all of this is that isSafe doesn't do quite the same thing anymore, which could theoretically break code, but since it was completely broken before, it won't actually break any code. It just poses the risk that someone will have wanted isSafelyCallable and will continue to use isSafe without realizing that it changed (but I did add a note to the changelog, so the risk of that is reduced - though if this doesn't get pulled in as part of 2.060, this pull will have to be changed again, since the changelog entry will be in the wrong place).

[dnadlinger]

Hm, I'm still not sure if this is the most consistent solution, but I see your point. If we go with this, I'd explicitly state that the template never worked as advertised, to avoid »why on earth was it necessary to deprecate my perfectly good function«-type responses in the future.

[andralex]

@klickverbot I tend to see isSafe and isTrusted more like reflection primitives than things a caller would be interested in. They do the obvious thing of looking at the respective flags. I'll pull this in now.

[dnadlinger]

@andralex: We already have std.traits.FunctionAttributes. This was a breaking change for the cases where isSafe previously worked. I don't think pulling this right now was a good idea, regardless of what we do or don't do about @trusted.

Also, as I noted in the forum discussion, there is no semantic reason to look at which of both attributes a function bears. They are completely equivalent. Where would you use isTrusted?

[dnadlinger]

Since the regression count is falling quickly: Ping? Are we going to leave this in the current state?

To summarize, why I think merging the changes was not a good decision, but a mistake in the heat of trying to get a release out:

The change breaks code. The following snipped always compiled, and was supposed to according to the documentation. isSafe might not work correctly in some cases, but my code uses it, and it doesn't compile with latest master any longer. This is a silent breaking change/regression, without any deprecation phase, etc.:

import std.traits;

void safe() @safe;
void trusted() @trusted;
void system() @system;

struct Test
{
    void safe() @safe;
    void trusted() @trusted;
    void system() @system;
}

static assert(isSafe!(safe));
static assert(isSafe!(trusted));
static assert(!isSafe!(system));

Test t;
static assert(isSafe!(t.safe));
static assert(isSafe!(t.trusted));
static assert(!isSafe!(t.system));

isSafe and isTrusted are not needed for two reasons:

• The functionality is already covered by functionAttributes. Are you proposing to add isPure, isNothrow, isRef and isTrusted as well? isSafelyCallable resp. the current isSafe is an exception, because it is another abstraction above that (as long as my NG proposal is not implemented).
• Even if you currently can by using functionAttributes, there are no situations [1] in which you would want to distinguish between the two attributes. isSafelyCallable is always the right choice. Usually you are the one demanding real-world use cases when new features are proposed for Phobos, Andrei. ;)

Yes, isSafe might be an unfortunate name given the current @safe/@trusted situation, but is this really justification enough for throwing our usual guidelines w.r.t. deprecation over board?

———

[1] Besides pretty-printing a function type, as done e.g. in the SetFunctionAttributes implementation for string mixin codegen. But this is extremely, extremely rare, and merely an artifact of the current situation, not an argument in favor of it.

[jmdavis]

I would expect all actual code breakage to be caused by isSafe being fixed, not by it being changed to only being true for @safe. So, the breakage is caused by bug fix, which happens all the time.

What would you be using isSafe for other than deciding whether to mark something as @safe or @system, given that it doesn't actually tell you whether the function is @safe? If you cared specifically about it being @safe or @trusted, you'd need to have been using FunctionAttributes anyway, since isSafe didn't distinguish. Worst case, with the change, @trusted stuff ends up using the @system branch instead of the @safe one.

The only problem that I see with these changes is that if @trusted gets changed the way that you proposed, then isSafelyCallable becomes pointless.

[dnadlinger]

So, the breakage is caused by bug fix, which happens all the time.

No, the breakage is caused by arbitrarily changing the definition of isSafe. Fixing it would mean to implement it as described by the docs. If code breakage was caused by fixing, it would be acceptable, but this is not the case.

What would you be using isSafe for other than deciding whether to mark something as @safe or @system, given that it doesn't actually tell you whether the function is @safe?

This is exactly what I am using it for and what I would expect other people to use it for. How is this an argument for changing isSafe to not fit this use case anymore? Yes, it doesn't perfectly fit the name, but if this really bugs you, why not go through a full deprecation cycle and change it this way?

[jmdavis]

What would you be using isSafe for other than deciding whether to mark something as @safe or @System, given >> that it doesn't actually tell you whether the function is @safe?

This is exactly what I am using it for and what I would expect other people to use it for. How is this an argument for changing isSafe to not fit this use case anymore? Yes, it doesn't perfectly fit the name, but if this really bugs you, why not go through a full deprecation cycle and change it this way?

If that's what you're doing, I don't see how it breaks anything. It just makes it so that your functions get marked as @system instead of @safe if they were @trusted. A bit annoying perhaps, but I don't see how it would break code.

[jmdavis]

And as for a deprecation cycle, how would you do that? isSafe's behavior is being changed rather than replacing it.

We could introduce isSafelyCallable, schedule isSafe for deprecation, put it through the whole deprecation cycle, and then reintroduce it as it is in this pull request, but that would take at least a year with the normal deprecation cycle, and isSafe is completely broken as it is in 2.059, so I really don't see much problem in changing it as part of the fix. Any code using it is at least partially broken anyway.

[dnadlinger]

It just makes it so that your functions get marked as @System instead of @safe if they were @trusted. A bit annoying perhaps, but I don't see how it would break code.

Huh? The function in question couldn't be called by @safe callers anymore. How is this not a breaking change?

The second use case where I'm using isSafe is in a task pool-like class, which accepts delegates from user code to run on worker threads/fibers. Because its interface is @safe, it can only accept @safe or @trusted delegates to execute. Currently, I'm using isSafe there. If the change stays in, code which supplies @trusted delegates won't compile anymore.

[dnadlinger]

isSafe's behavior is being changed rather than replacing it.

And there is no reason for doing so. I'm still waiting for arguments why we should have isSafe (the new one) and isTrusted in the first place, even if we keep the @safe/@trusted distinction (which, by the way, nobody found an argument for this so far either).

Any code using it is at least partially broken anyway.

Sigh. You are right, code using isSafe probably doesn't accept all the callables it could, respectively misses that it could mark template functions @trusted.

But the code isn't broken. It used isSafe exactly as it was documented to work, and the application compiles fine because it only happens to only trigger cases which isSafe handles correctly.

I don't see how the current implementation being buggy is an excuse to screw all its users. For me as a Phobos user, this is just like if we discovered that using zip with StoppingPolicy.longest was broken, and then decided to change its behavior because »any code using it is at least partially broken anyway«…

[andralex]

@klickverbot Is this a huge deal? If not, let's just move on. The simple argument is, there's @trusted and there's @safe and then there's isTrusted and isSafe. Defining isSafe to really mean isSafeOrTrusted only confuses people.

[dnadlinger]

@andralex: My definition of »let's just move on« is to revert the change, in the sense of just changing isSafe to the implementation of isSafelyCallable in the pull request. This is the simple, obvious solution, given that we already have isSafe in Phobos.

I really don't want to waste any more time on this, but the change breaks (my) code, and I don't see how this should be a good idea. Yes, the name isSafe might be a bit unfortunate, but this isn't a reason to just change it on the spot. I mean, if it is, why haven't we renamed the known-to-be-tricky clear yet?

[andralex]

I think my underlying assumption here is that fewer people use isSafe than other facilities. Though I have a preference for keeping this pull request in, you are making a good argument that it is, after all, a breakage of existing code. I'll defer the decision to @WalterBright.

[jmdavis]

If we're going to change it, I think that we should just change it now. Otherwise, we just leave isSafe as it is and ditch isSafelyCallable. It'll be less painful to just switch it now for the relatively few people that use it rather than go through the whole deprecation process just to shuffle names around.

I'm surprised that anyone would have written code which required that something be @safe. I would have expected all such code to have an @safe and @system version. Requiring @safe would be seriously limiting. But this will break code which does that, though given how broken isSafe was, that code wasn't really working correctly in the first place.

[dnadlinger]

@andralex:

you are making a good argument that it is, after all, a breakage of existing code

I also explained why this breaking change doesn't gain us anything. If you are unhappy with the name, why don't we just add isSafelyCallable and schedule isSafe for deprecation?

Although I think it's somewhat unnecessary, I won't complain about that, name changes in Phobos do happen, and it's not worth the time bikeshedding about that.

[jmdavis]

The deprecation path works fine for renaming, replacing, or removing functions/templates, but it doesn't really work very well for changing what a function/template is doing.

[dnadlinger]

The deprecation path works fine for renaming, replacing, or removing functions/templates, but it doesn't really work very well for changing what a function/template is doing.

@jmdavis: Why should we want to change it in the first place? You've successfully been avoiding to answer that question for the whole discussion, even though I provided two arguments why we shouldn't.

[jmdavis]

Because the name is completely wrong. It effectively lies about what the template is doing.

[dnadlinger]

@jmdavis: This might be a reason to change the name, but not to add a new, completely unnecessary isSafe template. And if we don't add isSafe/isTrusted, the usual procedure for changing names is completely fine.

[jmdavis]

I don't see any problem with having isSafe and isTrusted which specifically tell you whether a function is @safe or @trusted. That's the sort of thing that std.traits is for, and it's a whale of a lot better than using FunctionAttributes to do it, especially when not even Phobos got that right (not to mention, it's way more verbose to use FunctionAttributes). Granted, in the vast majority of cases, isSafelyCallable is actually what people are going to want to use, but that doesn't mean that there's no point to isSafe.

The only real issue that I have here is the fact that if @trusted gets changed like you proposed (and I'd love to see happen), then adding isSafelyCallable was pointless, and everyone could have continued to just use isSafe, which would then be properly named, since no function would be @trusted anymore as far as the signature was concerned.

[dnadlinger]

Granted, in the vast majority of cases, isSafelyCallable is actually what people are going to want to use, but that doesn't mean that there's no point to isSafe.

Sorry, this is just plain wrong. Having useless cruft in an API is always something you want to avoid.

The only real issue that I have here is the fact that if @trusted gets changed like you proposed (and I'd love to see happen)

Okay, so let me get this straight. The question is whether to add two templates,

• which might end up being the most rarely used primitives in Phobos (can you come up with a compelling use case?),
• the functionality of which is already easily available using functionAttributes (actually, safe/trusted are even used as an example for functionAttributes in its DDoc comment),
• which lead to conflicts with an already existing template, adding them would require breaking existing code,
• and which would actually become a source of confusing resp. which would need to be removed again if a language change you support (and which seems uncontroversial) is operated.

Now, tell me, what is so crucial about having isSafe/isTrusted that you want to include them despite all this?

[WalterBright]

I'm going to go with klickverbot on this, we shouldn't be redesigning things in the beta. Also, there's David Nadlinger's thread "@trusted considered harmful" which brings up some solid points, and we should take those into account in any design change.

We also need to stop breaking existing code. Names in the library with breaking behavior should get different names.

[dnadlinger]

@WalterBright: I am David Nadlinger. ;) Unfortunately my GitHub account had existed under this nick rather than my real name long before D was on GitHub, so I couldn't easily change it to make the connection more obvious, without breaking people's repo clones.

[WalterBright]

Sorry, I get lost as to which nick goes with which name!

May I suggest then changing your avatar image to be "David Nadlinger" rather than "k"? That would solve both problems.

[WalterBright]

BTW, in the service of enhancing one's professional reputation, it is a good idea to post professional work under one's real name. It puts one's reputation under one's own control, rather than someone else writing about one and that bubbling to the top of google's results.

Save the pseudonyms for the political forums :-)

[WalterBright]

It's also a good career move to stake out:

www.yourname.com
yourname@twitter.com

at least so nobody else grabs it.

[jmdavis]

LOL. My names so common that that sort of thing would be pointless (though I don't expect that David would have that problem - he' the only person I've ever heard of with the last name Nadlinger). I do use my real name for all of the programming and programming-related stuff I do online though, for whatever it's worth.

I'll have a pull request reverting the non-fix related part of this pull request shortly.

[andralex]

OK, will undo this. What's a robust method? I'm looking at git log and see that there are two unrelated pull requests that come on top of this. So if I say git reset --hard 3bd83aad16049fe037b1cded8e386fb9a509b313 (which is the SHA of the commit just before that), I'd also lose the other two pull requests. How to I excise this particular pull request and then repatch with the other two?

Guess we should have a script for that!

[jmdavis]

@andralex There's a required bugfix as part of the changes. We don't want to revert them. I'll be creating a pull request which undoes the unwanted changes shortly.

[andralex]

OK since @jmdavis is doing the work I'm off the hook. Nevertheless I'm curious about how the pull excision could be done.

[jmdavis]

You use git-revert to create a pull request which is the reverse of the one you want to revert. But it reverts an entire pull request (or multiple) at once, not pieces.

[dnadlinger]

@andralex: If you wanted to revert the whole commit – which, in this case, you most likely don't, since isSafe still needs to be fixed – you'd just use git revert <SHA> to create a new commit which reverses the given commit. The reason why you don't want to rebase stuff is that it breaks the clones of people who already pulled in the commit in question (and any after that).

If you just want to undo a commit locally, before making it public, I'd suggest using e.g. git rebase --interactive HEAD~5 (short: -i). This would open a text file containing a line for each of the 5 latest commits. For each of them, you can conveniently choose what you want to do with it by editing the line prefix – check it the »inline documentation« at the bottom of the generated file! I also find it incredibly helpful for squashing changes together, editing commit messages, etc., when preparing a feature branch for upstream merging.

[jmdavis]

#726

[andralex]

Thanks folks. This is the darndest thing - googling for "github undo pull" doesn't yield git-revert on the first page...

[dnadlinger]

@WalterBright: Thanks for the suggestions – I already grabbed the names quite a while ago, and almost exclusively participate in open source projects under my real name right now. GitHub is somewhat different, though, until not too long ago it has been the norm to use pseudonyms here (with the real name listed in the profile, of course) – if you look at the GitHub team or the Rails committer list, you'll find a lot of odd nicknames.

Also, sorry everyone for the overly long discussion. I just thought it wouldn't be a good idea to introduce breaking changes by adding new templates in the middle of a beta, regardless of how the @safe/@trusted discussion turns out, but you seemed to argue as if we already had had isSafe/isTrusted in Phobos, or the necessity of them was a given…