www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - safe function causes attribute inference since 2.072

reply Steven Schveighoffer <schveiguy gmail.com> writes:
I'm trying to fix a bug in phobos, and I was perplexed when the type I 
defined inferred  constructor attributes in my unittest. It looks like this:


 safe unittest
{
    static struct A
    {
       int x;
       this(scope ref return const A other) {}
    }
}

I was surprised that the copy constructor is further inferred to be 
pure,  nogc, nothrow.

I actually wanted it to be not inferred because that is part of the bug 
I'm fixing. After much head scratching, I discovered that it's the  safe 
tag on the unit test.

Observe:

void foo1()
{
   static struct S { void bar() {} }
   pragma(msg, typeof(S.bar)); // void()
}

 safe void foo2()
{
   static struct S { void bar() {} }
   pragma(msg, typeof(S.bar)); // pure nothrow  nogc  safe void()
}

It has been this way since 2.072 (prior to that, foo2.S.bar is just 
 safe void()). I don't see a changelog that shows this to be intended.

Should this be a bug? I think at least the fact that inference only 
happens on structs inside  safe functions doesn't make sense (why not 
pure,  nogc nothrow functions).

But if we "fix" the bug, we risk breaking a lot of code.

-Steve
Apr 13
next sibling parent reply jeckel <jeckel12381236 gmail.com> writes:
On Monday, 13 April 2020 at 15:17:24 UTC, Steven Schveighoffer 
wrote:
 I'm trying to fix a bug in phobos, and I was perplexed when the 
 type I defined inferred  constructor attributes in my unittest. 
 It looks like this:


  safe unittest
 {
    static struct A
    {
       int x;
       this(scope ref return const A other) {}
    }
 }

 I was surprised that the copy constructor is further inferred 
 to be pure,  nogc, nothrow.

 I actually wanted it to be not inferred because that is part of 
 the bug I'm fixing. After much head scratching, I discovered 
 that it's the  safe tag on the unit test.

 Observe:

 void foo1()
 {
   static struct S { void bar() {} }
   pragma(msg, typeof(S.bar)); // void()
 }

  safe void foo2()
 {
   static struct S { void bar() {} }
   pragma(msg, typeof(S.bar)); // pure nothrow  nogc  safe void()
 }

 It has been this way since 2.072 (prior to that, foo2.S.bar is 
 just  safe void()). I don't see a changelog that shows this to 
 be intended.

 Should this be a bug? I think at least the fact that inference 
 only happens on structs inside  safe functions doesn't make 
 sense (why not pure,  nogc nothrow functions).

 But if we "fix" the bug, we risk breaking a lot of code.

 -Steve
I'd say it's a feature. And I'd honestly prefer it to be expanded to regular functions as well. Rather than having to deal with the change all the defaults DIPs. I usually come across a function in phobos that is nogc and pure, someone just forgot to annotate it with those attributes. If it was auto inferred this wouldn't be a problem, it knows whether it is nogc and pure, it should just add those attributes itself.
Apr 13
parent reply Steven Schveighoffer <schveiguy gmail.com> writes:
On 4/13/20 3:11 PM, jeckel wrote:

 
 I'd say it's a feature. And I'd honestly prefer it to be expanded to 
 regular functions as well. Rather than having to deal with the change 
 all the defaults DIPs. I usually come across a function in phobos that 
 is  nogc and pure, someone just forgot to annotate it with those 
 attributes. If it was auto inferred this wouldn't be a problem, it knows 
 whether it is  nogc and pure, it should just add those attributes itself.
