fix #20508 escaping sliced stack arrays not detected

[WalterBright]

No description provided.

[dlang-bot]

Thanks for your pull request, @WalterBright!

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.

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• In preparation for migrating from Bugzilla to GitHub Issues, the issue reference syntax has changed. Please add the word "Bugzilla" to issue references. For example, Fix Bugzilla Issue 12345 or Fix Bugzilla 12345.(Reminder: the edit needs to be done in the Git commit message, not the GitHub pull request.)

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#22207"

[WalterBright]

m_lock.performLocked!({ m_streamWSize[sid] = newWin; });

  | ^
  | ../.dub/packages/vibe-http/1.3.1/vibe-http/source/vibe/http/internal/http2/exchange.d(84,9): Error: returning scope variable res is not allowed in a @safe function

Anyone know anything about this code?

[WalterBright]

@dkorpel may need to enable this with an edition

[dkorpel]

may need to enable this with an edition

If you guard the code with if (sc.hasEdition(Edition.v2024)) it will only take effect from that edition onwards.

[dkorpel]

Anyone know anything about this code?

https://github.com/vibe-d/vibe-http/blob/94e4d1fe6c5eace1de38456a79040c5e94b422e7/source/vibe/http/internal/http2/exchange.d#L55

It looks like it incorrectly marks the allocator parameter as scope instead of return scope in a @safe function, which should be an error but it passes because it's not compiled with -preview=dip1000. This PR restricts returning scope variables even without dip1000, so it now breaks.

[dkorpel]

This is the wrong place to change. This still enables escaping stack allocated slices through other means than return, and if you were to perform this change consistently you are basically just enabling DIP1000 by default.

Without dip1000, slicing a static array a[] should be treated the same as taking the address of a local &a as far as @safe is concerned, and taking the address of locals is prevented by the checkAddressVar function called from AddrExp::semantic. The same could be done with SliceExp::semantic:

--- a/compiler/src/dmd/expressionsem.d
+++ b/compiler/src/dmd/expressionsem.d
@@ -10954,6 +10954,17 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
         }
         else if (t1b.ty == Tsarray)
         {
+            if (auto ve = exp.e1.isVarExp())
+            {
+                if (auto vd = ve.var.isVarDeclaration())
+                {
+                    if (!checkAddressVar(sc, exp, vd))
+                    {
+                        result = ErrorExp.get();
+                        return;
+                    }
+                }
+            }
         }
         else if (t1b.ty == Ttuple)
         {
@@ -17649,7 +17660,7 @@ Expression modifiableLvalue(Expression _this, Scope* sc, Expression eorig = null

Of course this is a massive breaking change so it needs to be guarded by an edition.

[ntrel]

BTW DIP1000 is currently enabled for the 2024 edition, so it catches escaping slices anyway.

[WalterBright]

I appreciate your ideas here. But I have a bit of a different take. As you explained, I put the check on the return. (I also put it on an assignment to a global.) Returning a pointer to the stack is always an error (detected or not). That's why there is value in not requiring dip1000 to check it. But taking a slice of a stack array is legitimate (I use it often as a temporary buffer.

The compiler already attaches scope to any pointer or slice of a stack variable. Making use of that does not require any annotations nor inference of function interfaces, which were the problems with dip1000.

Returning a pointer to the stack is always a bug in the code, and I have no qualms about checking for this without adding the noted difficulties of dip1000. Yes, this doesn't detect everything dip1000 does, but I don't see a downside to adding these checks without requiring dip1000.

I'd like to see what more we can do without the problems of dip1000.

I was a bit nervous about what failures we'd encounter with this PR, but it seems that it is only detecting overlooked bugs in existing code. Which is good news! But in the spirit of not breaking existing code, putting it behind an edition is reasonable.

[dkorpel]

But taking a slice of a stack array is legitimate

Which you can always do in @system or @trusted code. You said before that @safe is not just a warning / linting system, it's supposed to be robust. Are we watering it down now, intentionally leaving holes in it?

[WalterBright]

We want to move to @safe by default. This PR increases safety by default, and the only code it breaks is code that is broken already. It does not carry with it the difficulties that dip1000 has. dip1000 makes @safe safer, but has turned out to be too much of a burden for programmers. I aim to make it safe without those difficulties. This is a good step in that direction.

[WalterBright]

Good observations!

1. yes, it is incomplete. It doesn't detect b = c ? null : s[1..2]; for example. But these cases are all fixable. But I don't see a point in doing that work if this doesn't move forward.
2. robust: yes, your proposed solution is robust. But it also does not allow practical and verifiably safe use cases. I use stack allocated arrays as buffers and then use slices of them. We should be allowing things that are verifiably safe.
3. I agree that dip1000 is a failure. Its failure is because of the confusing nature of the annotations. The failure is not one of concept. The semantics can be implemented without the need for annotations (except for functions with no bodies). We should be heading in that direction.
4. I agree the error message could be more helpful. A bad error message is insufficient reason to abandon the idea. Note that the user will get an error message with both your proposal and mine.

[WalterBright]

I'd like to add that:

int i;
int* p = &i;

can be replaced with ref r = i; but there isn't an equivalent ref slice. Therefore, the pointer restriction is not equivalent to restricting a slice.

[dkorpel]

I guess we'll have to wait until the editions meeting to see how we decide on this matter. As a reminder: this PR's code changes are still useless because of #22207 (comment), I can revert them with no observable change. (Try it if you don't believe me, I can even do it for you if you want)

[WalterBright]

I improved the error message to indicate inference of scope.

[WalterBright]

Yes, it is only in-play with the edition.

[WalterBright]

@dkorpel I added the edition check, thanks!

[WalterBright]

This is ready to go.

[WalterBright]

@dkorpel spec pr dlang/dlang.org#4356

[WalterBright]

@dkorpel spec PR has been merged

[dkorpel]

That doesn't change anything about #22207 (comment) though

[WalterBright]

@dkorpel I replied upthread.