Fix Issue 8161 - deprecate invalid property function definitions

[ntrel]

Add error for void getter or aggregate setter method with 2 params.

[dlang-bot]

Thanks for your pull request and interest in making D better, @ntrel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	8161	enhancement	give an error for invalid property functions

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#8320"

[jacob-carlborg]

I'm not sure if this is a good idea because I'm not sure what we want to do with @property.

[ntrel]

@jacob-carlborg I'm not aware of any plans for @property, could you elaborate? I think getters not returning void is intuitive. Setters accepting more than one value should either be illegal, or otherwise, we remove the limit on accepting a sequence iff it only has two elements (see here).

[jacob-carlborg]

Currently @property doesn't give any value. The question is if we should keep it or not.

[wilzbach]

No matter how our plans for @Property are, I think it's more than reasonable to forbid invalid cases like @Property getters which return void and show a better error message, so I like this idea.

BTW a potential DIP is here: dlang/DIPs#97

Though I think we should update the spec to match the compiler error.

[wilzbach]

Looks like this would need a deprecation period though:

std/parallelism.d(577): Error: getter properties must not return void
std/parallelism.d(605): Error: getter properties must not return void
std/parallelism.d(650): Error: getter properties must not return void
std/parallelism.d(934): Error: template instance `std.parallelism.Task!(run, void delegate())` error instantiating
std/parallelism.d(3587):        instantiated from here: `scopedTask!(void delegate())`
std/parallelism.d(577): Error: getter properties must not return void
std/parallelism.d(605): Error: getter properties must not return void
std/parallelism.d(650): Error: getter properties must not return void
std/parallelism.d(934): Error: template instance `std.parallelism.Task!(run, void delegate())` error instantiating
std/parallelism.d(3587):        instantiated from here: `scopedTask!(void delegate())`
std/regex/internal/thompson.d(73): Error: getter properties must not return void

[jacob-carlborg]

Looks like this would need a deprecation period though

That should be a given 😊.

[wilzbach]

Move test to compilable now we use deprecation

FWIW we typically use -de in the test, so that it can reside in fail_compilation. Though I don't think there's any style guide on this.

[jmdavis]

Personally, I don't think that we should do anything with @property until it's officially decided what the overall plan for it is. There's no guarantee at this point that @property is even going to be left in the language. So, forcing anyone to change code related to @property which may or may not need to be changed later based on what is done with @property doesn't seem like a great idea - even more so if we make them "fix" code that uses @property only to then end up deprecating the feature entirely, resulting in the fix having been pointless.

First, we need to decide what we're actually going to do with @property. After that, maybe changes like this will be necessary and desirable, but for now, it would just be forcing folks to change code that relates to a half-baked feature which both Andrei and Walter have previously stated they thought was a mistake.

[ntrel]

@jmdavis:

There's no guarantee at this point that @property is even going to be left in the language

Removing @property would break code, without any workaround because there is no other way to make typeof(someFunction) not have a function type. For this reason I can't see W & A doing it with D2.

[jmdavis]

Removing @Property would break code, without any workaround because there is no other way to make typeof(someFunction) not have a function type. For this reason I can't see W & A doing it with D2.

Personally, I think that that aspect of @property was a huge mistake. It makes type introspection harder and has caused problems - especially because property functions can only act like variables in a very limited sense. Having the symbols lie about what they are just makes life harder.

But regardless, the point is that @property is in limbo. The original plans for what to do with it have been dropped, and Walter and Andrei have both expressed that they think that the feature was a mistake. At some point here, they're going to need to make a decision about what to do with it. That may involve some sort of enforcement about what sort of function it's put on, and it may not. It may involve solving the problem of properties that return callables, or it could involve dropping the feature entirely. We don't know. We can argue until the cows come home about what we think should or shouldn't be done with @property, but we don't know what Walter and Andrei are actually going to decide to do other than that they dislike the feature and probably want to get rid of it but also don't want to break code, making the course forward anything but clear. And as such, I think that it's a terrible idea to be mucking around with @property until a decision has actually been made. Writing a DIP on the matter could force the issue (and the current DIP on trying to add binary assignment to property functions may end up forcing the issue), but until then, actually making changes in the compiler would mean that we'd be changing a feature for which there is no real plan and no clear future.

