Passing rvalues to const ref args #111

TurkeyMan · 2018-03-25T21:20:36Z

Created a DIP for the rval->ref situation

wilzbach · 2018-03-25T21:28:55Z

DIPs/DIP1xxx-rval_to_ref.md

+There are many reasons why every function can't or shouldn't be a template.
+1. API is not your code, and it's not alrady `auto ref`
+2. Is distributed as a binary lib
+3. Is expored from DLL


typo: exported

Did fix a couple of typos

TurkeyMan · 2018-03-25T23:18:14Z

Updated to clarify that only one scope should be introduced for the entire statement (clarifying behaviour for cascading calls), and that return ref is satisfied by this implementation.

TurkeyMan · 2018-03-26T02:13:43Z

I could perhaps be persuaded to remove the word 'const' from this proposal... but I'd prefer to see that as a separate follow-up debate (relaxation of the spec is always easier than tightening). I don't really feel that symptoms of the restrictiveness of const are on trial here.

skl131313 · 2018-03-26T02:30:26Z

Possibly add a bit about default parameters:

extern(C++) void foo(ref const(int) = 0)
{
}

Part of the problem when writing interfaces to C++ is that const& can have default parameters set to the function. But with D you can't do that, in the event that the function is nice and doesn't have T* const parameters in the function definition, you now have to create two different definitions of the same function. One for the interface and another to implement the default parameters.

What gets called when? For the following:

void foo(int);
void foo(const(int));
void foo(ref int);
void foo(ref const(int));

Didn't see anything about this in there either, though I honestly don't know when the second definition would even be called?

TurkeyMan · 2018-03-26T04:09:29Z

Nice points!

wilzbach · 2018-03-31T01:06:44Z

DIPs/DIP1xxx-rval_to_ref.md

+This issue is also likely to appear more frequently for vendors with tight ABI requirements.
+Users of closed-source libraries distributed as binary libs, or libraries distributes as DLLs are more likely to encounter these challenges interacting with those APIs as well.
+
+Another high-probability occurrence is OOP, where virtual function APIs inhibit the use of templates.


Another good use case is random generation. You always should take the generator by ref to avoid accidentally copying the generator (oops), but that makes the API more verbose as the user can't declare the RandomEngine and use it one line.
Phobos uses auto ref- it shouldn't :/
A summary of Phobos's broken std.random: https://dconf.org/2015/talks/wakeling.html

mdparker · 2018-04-02T04:57:49Z

@TurkeyMan As per the new procedures, I'm setting this to DIP to Draft status. I'll also put out a call for Draft Review on the forums.

mdparker · 2018-04-02T05:06:58Z

I recommend adding a link to the ongoing forum discussion in the References section.

schveiguy · 2018-04-02T15:13:15Z

"because it's a guarantee that the parameter is used strictly as an input argument, rather than some form of output."

Consider a reference inside such a type as being a potentially sound place for an output. In other words, the struct itself is a reference to a temporary, but what its members point at may not be temporary. In C++ this is fine, because head-const does exist. But in D, requiring transitive const has a much more far-reaching effect.

In any case, there is a large missing discussion in this DIP: the fact that this is passed by reference, even for rvalues. Apparently, the D world hasn't yet ended due to that loose interpretation of the rules, even without const, so maybe it's OK for other uses!

Bolpat · 2018-04-17T09:05:18Z

Your proposal does not only break current code, but current expression, because someone might want to express take it by ref, but only lvalues, which as far as I see, cannot be done anymore. If so, it must be in the document.
I've made a suggestion[1] in the Forum to reuse the in storage class for that. This way, one can still express take it by ref, but only lvalues. The in storage class is completely redundant, so deprecating it has a trivial migration to replace it by const scope. That's not the big deal. The aforementioned is not affected. Most programmers will not need const scope ref anyway.

So, how about redefining in to mean "const scope ref that also binds rvalues"?

The compiler can even suggest changing const scope ref to in when you plug in an rvalue for a const ref. Generally, scope should be part of all that: You propose an efficient (l/r)value-agnostic storage class for input parameters. Those should not be leaked.

[1] https://forum.dlang.org/post/medovwjuykzpstnwbfyy@forum.dlang.org

Bolpat · 2018-04-17T11:15:15Z

About the lowering, for R func(const ref T arg) and some expression expr resulting in an rvalue,
isn't (param => func(param))(expr()) the right lowering?

TurkeyMan · 2018-04-17T17:28:43Z

