Fix issue 12931: Make ambiguous qualifiers illegal on the LHS of a function

[Geod24]

This has been a pain point for many D users for years,
both new and old, as can be seen from the discussions on the issue.
It is however a potentially disruptive deprecation.

Note: The test for issue 5962 in test/runnable/xtest46.d has been
removed as the issue was only present with prefix syntax.

Depends on dlang/phobos#7389 & dlang/phobos#7391
I'll take a look at what is affected in Buildkite before we merge this.

Still needs a spec PR, and since it's a significant change, @WalterBright / @atilaneves ' approval.

[dlang-bot]

Thanks for your pull request, @Geod24!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	12931	enhancement	Make const, immutable, inout, and shared illegal as function attributes on the left-hand side of a function

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + dmd#10757"

[Geod24]

This probably would have to be held off for a bit. The D1 operators, body, and now this deprecation are starting to add up and hurt long-time users (CC @don-clugston-sociomantic ).

It would be great if we could go for longer-time support periods (e.g. LTS releases), but we'd probably need someone that is actually employed to do it.

I will split off the changes in header generation though, as those can go on their own.

[thewilsonator]

Should this also include scope? (modulo industry burden)

[Geod24]

scope is not a type constructor, it's a storage class. In other word, it does not "change" the type, just where said type is stored. The following code does not parse: scope(Object) foo ();, for the same reason static(Object) foo() would not parse.

Hence, scope is not ambiguous. I personally prefer to have everything that does not need to be on the LHS be on the RHS of the function (that is, any attributes such as @property, @safe, nothrow, scope, return, etc... but not override and visibility which need to be RHS), but those are at least not ambiguous.

[thewilsonator]

Then what does scope void mean in

     static class A { scope void toString(C, W)(scope ref W w) const { w.put(C('a')); } }
     static class S { scope void toString(W)(scope ref W w) const { w.put("s"); } }
     static class D { scope void toString(Dg)(scope Dg sink) const { sink("d"); } }
     static class F { scope const(char)[] toString()() const return { return "f"; } }

?

[Geod24]

It means the this parameter is scope. Code example:

class A {
    const(A) bar() const @safe { return this; }
}
const(A) bar () @safe
{
    scope foo = new A;
    auto x = foo.bar(); // Line 10
    return x;
}

Compiling with -preview=dip1000 yield:

foobar.d(10): Error: scope variable foo assigned to non-scope parameter this calling foobar.A.bar

You notice that the error is about the call to bar. Now if you add scope to A.bar:

foobar.d(3): Error: scope variable this may not be returned

The class itself does not know its allocation strategy. Nor does the caller know if the instance of A being returned is this or something else (e.g. new GC-allocated instance). Now it is okay for the function to return itself, as long as it tells that's the case. So if you add return to that A.bar function, the error turns into:

foobar.d(11): Error: scope variable x may not be returned

Which is, IMO the most correct error.

Going back to your examples, what does scope means ? Hard to say, it depends on the template parameter. Most of the time, it won't have any impact, without or without dip1000.

[thewilsonator]

It means the this parameter is scope

Which is what I thought, hence this PR, no?

[Geod24]

Which is what I thought, hence this PR, no?

The difference is you cannot make the return value scope. At least not via scope, only via the return scope combination (and return is actually what makes it scope).
While const string foo(); looks like you're returning a const(string) when you're in fact returning a string value and your this is const.

[thewilsonator]

The difference is you cannot make the return value scope.

Maybe, but it certainly looks like it.

[don-clugston-sociomantic]

This probably would have to be held off for a bit. The D1 operators, body, and now this deprecation are starting to add up and hurt long-time users (CC @don-clugston-sociomantic ).

Actually it's the other way around. If you are going to do the 'body' deprecation in the next release (which I really think you shouldn't right now, but if you do) then you should do this at the same time.
The worst scenario is when every single release breaks existing code.

The body thing seems just a pointless change with no benefit, just so that somebody can say the language has fewer keywords. But this one has an obvious benefit. Removing this ambiguity makes the code better, so it's an easier sell. Also the changes required by this one are backwards-compatible for more than a decade.

It would be great if we could go for longer-time support periods (e.g. LTS releases), but we'd probably need someone that is actually employed to do it.

If you have no resources at all then the next best thing is to delay major breaking changes, and then do them all at once (eg no more than one such release per year). This means that the version with the breaking changes is a pseudo-LTS because it will be at least year until more breaking changes occur.

