www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - DIP 1038: nodiscard - Unofficial "post-final" review

reply Paul Backus <snarwin gmail.com> writes:
In response to Walter's feedback [1] from the final review round, 
I've added a new section to DIP 1038 ( nodiscard) on "Design 
Goals and Possible Alternatives".

Since it's a substantial addition (almost 50% of the DIP's 
original length), Mike Parker, the DIP Manager, has graciously 
allowed me to postpone formal assessment to give the community a 
chance to look it over, and point out any silly mistakes I may 
have made.

If you'd like to help  nodiscard succeed, I hope you'll give the 
revised version a read and reply to this thread with any 
questions or feedback you may have, especially regarding the new 
section. You can also reach out to me on the dlang slack or the D 
community Discord if you'd prefer.

Revised DIP 1038:
https://github.com/pbackus/DIPs/blob/dip1038-final-revisions/DIPs/DIP1038.md

Direct link to new section:
https://github.com/pbackus/DIPs/blob/dip1038-final-revisions/DIPs/DIP1038.md#design-goals-and-possible-alternatives

[1] https://forum.dlang.org/post/rvgdj6$15na$1 digitalmars.com
Feb 22 2021
next sibling parent reply Dukc <ajieskola gmail.com> writes:
On Monday, 22 February 2021 at 12:03:40 UTC, Paul Backus wrote:

 Since it's a substantial addition (almost 50% of the DIP's 
 original length), Mike Parker, the DIP Manager, has graciously 
 allowed me to postpone formal assessment to give the community 
 a chance to look it over, and point out any silly mistakes I 
 may have made.


Shouldn't this mandate another official final review round 
instead (mainly asking Mike here)? Or even another community 
review?
Feb 22 2021
next sibling parent reply Mike Parker <aldacron gmail.com> writes:
On Monday, 22 February 2021 at 12:27:54 UTC, Dukc wrote:


 Shouldn't this mandate another official final review round 
 instead (mainly asking Mike here)? Or even another community 
 review?


No. If the changes affected the details of the proposed feature, 
then yes, I would reboot the process and go back to another 
official community review round. That's not the case here, so 
it's fine to do it informally so as not to drag the process out 
any longer than we need to.
Feb 22 2021
parent reply Dukc <ajieskola gmail.com> writes:
On Monday, 22 February 2021 at 12:59:02 UTC, Mike Parker wrote:

 No. If the changes affected the details of the proposed 
 feature, then yes, I would reboot the process and go back to 
 another official community review round. That's not the case 
 here, so it's fine to do it informally so as not to drag the 
 process out any longer than we need to.


Ok. Did a quick read and started to ponder one thing. The DIP 
claims no breaking changes. If somebody is currently annotating 
something with ` nodiscard` and using it so that the function 
return value is discarded, it means that he must have a local 
definition of ` nodiscard` and that will override the 
`core.attribute` definition, and be no longer recognised by the 
compiler. Is that correct?

Even in that case, there is a theoretical possibility that the 
user-defined ` nodiscard` is annotated in different module than 
where it is used, and the user also happens to import 
`core.attribute`. That would result in an ambiguous symbol.

This is so far-fetched that it'll have no practical signifiance, 
but if we're strict about mentioning all breaking changes it goes 
there.
Feb 22 2021
parent Paul Backus <snarwin gmail.com> writes:
On Monday, 22 February 2021 at 20:12:05 UTC, Dukc wrote:

 Ok. Did a quick read and started to ponder one thing. The DIP 
 claims no breaking changes. If somebody is currently annotating 
 something with ` nodiscard` and using it so that the function 
 return value is discarded, it means that he must have a local 
 definition of ` nodiscard` and that will override the 
 `core.attribute` definition, and be no longer recognised by the 
 compiler. Is that correct?


Yes, the compiler will only recognize `core.attribute.nodiscard`.


 Even in that case, there is a theoretical possibility that the 
 user-defined ` nodiscard` is annotated in different module than 
 where it is used, and the user also happens to import 
 `core.attribute`. That would result in an ambiguous symbol.

 This is so far-fetched that it'll have no practical 
 signifiance, but if we're strict about mentioning all breaking 
 changes it goes there.


