Implement DIP 1038: @mustuse #13589

pbackus · 2022-01-30T20:44:59Z

Supersedes #11765.

dlang-bot · 2022-01-30T20:45:01Z

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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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

MoonlightSentinel

Rough first review, looks good overal.

Some missing bits:

spec PR
changelog

src/dmd/must_use.d

MoonlightSentinel · 2022-01-30T21:30:35Z

test/compilable/must_use_not_reserved.d

+import core.attribute;
+
+@mustUse int n;
+@mustUse alias A = int;


Shouldn't this be a warning / error? Seems like an error that is very easy to do and silently allows broken code.

This is correct per the formal spec in DIP 1038, and is consistent with how other attributes work (e.g., @nogc).

This is correct per the formal spec in DIP 1038,

The "Diagnostics" section (and the DIP in general) doesn't define rules for alias declarations - unless the absence is to be interpreted as "no diagnostics" instead of "not considered".

and is consistent with how other attributes work (e.g., @nogc).

That's only partially true. E.g. the @nogc attribute is discarded but misuse usually results in a subsequent error because @nogc code tries to call a non-@nogc function pointer / delegate.

The relevant rule, from the "Formal Specification" section, is:

It is a compile-time error to discard an expression if all of the following are true:
[...]

its type is a struct or union type whose declaration is annotated with @mustUse.

Whether that type is referred to via an alias or directly, it is the attribute on the type itself that matters.

Indeed but that also assumes knowledge of the wording in the DIP. The current behavior might not be as obvious for common users.

Whether @mustUse alias ... should raise additional diagnostics is not defined / forbidden by the DIP. The current implementation already raises errors when annotating e.g. functions with @mustUse for the same reason - it's something a user would do but isn't supported and would have no effect.

Indeed but that also assumes knowledge of the wording in the DIP.

The wording from the DIP's "Formal Specification" section will be incorporated into the language spec.

Whether @mustUse alias ... should raise additional diagnostics is not defined / forbidden by the DIP.

The compiler is, of course, free to issue additional diagnostics above and beyond what the spec requires.

As a general rule, DMD does not currently issue diagnostics for attributes used on declarations that do not support them. If we wish to revisit that policy, fine, but I do not think this PR is the place for it.

The current implementation already raises errors when annotating e.g. functions with @mustUse for the same reason - it's not supported and would have no effect.

@mustUse on functions is specifically reserved by the DIP for potential future expansion. @mustUse on aliases is not reserved.

I agree with @pbackus. The DIP specifically states: "It is a compile-time error to annotate any function declaration or aggregate declaration other than a struct or union declaration with the @mustuse attribute. The purpose of this rule is to reserve such usage for possible future expansion.". So the implementation correctly covers the spec.

On the topic of ignoring attributes, this comes up once in a while. The general consensus is that we should ignore them because you also have the possibility to use attributes on-masse (e.g@safe:). If you issue an error for all the declarations for which the attribute doesn't make sense then you are basically making en-masse attributes unusable.

src/dmd/must_use.d

src/dmd/expressionsem.d

pbackus · 2022-01-30T23:49:56Z

@MoonlightSentinel Thanks for the review! I've pushed some new commits addressing your comments.

pbackus · 2022-01-31T15:03:10Z

Spec PR: dlang/dlang.org#3201

RazvanN7

LGTM

RazvanN7 · 2022-02-01T15:43:33Z

cc @WalterBright

pbackus · 2022-02-06T22:46:56Z

If I understand the DIP process correctly, this PR should not require @WalterBright's direct approval as long as reviewers are satisfied that it correctly implements DIP 1038. Perhaps @mdparker can clarify.

mdparker · 2022-02-07T01:27:34Z

I have nothing to do with the implementation side, and that is separate from the DIP process. That falls under Razvan's purview as PR manager.

RazvanN7 · 2022-02-07T08:47:27Z

To the best of my knowledge, this PR correctly implements DIP1038, however, I would feel more comfortable to merge this if @WalterBright would give his approval. However, since Walter is very busy with importC he might not want to be sidetracked this, therefore, I will wait for another 72h before merging this.

src/dmd/must_use.d

WalterBright · 2022-02-07T23:10:57Z

src/dmd/must_use.d