Once that decision has been made, then maybe these changes will fit in with that wonderfully, and we can have these extra checks on @property functions, but until then, we'd just be causing problems for folks by telling them to change code that uses a half-baked feature that's really only used for documentation purposes at this point.

[JinShil]

Once that decision has been made

If @WalterBright and @andralex believe @property is a mistake, then it is on them to officially declare it so by either updating the deprecated features table, or otherwise updating the documentation to discourage its use (Or asking one of us to do it). Until then, @property is a legitimate feature of D, and we are obligated to support it.

This PR helps users employ @property correctly, and having D code out there with fewer oddities like a void getter is a good thing for everyone. Having the compiler notify users when something's wrong or odd helps them learn the language and makes for a more pleasant coding experience. D already has enough oddities competing for mindshare; we need less, not more.

Though I think we should update the spec to match the compiler error.

Yes, @ntrel, please add a spec update to ensure these new enforcements are properly documented.

[jmdavis]

Until then, @Property is a legitimate feature of D, and we are obligated to support it.

What feature? Basically all it does is affect the result of typeof and disallow you to overload functions that aren't marked with @property with those that are. It's just documented wishful thinking right now, not really an implemented feature. What it was originally intended to do was never actually implemented.

[JinShil]

I'm not sure if this is a good idea because I'm not sure what we want to do with @Property.

This PR will actually help with the uncertainty surrounding @property. It will reduce the number of patterns in which @property can be used, so if/when a decision is made about @property there will be fewer cases for which there may be breakage, and that's good.

[jmdavis]

This PR will actually help with the uncertainty surrounding @Property. It will reduce the number of patterns in which @Property can be used, so if/when a decision is made about @Property there will be fewer cases for which there may be breakage, and that's good.

Not really. It's just breaking some of them now rather than later - and that's assuming that we're even going to do anything with @property where this change makes sense. Sure, if we add some sort of property enforcement, these changes make sense, but it was pretty much decided that we're not going to add property enforcement. These changes are forcing people to change their @property code on the assumption that @property is actually going to mean something and be enforced in some manner, and it's not at all clear that that's going to happen. Maybe it will, maybe it won't, but if it does, then these changes can be added then along with whatever potentially breaking changes would need to be made for whatever is done with @property. For now, it's forcing folks to change their code for an essentially unimplemented feature.

[jmdavis]

Honestly, I think that this is one of those cases where @WalterBright just needs to weigh in. If he's fine with adding these checks, then okay, but otherwise, they shouldn't go in. They're mucking around with an incomplete and pseudo-canceled feature, so they're really more in DIP territory rather than a bug fix or simple improvement that we clearly want.

[JinShil]

but it was pretty much decided that we're not going to add property enforcement

No, it wasn't decided, because that phantom decision that you claim exists is not currently reflected in the documentation. If you want any of that hearsay to hold water, it needs to be officially added somewhere in the documentation.

These changes are forcing people to change their @Property code on the assumption that @Property is actually going to mean something and be enforced in some manner,

No. It's forcing people to change their currently incorrect @property code based on the feature's current design and intended use.

For now, it's forcing folks to change their code for an essentially unimplemented feature.

It's not unimplemented. It's partially implemented. And this PR provides enforcement for that part which is implemented.

Honestly, I think that this is one of those cases where @WalterBright just needs to weigh in.

I would prefer that too, but @WalterBright and @andralex rarely weigh in on these little things. And, until they do, and their decision is somehow reflected in the documentation, @property is a feature of D that should be supported.

so they're really more in DIP territory rather than a bug fix or simple improvement that we clearly want.

No. Deprecating @property and choosing to not support it any longer is DIP territory, and until then, or until @WalterBright or @andralex say otherwise, it should be supported.