This can happen any time a new public symbol is added to a 
library. As far as I'm aware, adding a new public symbol is not 
normally considered a breaking change, so I don't think it's 
necessary to mention it in the DIP. Still, thanks for bringing it 
up--it never hurts to be thorough.
Feb 22 2021
prev sibling parent Paul Backus <snarwin gmail.com> writes:
On Monday, 22 February 2021 at 12:27:54 UTC, Dukc wrote:

 Shouldn't this mandate another official final review round 
 instead (mainly asking Mike here)? Or even another community 
 review?


I asked Mike the same question. Because the new section does not 
change "the core of the proposal," it does not require another 
official review round.

You can read Mike's full reply on Github:

https://github.com/dlang/DIPs/pull/204#issuecomment-783126503
Feb 22 2021
prev sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 2/22/21 7:03 AM, Paul Backus wrote:

 In response to Walter's feedback [1] from the final review round, I've 
 added a new section to DIP 1038 ( nodiscard) on "Design Goals and 
 Possible Alternatives".
 
 Since it's a substantial addition (almost 50% of the DIP's original 
 length), Mike Parker, the DIP Manager, has graciously allowed me to 
 postpone formal assessment to give the community a chance to look it 
 over, and point out any silly mistakes I may have made.
 
 If you'd like to help  nodiscard succeed, I hope you'll give the revised 
 version a read and reply to this thread with any questions or feedback 
 you may have, especially regarding the new section. You can also reach 
 out to me on the dlang slack or the D community Discord if you'd prefer.
 
 Revised DIP 1038:
 https://github.com/pbackus/DIPs/blob/dip1038-final-revisions/DIPs/DIP1038.md 
 
 
 Direct link to new section:
 https://github.com/pbackus/DIPs/blob/dip1038-final-revisions/DIPs/DIP1038.md#design-goals-and-p
ssible-alternatives 
 
 
 [1] https://forum.dlang.org/post/rvgdj6$15na$1 digitalmars.com


This is nice and well argued. The time-tested presence of the attribute 
in C++ is important, probably could use 1-2 paragraphs to emphasize 
that. (It's been a non-standard extension for a long time and clearly 
there was a lot of demand to standardize it.)

For integration, it would be nice if the proposal made pure non-void 
functions automatically nodiscard. That way ignoring the reusult of such 
a function is an error, as it should.

The proposal should mention what happens if two declararions differ only 
by nodiscard:

void* malloc(size_t);
 nodiscard void* malloc(size_t);

I forgot what we usually do. I think it's safe to mark that as as error.

The proposal should mention how  nodiscard works with overriding. Given 
that  nodiscard is more restrictive, by the old OOP principles a 
 nodiscard function should be overridable by a non- nodiscard function. 
(Rationale: overriding functions ASK LESS and RETURN MORE.) However, a 
practicality consideration is that the author of the override simply 
forgot to add  nodiscard, so we can make the attribute equivariant. It's 
important in all cases that a no- nodiscard function cannot be overriden 
by a  nodiscard one.

Great work!!!
Feb 22 2021
parent reply Paul Backus <snarwin gmail.com> writes:
On Monday, 22 February 2021 at 22:01:01 UTC, Andrei Alexandrescu 
wrote:

 This is nice and well argued. The time-tested presence of the 
 attribute in C++ is important, probably could use 1-2 
 paragraphs to emphasize that. (It's been a non-standard 
 extension for a long time and clearly there was a lot of demand 
 to standardize it.)


Are there any well-known [[nodiscard]] success stories that the 
DIP could cite? I have little personal experience with C++, and 
do not know where to look for examples outside of the standard 
library.


 For integration, it would be nice if the proposal made pure 
 non-void functions automatically nodiscard. That way ignoring 
 the reusult of such a function is an error, as it should.


