[PoC] Add @nodiscard attribute #11765

pbackus · 2020-09-19T02:42:10Z

Edit: This PR was based on an early draft version of DIP 1038. It has been superseded by #13589. The original PR description follows below.

Fixes issue 5464 - Attribute to not ignore function result

Still need to write up a DIP for this, but I figured I'd post it early to get feedback. The design is the same as Rust's #[must_use]: on a function, it applies to the return value; on a type, it applies to all expressions of that type.

dlang-bot · 2020-09-19T02:42:12Z

Thanks for your pull request and interest in making D better, @pbackus! 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 coverage diff by visiting the details link of the codecov check)
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
✓	5464	enhancement	Attribute to not ignore function result

Testing this PR locally

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

dub run digger -- build "master + dmd#11765"

TurkeyMan · 2020-09-19T03:17:04Z

It seems to me that pure functions should implicitly have this attribute... unless a separate warning to tell you've discarded a pure functions result already exists?

pbackus · 2020-09-19T03:29:24Z

@TurkeyMan Yes, the compiler already gives a warning (not an error) for discarding the result of a pure function.

zopsicle · 2020-09-19T09:09:05Z

What is the effect of using this attribute on a constructor? If none, may want to prohibit there.

pbackus · 2020-09-19T11:19:46Z

@chloekek Good question. It does work on constructors, because they return a reference to the constructed object. That's not really documented in the language spec, though (closest thing is a mention of constructors' return type under "Ref Return Scope Parameters"), so I should probably mention constructors explicitly in the DIP.

Fixes issue 5464 - Attribute to not ignore function result

RazvanN7

Mind the comments. Also, this need to be tested more thoroughly:

for BinAssignExps
for aliases to nodiscard types and functions
for overloaded @NODISCARD functions
for @NODISCARD member functions
for @NODISCARD overriden functions (how do you override a nodiscard function?)
@NODISCARD auto functions
@NODISCARD templated functions

RazvanN7 · 2021-01-08T10:54:26Z

src/dmd/parse.d

@@ -1444,6 +1444,8 @@ final class Parser(AST) : Lexer
            stc = isBuiltinAtAttribute(token.ident);
            if (!stc)
            {
+                if (token.ident == Id.nodiscard)
+                    deprecation("use of `@nodiscard` as a user-defined attribute is deprecated.");


The DIP should be amended to contain this deprecation.

The DIP is correct; this PR is based on a previous revision and needs to be updated.

src/dmd/parse.d

RazvanN7 · 2021-01-08T11:05:26Z

src/dmd/sideeffect.d

+    scope IsAssignment v = new IsAssignment();
+    e.accept(v);
+    return v.result;
+}


This function would be more efficient if it would simply be replaced by an if statement:

private bool isAssignmentExp(Expression e) @safe pure @nogc nothrow { return e.isAssignExp || e.isBinAssignExp; }

RazvanN7 · 2021-01-08T11:10:54Z

src/dmd/sideeffect.d

+        return false;
+    if (auto ce = e.isCallExp())
+    {
+        if (ce.f && (ce.f.storage_class & STC.nodiscard)) {


Curly brace on the next line

RazvanN7 · 2021-01-08T11:11:07Z

src/dmd/sideeffect.d

+    {
+        auto sym = e.type.toDsymbol(null);
+        auto ad = sym ? sym.isAggregateDeclaration() : null;
+        if (ad && (ad.storage_class & STC.nodiscard) != 0) {


Curly brace on the next line

RazvanN7 · 2021-01-08T11:15:02Z

src/dmd/sideeffect.d

+/**
+ * Check whether the e is a call to a @nodiscard function, or is
+ * a non-assignment expression with @nodiscard type.
+ */


The comment should specify that this function issues an error in the above mentioned cases.

Also, this function is called "isNodiscard", therefore the expectation would be that it simply verifies if an expression contains an @NODISCARD expression. However, this function does more than that: it also errors if the expression is nodiscard. I suggest you rename the function to checkNodiscard. This implies that an error might be issued.

pbackus · 2022-01-28T21:22:26Z

Since the final accepted version of DIP 1038 has diverged significantly from the draft this PR was based on, I am closing it. DIP 1038 will be implemented in a new PR.

Thank you, @RazvanN7, for your review—I plan to incorporate your feedback into the final implementation.

pbackus requested review from ibuclaw, kinke, RazvanN7, UplinkCoder and WalterBright as code owners September 19, 2020 02:42

dlang-bot added the Severity:Enhancement label Sep 19, 2020

pbackus force-pushed the nodiscard-attribute branch from 9cd94d2 to a943388 Compare September 19, 2020 02:53

Add @NODISCARD attribute

0101bc9

Fixes issue 5464 - Attribute to not ignore function result

pbackus force-pushed the nodiscard-attribute branch from a943388 to 0101bc9 Compare September 26, 2020 20:13

thewilsonator added Review:Needs Changelog A changelog entry needs to be added to /changelog Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org labels Sep 27, 2020

Add -preview=nodiscard flag to enable @NODISCARD

2c1bf7c

RazvanN7 added the Review:Pending DIP Approval label Jan 8, 2021

RazvanN7 requested changes Jan 8, 2021

View reviewed changes

RazvanN7 added the Review:Needs Rebase label Jan 8, 2021

pbackus marked this pull request as draft January 8, 2021 15:57

dlang-bot added the Review:stalled label May 20, 2021

pbackus closed this Jan 28, 2022

pbackus mentioned this pull request Jan 31, 2022

Implement DIP 1038: @mustuse #13589

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PoC] Add @nodiscard attribute #11765

[PoC] Add @nodiscard attribute #11765

pbackus commented Sep 19, 2020 •

edited

Loading

dlang-bot commented Sep 19, 2020 •

edited

Loading

TurkeyMan commented Sep 19, 2020

pbackus commented Sep 19, 2020

zopsicle commented Sep 19, 2020

pbackus commented Sep 19, 2020

RazvanN7 left a comment

RazvanN7 Jan 8, 2021

pbackus Jan 8, 2021

RazvanN7 Jan 8, 2021

RazvanN7 Jan 8, 2021

RazvanN7 Jan 8, 2021

RazvanN7 Jan 8, 2021

pbackus commented Jan 28, 2022

[PoC] Add @nodiscard attribute #11765

[PoC] Add @nodiscard attribute #11765

Conversation

pbackus commented Sep 19, 2020 • edited Loading

dlang-bot commented Sep 19, 2020 • edited Loading

Bugzilla references

Testing this PR locally

TurkeyMan commented Sep 19, 2020

pbackus commented Sep 19, 2020

zopsicle commented Sep 19, 2020

pbackus commented Sep 19, 2020

RazvanN7 left a comment

Choose a reason for hiding this comment

RazvanN7 Jan 8, 2021

Choose a reason for hiding this comment

pbackus Jan 8, 2021

Choose a reason for hiding this comment

RazvanN7 Jan 8, 2021

Choose a reason for hiding this comment

RazvanN7 Jan 8, 2021

Choose a reason for hiding this comment

RazvanN7 Jan 8, 2021

Choose a reason for hiding this comment

RazvanN7 Jan 8, 2021

Choose a reason for hiding this comment

pbackus commented Jan 28, 2022

pbackus commented Sep 19, 2020 •

edited

Loading

dlang-bot commented Sep 19, 2020 •

edited

Loading