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

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.

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

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.

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

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

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

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.

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

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).