I kind of agree, more inference = better code, especially for things that are in functions which cannot be prototyped in a .di file. I'd just as soon see ALL inner functions and types be fully inferred. But there are two sticking points: 1. If I WANT to mark something as gc, throw, or impure, I can't. So there are some cases (unittests are one of them) that are difficult to construct. For example, the now-merged PR that I made had to put this in the constructor: this(scope ref return const A other) { import std.stdio; x = other.x; // note, struct functions inside safe functions infer ALL // attributes, so the following 3 lines are meant to prevent this. new int; // prevent nogc inference writeln("impure"); // prevent pure inference throw new Exception(""); // prevent nothrow inference } I would hope there is a better way to do this. 2. I can't figure out why safe should be the only place where this happens. Which is why I'm not sure if it was intentional or not. Regardless of how helpful it is, if it's a bug, we should look to see why it was "added" and that would help figure out whether we should revert that without breaking the reason it happened. Or make it official, and expand the places it is useful. -Steve
Apr 13
parent reply jeckel <jeckel12381236 gmail.com> writes:
On Monday, 13 April 2020 at 19:45:54 UTC, Steven Schveighoffer 
wrote:
 On 4/13/20 3:11 PM, jeckel wrote:

 
 I'd say it's a feature. And I'd honestly prefer it to be 
 expanded to regular functions as well. Rather than having to 
 deal with the change all the defaults DIPs. I usually come 
 across a function in phobos that is  nogc and pure, someone 
 just forgot to annotate it with those attributes. If it was 
 auto inferred this wouldn't be a problem, it knows whether it 
 is  nogc and pure, it should just add those attributes itself.
I kind of agree, more inference = better code, especially for things that are in functions which cannot be prototyped in a .di file. I'd just as soon see ALL inner functions and types be fully inferred.
I'd say even to functions that appear in header files. Header files are auto generated, they would add the attributes as required. Yes this has the problem that you may unintentionally change the attributes by changing the implementation. If you need to ensure a certain attribute is followed you can just add it to the function.
 But there are two sticking points:

 1. If I WANT to mark something as  gc, throw, or impure, I 
 can't. So there are some cases (unittests are one of them) that 
 are difficult to construct.

 For example, the now-merged PR that I made had to put this in 
 the constructor:

         this(scope ref return const A other)
         {
             import std.stdio;
             x = other.x;
             // note, struct functions inside  safe functions 
 infer ALL
             // attributes, so the following 3 lines are meant 
 to prevent this.
             new int; // prevent  nogc inference
             writeln("impure"); // prevent pure inference
             throw new Exception(""); // prevent nothrow 
 inference
         }

 I would hope there is a better way to do this.
Why would you want to do this? If something is nogc, why would you want to mark it not as nogc? What use case is there? This hasn't been an issue for templates, at least I've never heard someone mention and it being an issue for them.
 2. I can't figure out why  safe should be the only place where 
 this happens. Which is why I'm not sure if it was intentional 
 or not.

 Regardless of how helpful it is, if it's a bug, we should look 
 to see why it was "added" and that would help figure out 
 whether we should revert that without breaking the reason it 
 happened.

 Or make it official, and expand the places it is useful.

 -Steve
That I feel is probably a bug. I tried to narrow down which commit it was, not sure why there's such a huge commit but it's somewhere in here: https://github.com/dlang/dmd/commit/7713ec376d446245ac60a0e3d0ad101d28fea5ba Really hate merges like this instead of a rebase.
Apr 13
parent Steven Schveighoffer <schveiguy gmail.com> writes:
On 4/13/20 8:44 PM, jeckel wrote:
 I would hope there is a better way to do this.
Why would you want to do this? If something is nogc, why would you want to mark it not as nogc? What use case is there? This hasn't been an issue for templates, at least I've never heard someone mention and it being an issue for them.
Here is the issue I was fixing: https://issues.dlang.org/show_bug.cgi?id=20732 What I wanted for a test was a struct with a non-pure, non- nogc, throwing copy constructor to test my fix. I figured this should do it: safe unittest { static struct A { this(scope ref return const A other) {} } A a1, a2; swap(a1, a2); } And I added that test before fixing the problem. And it passed! But doing similar things on run.dlang.io didn't work. Indeed, if you just copy that exact struct definition as a global definition, and then try to use it, it fails. It took me a while to figure out what was going on. So not a common use case. But I would hope one that has a better solution than a) define a unittest-only struct outside unittests, or b) do what I did and add things to prevent inference. I suppose I could have called an extern(C) prototype inside the constructor that was without attributes. I don't really like that either, but probably not as verbose. The bottom line is -- there are no opposites for some of these attributes, so it's awkward to force the inference that way. It's a weird thing to enforce only on safe functions anyway. -Steve
Apr 13
prev sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 4/13/2020 8:17 AM, Steven Schveighoffer wrote:
 Should this be a bug? I think at least the fact that inference only happens on 
 structs inside  safe functions doesn't make sense (why not pure,  nogc nothrow 
 functions).