The compiler currently issues a warning when the return value of 
a strongly-pure, nothrow, non-void function is discarded. I agree 
that there is a case to be made that this should be an error 
instead, but I think the appropriate place to make that case is 
in an enhancement request on Bugzilla, not DIP 1038.


 The proposal should mention what happens if two declararions 
 differ only by nodiscard:

 void* malloc(size_t);
  nodiscard void* malloc(size_t);

 I forgot what we usually do. I think it's safe to mark that as 
 as error.


 nodiscard is a user-defined attribute, and the compiler 
currently allows multiple declarations of a function that differ 
only by UDAs. [1] Again, I agree that this should probably be an 
error, but since it is an issue that affects UDAs in general, it 
should be addressed in a Bugzilla issue, not DIP 1038.


 The proposal should mention how  nodiscard works with 
 overriding. Given that  nodiscard is more restrictive, by the 
 old OOP principles a  nodiscard function should be overridable 
 by a non- nodiscard function. (Rationale: overriding functions 
 ASK LESS and RETURN MORE.) However, a practicality 
 consideration is that the author of the override simply forgot 
 to add  nodiscard, so we can make the attribute equivariant. 
 It's important in all cases that a no- nodiscard function 
 cannot be overriden by a  nodiscard one.


 nodiscard is a UDA, and UDAs are not inherited, so applying 
 nodiscard to a base-class method will have no affect on 
derived-class methods that override it.

The way to achieve the outcome you desire here is to apply 
 nodiscard to the method's return type, rather than the method 
itself:

      nodiscard struct Result { int unwrap; }

     class A { Result func() { return Result(1); } }
     class B : A { override Result func() { return Result(2); } }

