Menu
Search

Forums

Forum Index

June 08, 2022
Gravatar of claptrapPosted by claptrap
in reply to John Colvin		Reply
claptrap
Gravatar of claptrap
Posted in reply to John Colvin


Reply

On Wednesday, 8 June 2022 at 17:50:18 UTC, John Colvin wrote:

>

On Wednesday, 8 June 2022 at 16:58:41 UTC, deadalnix wrote:

>

On Wednesday, 8 June 2022 at 16:32:25 UTC, John Colvin wrote:

The problem is foo and whether the compiler should somehow prevent the inconsistency between the signature and implementation. Obviously the answer is “yes, ideally”, but in practice with @safe, @system, dip1000, @live and so on it’s all a mess.

Isn't the DIP process supposed to catch these sorts of things?

June 09, 2022
Gravatar of rikki cattermolePosted by rikki cattermole
in reply to claptrap		Reply
rikki cattermole
Gravatar of rikki cattermole
Posted in reply to claptrap


Reply
On 09/06/2022 7:18 AM, claptrap wrote:
> Isn't the DIP process supposed to catch these sorts of things?

DIP1000 was the first DIP to go through the new system. But even then, we are still having to change how it works many years after the fact due to the complexities involved.
June 08, 2022
Gravatar of 12345swordyPosted by 12345swordy
in reply to Meta		Reply
12345swordy
Gravatar of 12345swordy
Posted in reply to Meta


Reply
On Wednesday, 8 June 2022 at 19:07:00 UTC, Meta wrote:
> On Wednesday, 8 June 2022 at 18:44:28 UTC, 12345swordy wrote:
>> On Wednesday, 8 June 2022 at 18:32:41 UTC, Timon Gehr wrote:
>>> [...]
>>
>> I got to say here, you shouldn't be able to compile that code at all if it is going to shoot you in the foot unintentionally.
>>
>> - Alex
>
> I believe this is because foo is not annotated with @safe, thus it's @system by default and you're allowed to do all kinds of unsafe things. Mark it @safe and the compiler will correctly complain:
>
> ```
> @safe
> string foo(in string s)
> {
>     return s; // Error: scope variable `s` may not be returned
> }
>
> void main()
> {
>     import std.stdio;
>     string[] result;
>     foreach(c; "hello")
>     {
>         result ~= foo([c]);
>     }
>     writeln(result);
> }
> ```
>
> In addition, changing `in` to `const return scope` makes the compiler aware that you intend to return the value, and thus it seems to somehow know not to re-use that stack space, and correctly prints ["h", "e", "l", "l", "o"].

You shouldn't have to mark your functions safe to prevent shooting yourself in the foot. It should give a warning message that can be surpass by explicitly marking your function as system.

-Alex
June 08, 2022
Gravatar of Mathias LANGPosted by Mathias LANG
in reply to Steven Schveighoffer		Reply
Mathias LANG
Gravatar of Mathias LANG
Posted in reply to Steven Schveighoffer


Reply

On Wednesday, 8 June 2022 at 19:02:53 UTC, Steven Schveighoffer wrote:

>

But for some reason, this specific code doesn't fail with -preview=dip1000 or -preview=in, but only when both are specified.

Apparently in under preview in doesn't really mean the same thing as scope const. So does it have nothing to do with preview in? simple experimentation says it does.

Well the real bug in this code lies in scope + array being allocated on stack. Cherry picking the instance where it happens with -preview=in is just pointing the finger at a subset of the problem, which is probably where you first encountered the issue ? You unfortunately are at the weird intersection of a few design decisions made by a few different people.

Story time:

When DIP1000 was first introduced, there was a lot of discussion to make sure we have it behind a switch. So -dip1000 was born. Later on, we introduced the -preview / -revert / -transition trifecta as more work was being done on potentially breaking change features.

Now the decision that Walter made, and that I, honestly, still do not understand to this day, was to make in means simply const. The rationale was that it would "break too much code", and if I'm not mistaken, that people expected scope to do nothing (exception for delegates / class alloc), but changing the semantic of in would have too much of an impact. The DIP1000 changes being behind a preview switch, IMO, it shouldn't have been a problem.