[ntrel]

@Geod24:

there is previous work to go in exactly the other direction this PR is going

No, that pull just affected -property, not @property.

[AndrewEdwards]

@WalterBright @andralex @atilaneves

Gentlemen, the mail is far overdue on @Property. Request a discussion amongst yourselves and a formal decision made regarding the fate of this non-feature. Seven years is way more than enough time to make a decision. It's far better to kill it than to remain utterly indecisive about it.

[ntrel]

@AndrewEdwards you can't kill property without breaking code using typeof. Breaking D2 code with no error for previously correct code is unacceptable.

[AndrewEdwards]

@ntrel, point taken. In your opinion, what is the correct/acceptable solution moving forward? Is that to find a solution to improve typeof such that it no longer depends on @property? As far as I see it, the decision was already made. Accept the status quo! The problem is that because the leaders did not explicitly say so, the world is at a standstill. Therefore, I am asking for that decision.

[12345swordy]

I opt to finished this implementation as @andralex still uses the property code in the druntime recently. Property needs a major code overall here to be useful as c# properties.

[AndrewEdwards]

@WalterBright @andralex @atilaneves

Gentlemen, your input is requested.

[wilzbach]

@WalterBright @andralex @atilaneves

Gentlemen, your input is requested.

FYI: most people have GitHub notifications turned off as there's too much noise. So please don't assume that your pings will reach people. For certainty, old-school email is required.

[atilaneves]

I always get notifications if I'm pinged.

In this instance I'll defer to @WalterBright

[WalterBright]

@Property is more or less a failure. But I don't want to break peoples' code that uses it in good faith. I recommend not doing anything with @Property, document properly what it does now, and recommend people not use it for new code.

Fixing problems with it like this PR both breaks existing code and appears to encourage new code to be written with it.

[12345swordy]

I see andralax write new code using @Property in the druntime. I hate it to see it in a half bake state with no attempts to fix it.

[andralex]

@12345swordy You mean the typeinfo stuff? I think it was more to stay in keeping with the existing code. Not to worry about.

[12345swordy]

Real talk, why can't we just mark @Property as deprecate with no remove date? I want actual c# like properties here.
If I have to write a dip on this, then I will do it myself.

[andralex]

IMHO the best way (and the obvious first step) is to marginalize in the documentation.

[AndrewEdwards]

CC @WalterBright @andralex @atilaneves @ntrel

In order to summarize my understanding fully. The decision is made to:
1) Cease all attempts to modify/update/upgrade the current behavior of User-Defined Properties
2) Properly documentation the behavior as it is currently implemented in the language
3) Suggest to the community NOT to use @property for future code.
4) Issue a warning on use of @property ("Warning: Current implantation of @Property is incomplete and is only retained for backward compatibility. Not to be used in new code!")
5) Remove all uses of @property in the documentation where it is not absolutely necessary

To move forward we will need to update the content of the following pages:
https://dlang.org/spec/function.html#property-functions
https://dlang.org/spec/declaration.html#typeof
https://dlang.org/spec/traits.html#getFunctionAttributes
https://dlang.org/spec/grammar.html#Property
https://dlang.org/spec/operatoroverloading.html#array-ops
https://dlang.org/spec/abi.html#FuncAttrProperty

@wilzbach @jmdavis, request your assistance in making this happen. Once completed, this pull request will be closed.

[ntrel]

Feel free to close this pull. I don't see any good argument for deprecating @property. It causes no harm to users, remove it only if D3 happens. For the toolchain, document that no further work will be done on it. Don't break people's code with no workaround for (possibly external) library code using typeof.

[wilzbach]

There's nothing I can do here, hence I unassigned myself. This needs a decision from leadership.

If it's decided to deprecate @Property, I'm happy to help.

[Geod24]

As mentioned, there's no clear path for property. If a champion comes with a proper plan, timeline, and way to manage breakage, great. In the meantime, let's leave it be, there are bigger fish to fry.