Sorry for the silence guys. Work got very busy, and I haven't had any time after-hours for the last couple of weeks.
I'll integrate all the recent feedback as soon as I am able. PR's welcome too, if there are others who are motivated by this issue! :)

TurkeyMan · 2018-04-17T17:34:11Z

isn't (param => func(param))(expression()) the right lowering?

My proposal is more semantically direct. The syntax you present introduces a function, which relies on the inliner to perform as expected to produce the proper calling code equivalent to my proposal. That will not happen in debug, and as such, the code may be hard to inspect (additional call in the callstack obscuring outer locals) and debug/step.

Bolpat · 2018-04-26T13:08:00Z

About the lambda: I've seen stuff like that in other DIPs. I believe it's being used to specify behavior rigorously and rather simple and let the compiler guys decide on the implementation details.

TurkeyMan · 2018-04-26T17:34:03Z

It's not right though; i made a point about introducing a single scope, not one for each call. Otherwise chains using return ref won't work as you expect. All temps need to live the life of the statement, not the call. (calls can be nested)

Bolpat · 2018-05-21T17:42:56Z

It took me some time to understand how your proposal changes how ref should be looked at with respect to one of its implications. I thought of ref mainly as lvalues-only restriction and secondary as how a parameter is being passed (optimization).

You should mention that with your proposal, ref cannot be used as a restriction any more. It is still possible to implement the restriction as lvalues still prefer ref overloads and rvalues still prefer non-ref overloads:

void f(T) @disable; // catch and reject rvalues ...
void f(ref T); // ... therefore lvalues only

void g(ref T) @disable; // catch and reject lvalues ...
void g(T); // ... therefore rvalues only

One (good or bad depends on viewpoint) implication is that restriction to lvalues must be done much more explicit than before. Possibly there are algorithms that are not useful or don't even work correctly on l- or rvalues, but the assumption that those algorithms are rare certainly holds.

So the proposal makes ref more like C#'s ref[1] and VB.NET's ByRef than C++'s &. I believe that many D folks read D's ref similar to C++'s & (which without const means lvalues only[2]) with the only difference that it can only be used on function parameters and returns (NB: not function parameter types or return types, as ref is not a type constructor).

[1] jonskeet.uk or docs.microsoft.com (apart from ref locals)
[2] For a very explicit version of that meaning, see ref-qualified non-static member functions on cppreference.com § const-, volatile-, and ref-qualified member functions

TurkeyMan · 2018-05-21T17:47:10Z

Right, I had that thought about retaining the restriction. If people want that, they can use @disable.
I'll add that detail. I did think about that approach, I just forgot.

TurkeyMan · 2018-05-21T17:51:12Z

Right, it's moved a little farther from C++ const &, and that was due to feedback here.
I've become convinced that non-const cases are useful and they're actually a feature, and not just a thing to avoiding const-related issues.
Pipeline programming in D is super-popular, and almost all ranges are created and returned from functions as rvalues. Not all ranges should be copied by-value though, so things like return ref can be used along the pipeline. This broke the pipeline before, but it all works very nicely with this dip.

mdparker

As a first pass, I've made some suggestions for structural edits. I'll give you some time to address these before drilling down to a line/copy edit pass.

mdparker · 2018-05-29T09:24:15Z

DIPs/DIP1xxx-rval_to_ref.md

+| Author:         | Manu Evans (turkeyman@gmail.com)                                |
+| Status:         | Draft                                                           |
+
+## Abstract


I think the abstract as written is too dense and too detailed. It can be made much more concise, likely stripped down to a single paragraph. For example, you don't need to iterate here any of the specific issues you detail in later in the Rationale. Just get across the main ideas: the current status quo is sometimes troublesome and this DIP proposes a means to alleviate the pain.

mdparker · 2018-05-29T09:26:54Z

DIPs/DIP1xxx-rval_to_ref.md

+
+Here is proposed a strategy to emit implicit temporaries to conveniently interact with APIs that use `ref` arguments.
+
+## Reference


It's great that you've listed so many references, but it's a bit much for anyone coming to all of this for the first time without anything to guide them. A one-line summary for each reference link (forum threads, issues, and PRs all) would be extremely helpful here.

mdparker · 2018-05-29T09:31:16Z

DIPs/DIP1xxx-rval_to_ref.md

+
+## Rationale
+
+When calling functions that receive `ref` args, D prohibits supplying rvalues. It is suggested that this is to assist the author identifying likely logic errors where an rvalue will expire at the end of the statement, and it doesn't make much sense for a function to mutate a temporary whose life will not extend beyond the function call in question.


