Fix issue 20704 - -preview=rvaluerefparam does not work with init as default parameter #12413

nordlow · 2021-04-09T13:56:00Z

This PR fixes build errors with defaulted ref parameters in general, not just the ref parameters defaulted to T.init.

dlang-bot · 2021-04-09T13:56:03Z

Thanks for your pull request and interest in making D better, @nordlow! 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
✓	20704	normal	`\-preview=rvaluerefparam` does not work with `init` as default parameter

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

thewilsonator · 2021-04-09T13:58:03Z

tests?

nordlow · 2021-04-09T14:02:29Z

tests?

Done

thewilsonator · 2021-04-09T14:53:04Z

please include the description of the issue in the commit message so it shows up in the log

nordlow · 2021-04-09T14:59:37Z

please include the description of the issue in the commit message so it shows up in the log

In this case the title doesn't quite match what is being corrected. The correction in this PR involves default parameters in general.

thewilsonator · 2021-04-09T15:03:58Z

In which case the test case should test for more that just T.init

nordlow · 2021-04-09T19:41:31Z

In which case the test case should test for more that just T.init

Are you satisfied with compilable tests now, @thewilsonator?

thewilsonator · 2021-04-09T23:58:21Z

src/dmd/typesem.d

            if (isRefOrOut && !isAuto &&
-                !(global.params.previewIn && (fparam.storageClass & STC.in_)))
+                !(global.params.previewIn && (fparam.storageClass & STC.in_)) &&
+                !(global.params.rvalueRefParam))
                e = e.toLvalue(sc, e);


Actually could you change this to

