This is the feedback thread for the Final Review of DIP 1038, "@nodiscard".

===================================
**THIS IS NOT A DISCUSSION THREAD**

Posts in this thread must adhere to the feedback thread rules outlined in the Reviewer Guidelines (and listed at the bottom of this post).

https://github.com/dlang/DIPs/blob/master/docs/guidelines-reviewers.md

That document also provides guidelines on contributing feedback to a DIP review. Please read it before posting here. If you would like to discuss this DIP, please do so in the discussion thread:

https://forum.dlang.org/post/nfyfsmenjwzhbezffori@forum.dlang.org

==================================

You can find DIP 1038 here:

https://github.com/dlang/DIPs/blob/ab056150975a9a8db5b5da3dbffdd81529802a49/DIPs/DIP1038.md

The review period will end at 11:59 PM ET on February 17, or when I make a post declaring it complete. Feedback posted to this thread after that point may be ignored.

At the end of the Final Review, the DIP will be forwarded to the language maintainers for Formal Assessment.

==================================
Posts in this thread that do not adhere to the following rules will be deleted at the DIP manager's discretion:

* All posts must be a direct reply to the DIP manager's initial post, with only two exceptions:

    - Any commenter may reply to their own posts to retract feedback contained in the original post

    - The DIP author may (and is encouraged to) reply to any feedback solely to acknowledge the feedback with agreement or disagreement (preferably with supporting reasons in the latter case)

* Feedback must be actionable, i.e., there must be some action the DIP author can choose to take in response to the feedback, such as changing details, adding new information, or even retracting the proposal.

* Feedback related to the merits of the proposal rather than to the contents of the DIP (e.g., "I'm against this DIP.") is allowed in Community Review, but not in Final Review, and must be backed by supporting arguments (e.g., "I'm against this DIP because..."). The supporting arguments must be reasonable. Obviously frivolous arguments waste everyone's time.

* Feedback should be clear and concise, preferably listed as bullet points (those who take the time to do an in-depth review and provide feedback in the form of answers to the questions in the reviewer guidelines will receive much gratitude). Information that is irrelevant to the DIP or is not provided in service of clarifying the feedback is unwelcome.

1. > Note that the former is a syntax-level check, while the latter is a type-level check. This means that the value returned from a @nodiscard function may in fact be discarded as long as the function call itself is enclosed in some other expression.

These two sentences do not make sense together. The second sentence only refers to the syntax-level check.


2. Consider:

    @nodiscard int foo();
    a ? b : foo();

discards the return result of foo(), but according to the DIP no error will be detected. This is another case of the comma expression one mentioned in the Description.

This is a critical shortcoming in the DIP that needs to be addressed. Perhaps this feature in other languages can provide guidance.

On Thursday, 4 February 2021 at 09:09:02 UTC, Walter Bright wrote:
> 1. > Note that the former is a syntax-level check, while the latter is a type-level check. This means that the value returned from a @nodiscard function may in fact be discarded as long as the function call itself is enclosed in some other expression.
>
> These two sentences do not make sense together. The second sentence only refers to the syntax-level check.

I intended "a @nodiscard function" to refer only to the syntax-level check, but I can see how it can be read as ambiguous. A clearer way to phrase it would be "a function annotated with @nodiscard."

> 2. Consider:
>
>     @nodiscard int foo();
>     a ? b : foo();
>
> discards the return result of foo(), but according to the DIP no error will be detected. This is another case of the comma expression one mentioned in the Description.
>
> This is a critical shortcoming in the DIP that needs to be addressed. Perhaps this feature in other languages can provide guidance.

This is exactly how the feature works in GCC, Clang, C++17, and Rust.

We have a name for information about an expression that propagates, transitively, from sub-expressions to super-expressions the way you suggest @nodiscard ought to propagate. We call it the expression's "type." So what you are suggesting here, in effect, is that @nodiscard should become a type qualifier, and that we should write

    @nodiscard(int) foo();

While this would indeed detect an error in cases like the one you give above, there are second-order effects to making `T` and `@nodiscard(T)` distinct types. For example:

    @nodiscard(int) foo();
    int bar()
    auto a = foo();
    auto b = bar();
    b = a; // Error: cannot implicitly convert `@nodiscard(int)` to `int`

This kind of error is obviously not going to help anybody catch bugs in their code--it is pure noise.

If the syntax-level check is deemed inadequate, I think the best way to proceed is to simply remove it from the DIP and include only the type-level check (this is what Rust did, originally). But I think it's important to keep in mind that there is no perfect solution here, only tradeoffs.

On Wednesday, 3 February 2021 at 11:28:31 UTC, Mike Parker wrote:
> Posts in this thread that do not adhere to the following rules will be deleted at the DIP manager's discretion:

Once again, I would like to remind everyone that I will not be replying to posts in this thread that do not follow the Feedback Thread rules.