The idea behind the proposed design is that if you want strong 
guarantees, you should be applying  nodiscard to your types, not 
to your functions. If you are willing to accept weaker guarantees 
in return for greater ease-of-use, that's when you should use 
 nodiscard as a function attribute. (This design is cribbed from 


Of course, the fact we're having this exchange means that the 
above explanation really ought to be in the DIP itself.

Thanks for your comments!

[1] https://run.dlang.io/is/tK14gJ
Feb 22 2021
next sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2021-02-23 00:32, Paul Backus wrote:


  nodiscard is a user-defined attribute, and the compiler currently 
 allows multiple declarations of a function that differ only by UDAs. [1] 
 Again, I agree that this should probably be an error, but since it is an 
 issue that affects UDAs in general, it should be addressed in a Bugzilla 
 issue, not DIP 1038.


I would like to point out that compiler recognized user defined 
attributes were introduced as an alternative to keywords and attributes 
prefixed with ` `. Keywords are reserved words and cannot be used for 
user defined symbols. Attributes prefixed with ` `, i.e. ` property`, 
were introduced as a form of an additonal namespace for keywords. The 
word after ` ` is not reserved so it can be used as user defined 
symbols. After that, user defined attributes were introduced. Even 
though you can have a user defined symbol named `property`, you cannot 
use that as a UDA, because it would cause a conflict with ` property`. 
This makes attributes prefixed with ` ` for or less the same as keywords.

Then compiler recognized UDAs were introduced to solve all of this. 
` selector` is a recognized UDA, but you can still name your symbols 
`selector` and you can still use that as a UDA. If there's a conflict 
with the regular UDA and the compiler recognized UDA, the user can use 
the normal language features to disambiguate the names, i.e. renamed 
imports, static imports, aliases an so on.

That means that the compiler should be free to, more or less, add any 
semantic behavior it wants to a compiler recognized UDA. For example, 
both the ` selector` and the ` optional` compiler recognized UDAs have 
several restrictions and other semantic behavior which a regular UDA 
doesn't have. They can only appear once in a method declaration, they 
can only be attached to methods with Objective-C linkage, they cannot be 
attached to templates, the number of colons in the string passed to 
` selector` need to match the number of parameters. ` optional` affects 
how a class implements an interface.

If you say that you cannot add a specific semantic behavior to a 
compiler recognized UDA because it's a UDA it all falls apart. If you 
don't add any special semantic behavior at **all**, it would just be a 
regular UDA.

-- 
/Jacob Carlborg
Feb 26 2021
parent Paul Backus <snarwin gmail.com> writes:
On Friday, 26 February 2021 at 20:00:52 UTC, Jacob Carlborg wrote:

 On 2021-02-23 00:32, Paul Backus wrote:


  nodiscard is a user-defined attribute, and the compiler 
 currently allows multiple declarations of a function that 
 differ only by UDAs. [1] Again, I agree that this should 
 probably be an error, but since it is an issue that affects 
 UDAs in general, it should be addressed in a Bugzilla issue, 
 not DIP 1038.




[...]

 If you say that you cannot add a specific semantic behavior to 
 a compiler recognized UDA because it's a UDA it all falls 
 apart. If you don't add any special semantic behavior at 
 **all**, it would just be a regular UDA.


You've misread me. I am not saying that I *cannot* add this 
behavior specifically for  nodiscard, I am saying that I *should 
not*, because the current behavior is a bug that affects other 
attributes as well.

Maybe you meant to reply to the part of the post about 
inheritance? Certainly it would be possible to have overrides of 
virtual methods inherit  nodiscard, or at least to forbid them 
from dropping it. The question is whether that's actually a good 
idea.

Personally, I think the increased spec and implementation 
complexity would not be worth the benefit you'd get--especially 
since it would only apply to the weak syntax-level check, which 
is trivial to defeat even if you plug this particular hole.

It's worth noting that [[nodiscard]] is not inherited in C++. [1]

[1] 
https://stackoverflow.com/questions/49576298/are-function-attributes-inherited
Feb 26 2021
prev sibling parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 2/22/21 6:32 PM, Paul Backus wrote:

 On Monday, 22 February 2021 at 22:01:01 UTC, Andrei Alexandrescu wrote:

 This is nice and well argued. The time-tested presence of the 
 attribute in C++ is important, probably could use 1-2 paragraphs to 
 emphasize that. (It's been a non-standard extension for a long time 
 and clearly there was a lot of demand to standardize it.)


 
 Are there any well-known [[nodiscard]] success stories that the DIP 
 could cite? I have little personal experience with C++, and do not know 
 where to look for examples outside of the standard library.


Couldn't find an article specifically discussing a success story, but 
those are not easy to find quickly. One thing good to cite is that 
implementers do deprecate/ignore unsuccessful attributes (e.g. gcc had a 
"returns" attribute for a while indicating that a function returns a 
specific parameter, which was superseded by better RVO implementation in 
the compiler). Au contraire, noreturn was present in all major compilers 
and graduated to standardization. This only happens to useful attributes.

There's even an enhancement to it that you may also want to consider: 
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1301r2.html


 void* malloc(size_t);
  nodiscard void* malloc(size_t);

 I forgot what we usually do. I think it's safe to mark that as as error.


 
  nodiscard is a user-defined attribute, and the compiler currently 
 allows multiple declarations of a function that differ only by UDAs. [1] 
 Again, I agree that this should probably be an error, but since it is an 
 issue that affects UDAs in general, it should be addressed in a Bugzilla 
 issue, not DIP 1038.


Hm, bit of a bummer.


 The way to achieve the outcome you desire here is to apply  nodiscard to 
 the method's return type, rather than the method itself:
 
       nodiscard struct Result { int unwrap; }
 
      class A { Result func() { return Result(1); } }
      class B : A { override Result func() { return Result(2); } }
 
 The idea behind the proposed design is that if you want strong 
 guarantees, you should be applying  nodiscard to your types, not to your 
 functions. If you are willing to accept weaker guarantees in return for 
 greater ease-of-use, that's when you should use  nodiscard as a function 

 which works the same way.)
 
 Of course, the fact we're having this exchange means that the above 
 explanation really ought to be in the DIP itself.


Yah, would be great to include such considerations, and now you've 
already done most of the work.
Feb 27 2021