Later on, there was a push to get rid of most, if not all, usages of in, especially in druntime C bindings were they were favored.
In parallel, Atila submitted a PR for a new -preview switch which would make in have its actual meaning (dear old -preview=inMeansConstRef).
I once again expressed my (unfavorable) opinion on the topic (https://github.com/dlang/dmd/pull/10769#issuecomment-583229694).

Now, that's where I come in. I implemented -preview=in as you see it today.
But there was ONE constraint I had to keep in, when I did so: -preview=in should also make in act as scope.

That's how you end up with: https://github.com/dlang/dmd/blob/7e1b115a0c62c04b74fecfed8a220d5e31cf4fe0/src/dmd/mtype.d#L4397-L4399

Does it make sense ? I don't think so. If it was up to me, in would always means scope.
Should we change it now ? If we couldn't do it when scope had no effect unless -dip1000 was used, I don't see how we could now that scope does have an effect even without -dip1000.

>

Whether it's right or wrong, it's a change that silently introduces memory corruption. It ought to produce a warning for such code. I'm not blaming anybody here for behavior that is probably correct per the spec. But is there no mechanism to be had for warning about this? D already doesn't allow returning a pointer to stack data, even in @system code. Doesn't this also qualify?

I have argued with Walter for a long time that having scope enforced only in @safe code was a grave mistake, and would be extremely confusing. It would have avoided this situation.

June 08, 2022
Gravatar of DukcPosted by Dukc
in reply to Steven Schveighoffer		Reply
Dukc
Gravatar of Dukc
Posted in reply to Steven Schveighoffer


Reply

On Wednesday, 8 June 2022 at 15:35:56 UTC, Steven Schveighoffer wrote:

>

So silently changing behavior to create new dangling pointers with a preview switch is ok?

Remember, there is already code that does this. It's not trying to be clever via scope, it's not trying to be @safe, it's expecting that an array literal is allocated on the GC (as has always been the case).

This is one of the reasons why all code should endeavour to be @safe wherever possible. I believe C and C++ code often have the same problem: accidently relying on undefined behaviour, that then changes later. D in @system or @trusted is fundamentally no different, even if it sometimes tries to make footguns harder to make.

Alas, I do agree that most of us use @system way too much and thus changes like this always trip us, even when they theoretically should not. But I can't see a good way to avoid that. We could in principle try to avoid UB changes until @safe has become more widespread, but since we are people I suspect the habits don't change before we are kicked often enough :(.

June 08, 2022
Gravatar of DukcPosted by Dukc
in reply to Steven Schveighoffer		Reply
Dukc
Gravatar of Dukc
Posted in reply to Steven Schveighoffer


Reply

On Wednesday, 8 June 2022 at 19:02:53 UTC, Steven Schveighoffer wrote:

>

D already doesn't allow returning a pointer to stack data, even in @system code. Doesn't this also qualify?

-Steve

D does allow it, just not directly:

// error
int* escape(int arg){return &arg;}

// ok
int* escape(int arg)
{ auto temp = &arg;
  return temp;
}

The philosophy is to force to be explicit in the simplest cases that are almost always errors, but not trying to seriously prevent the escape.

June 08, 2022
Gravatar of Steven SchveighofferPosted by Steven Schveighoffer
in reply to Dukc		Reply
Steven Schveighoffer
Gravatar of Steven Schveighoffer
Posted in reply to Dukc


Reply

On 6/8/22 4:49 PM, Dukc wrote:

>

The philosophy is to force to be explicit in the simplest cases that are almost always errors, but not trying to seriously prevent the escape.

This is the same case. You are returning what is a a reference to a scope (local) variable, directly.

Yes, I know this doesn't mean it catches everything.

There is also something to be said about dip1000 not allowing this even for @system code, when it can prove imminent memory corruption.

@system code should not set footguns all over the floor, they should be on a back shelf, where you have to go and grab it in order to fire southward.

-Steve

June 08, 2022
Gravatar of deadalnixPosted by deadalnix
in reply to claptrap		Reply
deadalnix
Gravatar of deadalnix
Posted in reply to claptrap


Reply

On Wednesday, 8 June 2022 at 19:18:20 UTC, claptrap wrote:

>

On Wednesday, 8 June 2022 at 17:50:18 UTC, John Colvin wrote:

>

On Wednesday, 8 June 2022 at 16:58:41 UTC, deadalnix wrote:

>

On Wednesday, 8 June 2022 at 16:32:25 UTC, John Colvin wrote:

The problem is foo and whether the compiler should somehow prevent the inconsistency between the signature and implementation. Obviously the answer is “yes, ideally”, but in practice with @safe, @system, dip1000, @live and so on it’s all a mess.

Isn't the DIP process supposed to catch these sorts of things?

It did.

June 08, 2022
Gravatar of DukcPosted by Dukc
in reply to Steven Schveighoffer		Reply
Dukc
Gravatar of Dukc
Posted in reply to Steven Schveighoffer


Reply

On Wednesday, 8 June 2022 at 21:00:25 UTC, Steven Schveighoffer wrote:

>

On 6/8/22 4:49 PM, Dukc wrote:

>

The philosophy is to force to be explicit in the simplest cases that are almost always errors, but not trying to seriously prevent the escape.

This is the same case. You are returning what is a a reference to a scope (local) variable, directly.

I suppose you're right, that would be good if it errored.

In fact, I'm surprised the compiler does not siletly add return. I did complain about silent adding of return in a fairly similar function here: https://forum.dlang.org/thread/edtbjavjzkwogvutxpho@forum.dlang.org

I did initially advocate for compiling without adding return but as your example demonstrates an error would be better.

June 08, 2022
Gravatar of Walter BrightPosted by Walter Bright
in reply to John Colvin		Reply
Walter Bright
Gravatar of Walter Bright
Posted in reply to John Colvin


Reply
On 6/8/2022 10:50 AM, John Colvin wrote:
> The problem is `foo` and whether the compiler should somehow prevent the inconsistency between the signature and implementation. Obviously the answer is “yes, ideally”, but in practice with @safe, @system, dip1000, @live and so on it’s all a mess.

The checks aren't done for @system code. Yes, the compiler believes you for @system code. It's the point of @system code.

If foo() is annotated with @safe,

  test6.d(5): Deprecation: scope variable `s` may not be returned

The compiler is working as intended, this is not unexpected behavior.
« First   ‹ Prev
1 2 3 4 5 6 7 8 9 10 11 12 13 …
Next ›   Last »