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 2020
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 2020
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 2020
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 2020
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 2020
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 2020
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 2020
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 2020
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 2020
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 2020
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 2020