This first sentence would work well for your opening statement in the abstract, though it could be reworded a bit, e.g. "D does not allow rvalues to be passed as function arguments where ref parameters are declared." or some such.

mdparker · 2018-05-29T09:34:49Z

DIPs/DIP1xxx-rval_to_ref.md

+With these cases in mind, the existing rule feels out-dated or inappropriate, and the presence of the rule may often lead to aggravation while trying to write simple, readable code.  
+Calling a function should be simple and orthogonal, generic code should not have to concern itself with details about ref-ness of function parameters, and users should not be required to jump through hoops when `ref` appears in API's they encounter.
+
+Consider the example:


You examples should be collocated above with the issues they are intended to demonstrate, e.g.

"The first issue is yadda yadda.

Example Here

A related issue is yadda yadda.

Example here

mdparker · 2018-05-29T09:37:53Z

DIPs/DIP1xxx-rval_to_ref.md

+    }
+}
+```
+This example situation is simplified, but it is often that such issues appear in complex aggregate meta, which may be difficult to understand, or the issue is caused indirectly at some layer the user did not author.


Be careful about authoritative claims about frequency, or side effects that are hard to quantify. If you have precise claims to make, with demonstrative data to back them up, then the data need to be referenced somehow. Otherwise, consider language such as "can lead to" or "may cause", with examples to demonstrate. This applies not just here with these lines, but throughout the document.

mdparker · 2018-05-29T09:38:36Z

DIPs/DIP1xxx-rval_to_ref.md

+
+These work-arounds damage readability and brevity, they make authoring correct code more difficult, increase the probability of brittle meta, and it's frustrating to implement repeatedly.
+
+## Proposal


For consistency with other DIPs, let's keep the 'Description' label here.

mdparker · 2018-05-29T09:39:55Z

DIPs/DIP1xxx-rval_to_ref.md

+
+It is important that `T` be defined as the argument type, and not `auto`, because it will allow implicit conversions to occur naturally, with identical behaviour as when argument was not `ref`. The user should not experience edge cases, or differences in functionality when calling `fun(int x)` vs `fun(ref int x)`.
+
+## Temporary destruction


This should be a level 3 subheading under description, as should all the headings between here and the copyright.

atilaneves · 2018-05-29T13:29:55Z

DIPs/DIP1xxx-rval_to_ref.md

+When calling functions that receive `ref` args, D prohibits supplying rvalues. It is suggested that this is to assist the author identifying likely logic errors where an rvalue will expire at the end of the statement, and it doesn't make much sense for a function to mutate a temporary whose life will not extend beyond the function call in question.
+
+However, many functions receive arguments by reference, and this may be for a variety of reasons.  
+One common reason is that the cost of copying large structs via the parameter list is expensive, so struct parameters may be received by reference to mitigate this cost.  


I'd change this to "the cost of copying or moving large structs".

atilaneves · 2018-05-29T13:31:14Z

DIPs/DIP1xxx-rval_to_ref.md

+One common reason is that the cost of copying large structs via the parameter list is expensive, so struct parameters may be received by reference to mitigate this cost.  
+Another common case is that the function may want to mutate the caller's data directly or return data via `out` parameters due to ABI limitations regarding multiple return values. This is the potential error case that the existing design attempts to mitigate, but in D, pipeline programming is vary popular, and contrary to conventional wisdom where the statement is likely to end at the end of the function call, pipeline expressions may result in single statements performing a lot of work, mutating state as it passes down the pipeline.
+
+A related issue is with relation to generic code which reflects or received a function by alias. Such generic code may want to call that function, but it is often the case that details about the ref-ness of arguments lead to incorrect semantic expressions in the generic code depending on the arguments, necessitating additional compile-time logic to identify the ref-ness of function arguments and implement appropriate workarounds on these conditions. This leads to longer, more brittle, and less-maintainable generic code. It is also much harder to write correctly the first time, and such issues may only emerge in niche use cases at a later time.


An example here would help.

atilaneves · 2018-05-29T13:32:44Z

DIPs/DIP1xxx-rval_to_ref.md

+fun(my_short); // implicit type conversions (ie, short->int promotion)
+// etc... (basically, most things you pass to functions)
+```
+The work-around can bloat the number of lines around the call-site significantly, and the user needs to declare names for all the temporaries, polluting the local namespace, and often for expressions where no meaningful name exists, leading to.


The "leading to" at the end of the sentence doesn't seem to belong there.

atilaneves · 2018-05-29T13:35:33Z

DIPs/DIP1xxx-rval_to_ref.md