Here's what you're looking for: https://github.com/dlang/dmd/blob/master/src/dmd/func.d#L1245
Apr 14
parent reply Steven Schveighoffer <schveiguy gmail.com> writes:
On 4/14/20 4:25 PM, Walter Bright wrote:
 On 4/13/2020 8:17 AM, Steven Schveighoffer wrote:
 Should this be a bug? I think at least the fact that inference only 
 happens on structs inside  safe functions doesn't make sense (why not 
 pure,  nogc nothrow functions).
Here's what you're looking for: https://github.com/dlang/dmd/blob/master/src/dmd/func.d#L1245
Thanks. Looks like the real place where this was changed is probably here: https://github.com/dlang/dmd/pull/5881 And then that was refactored. Is there any reason why safe functions are also affected? Is there any reason we shouldn't extend this to other attributed functions, or just all things inside a function context? -Steve
Apr 14
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 4/14/2020 2:08 PM, Steven Schveighoffer wrote:
 Is there any reason why  safe functions are also affected? Is there any reason 
 we shouldn't extend this to other attributed functions, or just all things 
 inside a function context?
I think the rationale was that system-by-default didn't make sense for nested functions inside safe functions when source was available. So being in safe turned on attribute inference for them. It probably does make sense to turn on attribute inference for everything defined within a function.
Apr 14
next sibling parent Steven Schveighoffer <schveiguy gmail.com> writes:
On 4/14/20 10:07 PM, Walter Bright wrote:

 It probably does make sense to turn on attribute inference for 
 everything defined within a function.
I'm 100% on board with that. The only problem is unittests. When I define a type inside unittests to test that things do or do not compile with no-attributed functions, it's hard to make this work. And there are likely a lot of unittests out there that expect no attributes to be significant. Could there be a way to specify the inference should be turned off? Perhaps just for unittests? -Steve
Apr 15
prev sibling parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 15.04.20 04:07, Walter Bright wrote:
 On 4/14/2020 2:08 PM, Steven Schveighoffer wrote:
 Is there any reason why  safe functions are also affected? Is there 
 any reason we shouldn't extend this to other attributed functions, or 
 just all things inside a function context?
I think the rationale was that system-by-default didn't make sense for nested functions inside safe functions when source was available. So being in safe turned on attribute inference for them. It probably does make sense to turn on attribute inference for everything defined within a function.
Yes, absolutely. Also, any kind of attribute "inheritance" is misguided and should be turned off. There is no reason why functions nested in nogc functions should be implicitly nogc, for example. If they are inferred to possibly use the GC, nogc already checks that the enclosing function only calls it in a context where that is okay (for example, in CTFE).
Apr 15
parent RazvanN <razvan.nitu1305 gmail.com> writes:
On Thursday, 16 April 2020 at 03:33:35 UTC, Timon Gehr wrote:
 On 15.04.20 04:07, Walter Bright wrote:
 On 4/14/2020 2:08 PM, Steven Schveighoffer wrote:
 Is there any reason why  safe functions are also affected? Is 
 there any reason we shouldn't extend this to other attributed 
 functions, or just all things inside a function context?
I think the rationale was that system-by-default didn't make sense for nested functions inside safe functions when source was available. So being in safe turned on attribute inference for them. It probably does make sense to turn on attribute inference for everything defined within a function.
Yes, absolutely. Also, any kind of attribute "inheritance" is misguided and should be turned off. There is no reason why functions nested in nogc functions should be implicitly nogc, for example. If they are inferred to possibly use the GC, nogc already checks that the enclosing function only calls it in a context where that is okay (for example, in CTFE).
+1. Currently, dmd is completely inconsistent with regards to qualifier inference. You have different behaviors depending on the attribute (for example: safe is inherited for nested functions, while pure is not; and this depends on the use case, see [1]). I think it would be much more intuitive to infer attributes for functions where the body is certainly known (delegates, nested functions, generated functions - think about member functions of nested structs), but enforce those only when the function is called in a specific context. [1] https://github.com/dlang/dmd/pull/8607
Apr 15