[WalterBright]

Note that:

const void foo();
const: void foo();
const { void foo(); }

all do the same thing. Breaking this for the first case would just introduce a weird inconsistency.

[Geod24]

Do you want const: and const {} deprecated as well ?

[WalterBright]

No.

I just see this change as pointlessly ticking people off. May I remind you of the proposal to remove static struct initializers :-) I didn't think anyone would complain about it, but sure as shootin', people who admitted never using them became dauntless champions of them.

[Geod24]

The difference is:

• There is a simple replacement / fix for this, which is backward compatible (as Don noted);
• There is an issue open for 5 1/2 years on Bugzilla which has garnered wide support (and not a single dissenting voice so far);
• There is a precedent of disallowing error-prone syntax (static: this(), parenthesis around ternary, etc...)
• It is a common mistake when translating C/C++ to D (e.g. fix some compilation issue with 2.066.0 D-Programming-Deimos/openssl#23)

To me this is more similar to disallowing the C-style array syntax (char arr[6]) than the struct literal case.

Note that if you feel strongly that we shouldn't pull this as-is, I can see two ways forward:

• We need to make it a DIP, in which case I'll submit one;
• You think it's a bad idea, close the issue as WONTFIX, and we move on;

I was really on the fence about raising a DIP, but figured, since no-one seemed to disagree on this, I'd just go ahead with a PR.

[WalterBright]

Let's just go with WONTFIX. We've got a mountain of more important things we should be doing.

[WalterBright]

thank you

[schveiguy]

people who admitted never using them became dauntless champions of them.

This is an unfair characterization (as it was aimed at me). I took a look at the use cases that were provided and agreed with the people who were complaining that those use cases would look much worse. Before seeing the use cases, I was fully on board with deprecation of brace initialiation.

One does not have to be a user of a feature to comprehend why it would be painful to remove for those who do use it.

[WalterBright]

Perhaps it is a little unfair, and I apologize for that. But it is an obscure use case, just a few extra characters, and not the burden it was characterized as.

Nevertheless, I'm leaning towards withdrawing support for the DIP. There's far more important work to be done than that DIP.

[Geod24]

Today, someone on my team asked this question:

Why do I get this error message ??
source/tests.d(105,13): Error: associative array literal in @nogc function tests.NodeLocatorMock.extractValues may cause a GC allocation

The code in question:

/// Interface for the node locator
public interface INodeLocator
{
    public bool start ();
    public string[] extractValues (string address, string[] paths);
}
public class NodeLocatorMock : INodeLocator
{
    public bool start () {return true;} const @safe @nogc nothrow pure
    public string[] extractValues (string address, string[] paths)
    {
        // Returns the same information regardless of the address
        string[string] path_map =
            [
                "continent->names->en" : "Asia",
                "country->names->en"   : "South Korea",
                "city->names->en"     : "Seoul (Namdaemunno 5(o)-ga)",
                "location->latitude"   : "1.111",
                "location->longitude"  : "2.222",
            ];
        return null;
    }
}
int testmain()
{
   return 0;
}

There might be "more important things to do", but it doesn't mean this isn't important.

[wilzbach]

"more important things to do"

With this argument one can close a lot of PRs...

[WalterBright]

Today, someone on my team asked this question:

As noted before, dealing with this just creates another inconsistency and wart.

Not in D, C, C++, Java, or any curly brace language I've ever heard of are attributes allowed after the closing } of a function definition. At some point, we're chasing rainbows by breaking one syntax to add another, and not really getting anywhere.

But one can use this as a teaching moment. In D, C and C++, whitespace is not significant and is used only for aesthetics. Hence, if one is baffled by an error message on line X, it can be helpful to look at what is at the end of line X-1 or the start of line X+1.

[Geod24]

Well you mentioned that this would be "ticking people off". I think this change would be well received on the long run, even if it would create annoyance in the short term, just like implicit string concatenation did. If it's any indication, look at the reactions (👍) to the original post and your veto.

As to inconsistencies, they already exists. TDPL (rightfully) boasts about const being transitive, so that the following declarations have the same type:

void main ()
{
    const int* ptr;
    const(const(int)*) ptr2;
    const(int*) ptr3;
}

However, I can't count how many times I had to point out that the following are not the same:

const(Object) get () nothrow;
const Object  get () nothrow;