This will compile without a single warning:
//----
int[1] foo();
int[] a = foo();
//----
It's wrong, because we are obtaining a slice to temporary data.
This isn't even a question of escape analysis or anything like that. It's really just nothing more than (basically) taking the address of a temporary, which is pretty much wrong 100% of the time, and never correct.
I've been hit by it several times. I've fixed and seen the bug in phobos. It's come up regularly in phobos learn.
Without entirelly putting into question the implicit slicing of static arrays, can we *please* at least patch this specific use case?
The *only* code this *can* break (and *will* break) is code that is certifiably worng.
Comment #1 by issues.dlang — 2014-05-08T07:05:41Z
Why just make the implicit slice illegal? Shouldn't it just be illegal to slice it at all, since it's guaranteed to be wrong?
Comment #2 by monarchdodra — 2014-05-08T08:16:28Z
(In reply to Jonathan M Davis from comment #1)
> Why just make the implicit slice illegal? Shouldn't it just be illegal to
> slice it at all, since it's guaranteed to be wrong?
I thought about it some more, and there *are* case where you can slice an rvalue static array, provided you've "used" it by the end of the expression. EG:
//----
int sum(int[] i)
{
return reduce!"a+b"(i);
}
void main()
{
enum int[3] ints = [1, 2, 3];
auto s = sum(ints);
}
//----
Here, "ints" is an rvalue static array, and slicing it is legit.
So it appears my initial description of the problem is no accurate. There are times where you can slice an rvalue array.
Not sure where to take this now.
- Do we say "slicing an Rvalue static array is dangerous, and deprecated, please do it explicitly"?
- Do we find some way to detect *only* the "rvalue static array to dynamic array assignment" case?
- Give up?
Comment #3 by issues.dlang — 2014-05-08T09:32:18Z
Well, per issue# 8838, we need to make slicing a static array - implicit or otherwise - @system. As it stands, that's a huge hole in @safe. So, that affects this on some level.
Regardless of whether implicit slicing takes place, I think that if there are any situations where we can statically know that slicing a static array not only unsafe, but it's guaranteed to be wrong, we should make that an error. There's no reason to allow it. I don't know how easy it will be to enumerate those cases and make them errors, but at minimum, as we find them, we should make them errors.
I am 100% convinced that it was a huge mistake to make it so that static arrays are ever implicitly sliced, so I would be very much in favor of deprecating that and move towards removing it from the language, but it would break a lot of code. I think that it's at least similar to the breakage caused by removing implicit fallthrough in that it's highly bug-prone and as such is worth the breakage, but I also don't think that it's wrong in anywhere near as many cases as implicit fallthrough is, so the gain isn't as great. It's also probably far more prevalent in code than switch statements are, so it's likely to be a hard sell. A number of people have already balked at the idea of making slicing static arrays @system even though it's a fact that that has to happen, or we have a whole in @safe - there's no way around that (at best, we _might_ be able to make it @safe in a few cases where scope was used - if scope were fully implemented, which it's not). So, I'd expect a lot of folks to balk at this as well. But I'm convinced that it would be worth it. The question is whether enough people (and in particular, Walter and Andrei) can be convinced of that as well.
But I definitely don't think that we should give up on this in that any case where we know that it's guaranteed to be wrong, it should be an error. There's no reason to allow it aside from the fact that we simply missed that particular case (which should then be fixed when we find it).
Comment #4 by bearophile_hugs — 2014-05-08T09:37:32Z
(In reply to monarchdodra from comment #2)
> - Do we find some way to detect *only* the "rvalue static array to dynamic
> array assignment" case?
> - Give up?
Those seem two options. The first introduces a special case.
Comment #5 by nick — 2016-04-07T14:35:43Z
(In reply to monarchdodra from comment #2)
> I thought about it some more, and there *are* case where you can slice an
> rvalue static array, provided you've "used" it by the end of the expression.
...
> enum int[3] ints = [1, 2, 3];
> auto s = sum(ints);
...
> - Do we find some way to detect *only* the "rvalue static array to dynamic
> array assignment" case?
Your original example is always wrong:
int[1] foo();
int[] a = foo();
Slicing a returned static array should be disallowed by the compiler. Should we rename this issue or file a new one for just this?
Comment #6 by dfj1esp02 — 2016-04-07T14:52:35Z
Allow sliced temporary to be passed straight to function parameter (sounds like rvalue reference though), disallow everything else?
Comment #7 by schveiguy — 2016-04-14T13:30:11Z
This came up in the newsgroup: (https://forum.dlang.org/post/[email protected])
I think we can say safely that slicing an rvalue return cannot lead to anything good.
As for the example of why it should be allowed, I'm not convinced. That example requires blind faith that the receiver of the slice never does anything bad with it, and assumes the compiler doesn't cleverly reclaim the memory for the rvalue seeing as it wasn't used anywhere. We should reject it for the same reason we reject fun(&generateSomeInt()). Sure, fun may not do anything bad, but it has a lot of leeway to do so.
Not sure how to up the importance of this.
Note, let's not confuse this issue with slicing of a static array that has dedicated stack space (i.e. not a temporary). That may also be rendered an error somehow, but this is vastly more erroneous.
Comment #8 by dfj1esp02 — 2016-04-14T14:27:42Z
(In reply to Steven Schveighoffer from comment #7)
> I think we can say safely that slicing an rvalue return cannot lead to
> anything good.
>
> As for the example of why it should be allowed, I'm not convinced.
tempCString is another example of an rvalue reference. Works good so far.
> example requires blind faith that the receiver of the slice never does
> anything bad with it, and assumes the compiler doesn't cleverly reclaim the
> memory for the rvalue seeing as it wasn't used anywhere.
If the temporary isn't used, it should be reclaimed.
Comment #9 by schveiguy — 2016-04-14T14:55:21Z
(In reply to Sobirari Muhomori from comment #8)
> tempCString is another example of an rvalue reference. Works good so far.
Another good example of an error waiting to happen:
char *str = someString.tempCString;
This would be more difficult for the compiler to figure out, because the address-taking is done inside a method.
tempCString also has a destructor, so pushing the destruction of the temporary to after the expression is actually easier to mandate/do.
>
> > example requires blind faith that the receiver of the slice never does
> > anything bad with it, and assumes the compiler doesn't cleverly reclaim the
> > memory for the rvalue seeing as it wasn't used anywhere.
>
> If the temporary isn't used, it should be reclaimed.
What are the rules for temporaries that aren't further used in the expression? In this case, the result of foo isn't passed into another function, just a slice is.
I suppose if the rules mandate any temporaries live until after the expression is completed, then we don't need to worry about the memory being reclaimed mid expression. We are then just left with the issue of assigning a pointer that points at the temporary. How to figure out generally that this is the case? Not sure.
IMO, however, the original problem (assigning a slice of a temporary) should be easy to flag, since the compiler understands what is happening, and knows that it is obviously invalid.
Comment #10 by issues.dlang — 2016-04-14T16:29:36Z
> What are the rules for temporaries that aren't further used in the expression? In this case, the result of foo isn't passed into another function, just a slice is.
In C++, the temporary has to be valid until the end of the statement (which is by far the safest approach from what I can see), but I'm not sure that we've actually spec-ed it out to that level of detail in D. So, temporaries might disappear sooner (though I'm inclined to think that they shouldn't). Regardless,
int[1] foo()
{
return [1];
}
void main()
{
int[] a = foo();
}
is essentially equivalent to
int[1] foo()
{
return [1];
}
void main()
{
int a* = &foo();
}
and that doesn't compile even with @system.
I'm 120% of the opinion that slicing a static array should _always_ be considered @system, and I really don't see much reason to make it any more legal to slice a temporary static array than it is to take its address. And isn't that pretty much what separates an lvalue and an rvalue - that the lvalue is guaranteed to have an address that can be assigned to, whereas an rvalue isn't guaranteed to have an address? By slicing a temporary static array, you're basically treating an rvalue like an lvalue, which is clearly bad.
Comment #11 by petar.p.kirov — 2016-04-15T06:39:59Z
I don't think that it should be allowed even in @system code. I had a very ugly stack corruption because of this bug in a networking project. Thankfully, I had thorough unittests to catch it early, but still it wasn't obvious to track down at all.
Comment #12 by petar.p.kirov — 2016-04-15T07:23:34Z
(In reply to ZombineDev from comment #11)
> I don't think that it should be allowed even in @system code. I had a very
> ugly stack corruption because of this bug in a networking project.
> Thankfully, I had thorough unittests to catch it early, but still it wasn't
> obvious to track down at all.
Just to clarify - I'm talking about OP case. Slicing static arrays in general can be @safe with enough escape analysis - e.g. when the slice doesn't outlive the slicee.
Comment #14 by issues.dlang — 2016-04-15T23:55:33Z
(In reply to ZombineDev from comment #12)
> Just to clarify - I'm talking about OP case. Slicing static arrays in
> general can be @safe with enough escape analysis - e.g. when the slice
> doesn't outlive the slicee.
In theory, it colud be @safe in such a case, but the same can be said of taking the address of a local variable, which is currently always @system. The two cases are identical save for the fact that slicing a static array gives you a pointer and a size_t in a struct rather than just a naked pointer. So, they should be treated the same with regards to @safety and what types of operations are legal. If that means that we get escape analysis for both, then that's fine, but it would be inconsistent to do such escape analysis on the slicing of static arrays and not taking the address of a local variable.
Regardless, barring the addition of such escape analysis, slicing a static array should _always_ be @system where it's legal at all. And this bug highlights a case where it shouldn't even be legal (and isn't legal with the equivalent code that involves taking the address of the temporary instead of slicing it).
Comment #15 by issues.dlang — 2016-04-16T04:34:59Z
Related: issue# 15932
Comment #16 by petar.p.kirov — 2016-04-19T15:10:23Z
(In reply to Jonathan M Davis from comment #14)
> (In reply to ZombineDev from comment #12)
> > Just to clarify - I'm talking about OP case. Slicing static arrays in
> > general can be @safe with enough escape analysis - e.g. when the slice
> > doesn't outlive the slicee.
>
> In theory, it colud be @safe in such a case, but the same can be said of
> taking the address of a local variable, which is currently always @system.
> The two cases are identical save for the fact that slicing a static array
> gives you a pointer and a size_t in a struct rather than just a naked
> pointer. So, they should be treated the same with regards to @safety and
> what types of operations are legal. If that means that we get escape
> analysis for both, then that's fine, but it would be inconsistent to do such
> escape analysis on the slicing of static arrays and not taking the address
> of a local variable.
Yes, I agree. I meant escape analysis in general.
> Regardless, barring the addition of such escape analysis, slicing a static
> array should _always_ be @system where it's legal at all. And this bug
> highlights a case where it shouldn't even be legal (and isn't legal with the
> equivalent code that involves taking the address of the temporary instead of
> slicing it).
Yes, this OK as temporary solution. Unfortunately, it may be considered a huge breaking change for valid existing code, because it makes static arrays useless in @safe D.
Comment #17 by petar.p.kirov — 2017-03-16T18:35:41Z
*** Issue 17261 has been marked as a duplicate of this issue. ***
Comment #18 by hsteoh — 2017-03-16T22:40:15Z
Marking this as accepts-invalid, because this code is allowed even with -dip1000:
------
int[16] func();
int[] a = func();
------
According to DIP 1000:
1) Lifetimes of statically-sized arrays T[n] is analyzed as if the array were a struct with n fields, each of type T. [Under "Aggregates"]
2) For types without indirections such as int, reachability and lifetime are equal for rvalues and lvalues. [Under "Definitions" > "Reachability vs. lifetime"]
3) For an rvalue, the reachability is the expression within which it is used. [Under "Definitions" > "Reachability vs. lifetime"]
4) The lifetime of e[] (implicit in this case) is lifetime(e) [Under "Definitions" > "Algebra of lifetimes"]
5) A scope variable can only be initialized and assigned from values that have lifetimes longer than the variable's lifetime. [Under "Fundamentals of scope"]
Point (1) indicates that static arrays should be analysed as structs with n fields, and therefore int[n] is subject to (2): reachability and lifetime are equal. Point (3) indicates that an rvalue has reachability equal to the expression in which it is used, which, by point (2), implies that the lifetime of the static array is also the expression in which it is used. Then the slice produced by the implicit slicing, according to (4), must also have the same lifetime, i.e., the expression in which the rvalue occurs.
Since the lifetime of `a` in the code above is longer than the lifetime of the expression on the right-hand side, by (5) this assignment is illegal.
So, the fact that -dip1000 lets this code through is a bug in the implementation of DIP 1000.
For those not following the PR in Phobos discussion, an interesting alternative idea has been floated by ZombineDev:
char[16] get();
string s = get();
// can be lowered to:
auto _tmp = get();
scope string s = _tmp[];
Which should work and keep existing code intact. Requires dip1000 I believe.
Comment #21 by github-bugzilla — 2017-11-29T10:43:09Z