+Is rewritten:
+```d
+{
+  T __temp0 = void;


Why not T __temp0 = 10?

Because that would change the order of evaluation.
The order of evaluation should match function call argument evaluation exactly. I tried to express that in the simplest terms I could.

Another point is: T might not be constructable from an int.

In the text below: "It is important that T be defined as the argument type", so in this case, T is int.

Removed `const` from proposal.

Copy edit pass.

TurkeyMan · 2018-07-19T01:34:53Z

Integrate changes and tweaked a touch.

TurkeyMan changed the title ~~Added first draft~~ Passing rvalues to const ref args Mar 25, 2018

wilzbach reviewed Mar 25, 2018

View reviewed changes

TurkeyMan force-pushed the ref_args branch from 5ee8506 to 7a37038 Compare March 25, 2018 21:32

TurkeyMan force-pushed the ref_args branch from d67b4c0 to 87fb4e9 Compare March 26, 2018 02:22

wilzbach reviewed Mar 31, 2018

View reviewed changes

TurkeyMan force-pushed the ref_args branch 2 times, most recently from 67ea010 to ba70f30 Compare May 20, 2018 00:17

mdparker requested changes May 29, 2018

View reviewed changes

atilaneves reviewed May 29, 2018

View reviewed changes

TurkeyMan and others added 5 commits June 16, 2018 01:23

Added first draft

edbe0da

Set to draft status

37986cb

Edits from feedback

cf20105

Removed `const` from proposal.

Added references.

37eebfc

Added lvalue-restrictive concept.

7b1974f

Dun a fair refactor addressing most criticism.

9b9a355

TurkeyMan force-pushed the ref_args branch 3 times, most recently from d91d6af to f310689 Compare June 16, 2018 09:06

mdparker approved these changes Jun 18, 2018

View reviewed changes

Added a comment on the const discussion.

31105c3

TurkeyMan force-pushed the ref_args branch from f310689 to 31105c3 Compare June 18, 2018 04:02

mdparker and others added 3 commits July 2, 2018 14:04

Copy edit pass.

ee8d8fb

Merge pull request #1 from mdparker/ref_args2

5a48ebe

Copy edit pass.

Tweak

28c0d8b

mdparker merged commit 58e234f into dlang:master Jul 20, 2018

PetarKirov mentioned this pull request Sep 2, 2019

Add -transition=in compiler flag to help users transition away from using in dlang/dmd#10382

Closed


		Here is proposed a strategy to emit implicit temporaries to conveniently interact with APIs that use `ref` arguments.

		## Reference


		## Rationale

		When calling functions that receive `ref` args, D prohibits supplying rvalues. It is suggested that this is to assist the author identifying likely logic errors where an rvalue will expire at the end of the statement, and it doesn't make much sense for a function to mutate a temporary whose life will not extend beyond the function call in question.


		These work-arounds damage readability and brevity, they make authoring correct code more difficult, increase the probability of brittle meta, and it's frustrating to implement repeatedly.

		## Proposal


		It is important that `T` be defined as the argument type, and not `auto`, because it will allow implicit conversions to occur naturally, with identical behaviour as when argument was not `ref`. The user should not experience edge cases, or differences in functionality when calling `fun(int x)` vs `fun(ref int x)`.

		## Temporary destruction

Uh oh!

Passing rvalues to const ref args #111

Passing rvalues to const ref args #111

Uh oh!

Conversation

TurkeyMan commented Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TurkeyMan commented Mar 25, 2018

Uh oh!

TurkeyMan commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skl131313 commented Mar 26, 2018

Uh oh!

TurkeyMan commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdparker commented Apr 2, 2018

Uh oh!

mdparker commented Apr 2, 2018

Uh oh!

schveiguy commented Apr 2, 2018

Uh oh!

Bolpat commented Apr 17, 2018

Uh oh!

Bolpat commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TurkeyMan commented Apr 17, 2018

Uh oh!

TurkeyMan commented Apr 17, 2018

Uh oh!

Bolpat commented Apr 26, 2018

Uh oh!

TurkeyMan commented Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bolpat commented May 21, 2018

Uh oh!

TurkeyMan commented May 21, 2018

Uh oh!

TurkeyMan commented May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdparker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TurkeyMan commented Mar 25, 2018 •

edited

Loading

TurkeyMan commented Mar 26, 2018 •

edited

Loading

TurkeyMan commented Mar 26, 2018 •

edited

Loading

Bolpat commented Apr 17, 2018 •

edited

Loading

TurkeyMan commented Apr 26, 2018 •

edited

Loading

TurkeyMan commented May 21, 2018 •

edited

Loading