if (isRefOrOut && !isAuto &&) { if ((!(global.params.previewIn && (fparam.storageClass & STC.in_))) || !(global.params.rvalueRefParam)) { e = e.toLvalue(sc, e); if (e.op == TOK.error) errorSupplimental(e.loc, "use `-preview=in` or `preview=rvaluerefparam`" } }

note || not &&

I don't see the logic in using || instead of &&. Using || makes my compilable test fail.

I've updated the condition accordingly with the exception of leaving the above mentioned && unchanged. Along with and updated expected message for fail_compilation.

thewilsonator · 2021-04-10T00:55:18Z

also cc @Geod24 as this touches -preview=in to double check

nordlow · 2021-04-11T00:02:24Z

When this is merged what's keeping us from activating -preview=rvaluerefparam by default?

thewilsonator · 2021-04-11T00:36:24Z

Note you will also have to update the error messages in

- fail_compilation/fail7603a.d
- fail_compilation/fail7603b.d
- fail_compilation/fail7603c.d
- fail_compilation/fail9773.d
- fail_compilation/fail9891.d
- ```

nordlow · 2021-04-11T15:57:38Z

Note you will also have to update the error messages in

- fail_compilation/fail7603a.d
- fail_compilation/fail7603b.d
- fail_compilation/fail7603c.d
- fail_compilation/fail9773.d
- fail_compilation/fail9891.d
- ```

Thanks. Done.

nordlow · 2021-04-11T17:15:09Z

Are we satisfied with error message struct and class types being

`S(0)` is not an lvalue and cannot be modified
`null` is not an lvalue and cannot be modified

respectively?

If so, ok to modify error message

cannot modify constant `0`

to

`0` is not an lvalue and cannot be modified

?

If we do this I believe we should modify all other messages beginning with "cannot modify aswell for consistency. I'm not sure whether that's a good idea or not. Maybe the message needs to be adjusted just in the context of being passed as a default value to a const ref parameter.

Geod24 · 2021-04-12T06:00:20Z

When this is merged what's keeping us from activating -preview=rvaluerefparam by default?

Probably the fact that the DIP was rejected. IMO this switch should be removed but ALAS @TurkeyMan uses it.

Geod24 · 2021-04-12T06:04:55Z

src/dmd/typesem.d

+                    if (e.op == TOK.error)
+                        errorSupplemental(e.loc, "use `-preview=in` or `preview=rvaluerefparam`");


Don't think we should recommend those -preview switch as neither have been approved by our BDFLs.

Ok with me to remove it. What's the DIP-number for it? And why was this added to dmd if it was rejected? Is there a plan for a substitute DIP?

I'd rather leave this in.

https://github.com/dlang/DIPs/blob/master/DIPs/rejected/DIP1016.md

reject because

void fun(ref int x); fun(10);

which is disallowed by the current implementation. The current implementation was done because @TurkeyMan quite rightly complained that the review process was not good and that there is utility in the feature.

https://github.com/dlang/DIPs/blob/master/DIPs/rejected/DIP1016.md#formal-assessment

@thewilsonator : I would appreciate if you gave me the time to reply to this. Whether or not you or someone else feels the review process wasn't good doesn't change the fact that the BDFLs decision have been expressed clearly and this is somewhat going against it.

And regarding -preview=in, as much as I want to recommend it to people I will not do it through the compiler as long as we haven't reached a consensus on it.

Is the problem the fact that mutable references also can be passed rvalues? Like in

struct S {} void e(const ref int x) {} void f(ref int x) {} void fS(ref S x) {} void g(const ref shared long x) {} unittest { e(10); // ok f(10); // ok with -preview=rvaluerefparam, error otherwise fS(S()); // ok with -preview=rvaluerefparam, error otherwise g(10L); // always error }

. Why not, for now, disallow a mutable reference parameter from being passed an r-value? C++ does this; for instance

void f(int& x) { x = 3; } int main() { f(3); }

fails as

<stdin>:2:16: error: cannot bind non-const lvalue reference of type ‘int&’ to an rvalue of type ‘int’ <stdin>:1:13: note: initializing argument 1 of ‘void f(int&)’

Ping, @Geod24

@nordlow : I submitted #12425
As I explain there, we shouldn't recommend preview any switches which were completely broken until last release, nor should we start recommending switches for which there is no clear plan.

I agree. It doesn't seem to be that difficult to adjust the corrnercases for -refvaluerefparam and make it activate by default. This is such a cornercase.

Fix issue 20704

9a20424

dlang-bot added the Severity:Bug Fix label Apr 9, 2021

thewilsonator added the Review:Needs Tests label Apr 9, 2021

Add test

7037ca7

thewilsonator changed the title ~~Fix issue 20704~~ Fix issue 20704 - -preview=rvaluerefparam does not work with init as default parameter Apr 9, 2021

nordlow added 2 commits April 9, 2021 21:32

More extensive tests

1ca2894

Add tests for struct and class aswell

5f4abeb

Add failing compilation test

c9cb2c1

thewilsonator reviewed Apr 9, 2021

View reviewed changes

nordlow added 2 commits April 11, 2021 01:23

Update condition that triggers toLvalue

11fbc2e

Update condition again and adjust test

da138ae

thewilsonator removed the Review:Needs Tests label Apr 11, 2021

Update fail_compilation tests

2db018e

Adjust failing messages once more, this time adjusting line numbers

b9457dd

Geod24 reviewed Apr 12, 2021

View reviewed changes

thewilsonator merged commit b78fe97 into dlang:master Apr 12, 2021

Geod24 mentioned this pull request Apr 12, 2021

Do not recommend un-approved preview switches #12425

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue 20704 - -preview=rvaluerefparam does not work with init as default parameter #12413

Fix issue 20704 - -preview=rvaluerefparam does not work with init as default parameter #12413

nordlow commented Apr 9, 2021 •

edited

Loading

dlang-bot commented Apr 9, 2021

thewilsonator commented Apr 9, 2021

nordlow commented Apr 9, 2021

thewilsonator commented Apr 9, 2021

nordlow commented Apr 9, 2021

thewilsonator commented Apr 9, 2021

nordlow commented Apr 9, 2021 •

edited

Loading

thewilsonator Apr 9, 2021

nordlow Apr 10, 2021 •

edited

Loading

nordlow Apr 10, 2021 •

edited

Loading

thewilsonator commented Apr 10, 2021

nordlow commented Apr 11, 2021 •

edited

Loading

thewilsonator commented Apr 11, 2021

nordlow commented Apr 11, 2021 •

edited

Loading

nordlow commented Apr 11, 2021 •

edited

Loading

Geod24 commented Apr 12, 2021

Geod24 Apr 12, 2021

nordlow Apr 12, 2021 •

edited

Loading

thewilsonator Apr 12, 2021

thewilsonator Apr 12, 2021

Geod24 Apr 12, 2021

nordlow Apr 12, 2021 •

edited

Loading

Geod24 Apr 12, 2021

nordlow Apr 13, 2021

		if (e.op == TOK.error)
		errorSupplemental(e.loc, "use `-preview=in` or `preview=rvaluerefparam`");

Fix issue 20704 - -preview=rvaluerefparam does not work with init as default parameter #12413

Fix issue 20704 - -preview=rvaluerefparam does not work with init as default parameter #12413

Conversation

nordlow commented Apr 9, 2021 • edited Loading

dlang-bot commented Apr 9, 2021

Bugzilla references

Testing this PR locally

thewilsonator commented Apr 9, 2021

nordlow commented Apr 9, 2021

thewilsonator commented Apr 9, 2021

nordlow commented Apr 9, 2021

thewilsonator commented Apr 9, 2021

nordlow commented Apr 9, 2021 • edited Loading

thewilsonator Apr 9, 2021

Choose a reason for hiding this comment

nordlow Apr 10, 2021 • edited Loading

Choose a reason for hiding this comment

nordlow Apr 10, 2021 • edited Loading

Choose a reason for hiding this comment

thewilsonator commented Apr 10, 2021

nordlow commented Apr 11, 2021 • edited Loading

thewilsonator commented Apr 11, 2021

nordlow commented Apr 11, 2021 • edited Loading

nordlow commented Apr 11, 2021 • edited Loading

Geod24 commented Apr 12, 2021

Geod24 Apr 12, 2021

Choose a reason for hiding this comment

nordlow Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

thewilsonator Apr 12, 2021

Choose a reason for hiding this comment

thewilsonator Apr 12, 2021

Choose a reason for hiding this comment

Geod24 Apr 12, 2021

Choose a reason for hiding this comment

nordlow Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

Geod24 Apr 12, 2021

Choose a reason for hiding this comment

nordlow Apr 13, 2021

Choose a reason for hiding this comment

nordlow commented Apr 9, 2021 •

edited

Loading

nordlow commented Apr 9, 2021 •

edited

Loading

nordlow Apr 10, 2021 •

edited

Loading

nordlow Apr 10, 2021 •

edited

Loading

nordlow commented Apr 11, 2021 •

edited

Loading

nordlow commented Apr 11, 2021 •

edited

Loading

nordlow commented Apr 11, 2021 •

edited

Loading

nordlow Apr 12, 2021 •

edited

Loading

nordlow Apr 12, 2021 •

edited

Loading