+                auto argExp = (*tiargs)[0].isExpression();
+                auto op = argExp ? argExp.isStringExp() : null;
+                scope plusPlus = new StringExp(Loc.initial, "++");
+                scope minusMinus = new StringExp(Loc.initial, "--");


I'm concerned these allocations will become a major performance problem and memory consumption problem, as they are called for a large fraction of all the expression nodes.

Changed from scope to static const.

src/dmd/must_use.d

WalterBright · 2022-02-07T23:14:01Z

src/dmd/must_use.d

+        // e.g., (auto tmp = a, ++a, tmp)
+        auto left = comma.e1 ? comma.e1.isCommaExp() : null;
+        auto middle = left ? left.e2 : null;
+        if (middle && isIncrementOrDecrement(middle))


These three lines have multiple redundant tests.

I'm afraid it's not clear to me which tests here are redundant. By my count, there are 3, and all test separate pointers:

comma.e1

comma.e1.isCommaExp() (left)

comma.e1.isCommaExp().e2 (middle)

Is there some undocumented invariant that would allow one or more of these tests to be skipped? (E.g., "e1 and e2 of a CommaExp are never null".)

!comma.e1 is tested which then sets left to null, which is then tested for null to set middle, then middle is tested for null.

For example, if comma.e1 is false, none of the rest of the code needs to be executed.

src/dmd/common/string.d

WalterBright · 2022-02-07T23:17:29Z

src/dmd/must_use.d

+        auto fd = ce.f;
+        auto id = fd ? fd.ident : null;
+        // opXXX are reserved identifiers, so it's ok to match too much here
+        if (id && id.toString().startsWith("op") && id.toString().endsWith("Assign"))


I'm concerned about the performance implications of doing this an astounding number of times.

Changed to == comparison with Id.assign and friends.

pbackus · 2022-04-06T01:33:43Z

Rebased to resolve merge conflicts.

pbackus · 2022-04-06T03:59:11Z

Ping @WalterBright @dkorpel @RazvanN7

dkorpel · 2022-04-06T07:45:46Z

The files src/dmd/expressionsem.d.orig and src/dmd/statementsem.d.orig look accidentally committed.

pbackus · 2022-04-06T15:55:59Z

Whoops; good catch. They're gone now.

dkorpel · 2022-04-06T16:01:03Z

good catch.

Kind of hard not to notice when the diff says +18,000 lines 🙂

I've e-mail Walter asking him to either re-review, or trust Razvan and me to approve this. If he doesn't respond, I'll bring it up in the foundation meeting Friday.

ghost · 2022-04-06T16:39:45Z

I'd suggest to merge all the compilable tests in a single file

pbackus · 2022-04-06T16:50:27Z

@SixthDot The rationale behind splitting them is that shorter tests are easier to work with when you're running the compiler in a debugger.

ghost · 2022-04-06T16:56:03Z

I'll not insist much since this is just a detail but but as counter argument: you have plenty of option in case you would need to debug a bigger test file

p stuff.loc
comment code
version(all) / version(none)

pbackus · 2022-04-06T17:29:58Z

@SixthDot Yes, that's exactly my point: with a short test file, you don't need to do any of that. So short tests are easier to work with.

Can you articulate a compelling benefit of combining the tests, which outweighs the benefit of splitting them?

maxhaton · 2022-04-06T18:18:47Z

Merging runnable tests is usually a good thing but for compilables the overhead of the files isn't very high unless you're doing them all serially so I prefer separate test files.

pbackus marked this pull request as ready for review January 30, 2022 20:59

pbackus force-pushed the implement-dip1038 branch from 6533eaf to 442a872 Compare January 30, 2022 20:59

pbackus requested a review from ibuclaw as a code owner January 30, 2022 21:25

MoonlightSentinel reviewed Jan 30, 2022

View reviewed changes

This was referenced Jan 31, 2022

[PoC] Add @nodiscard attribute #11765

Closed

Add @mustuse (DIP 1038) to the language spec dlang/dlang.org#3201

Merged

RazvanN7 approved these changes Feb 1, 2022

View reviewed changes

RazvanN7 added the Walter Bright label Feb 1, 2022

RazvanN7 added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Feb 7, 2022

maxhaton approved these changes Feb 7, 2022

View reviewed changes