Skip to content

[New DIP] Binary assignment operators for properties#97

Closed
JinShil wants to merge 7 commits intodlang:masterfrom
JinShil:master
Closed

[New DIP] Binary assignment operators for properties#97
JinShil wants to merge 7 commits intodlang:masterfrom
JinShil:master

Conversation

@JinShil
Copy link
Contributor

@JinShil JinShil commented Nov 4, 2017

@mdparker I'm ready to start the submission process.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example is wrong - the getter is called because an integer is not implicitly convertible to R, so the setter taking an R cannot be called.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I've removed the example for now.

@mdparker
Copy link
Member

mdparker commented Mar 7, 2018

@JinShil I want to thank you for your patience. If you're still ready to move forward with this, we can get it going now.

I've read through it and it's well-written. Before I make any edit suggestions, however, I ask that you update the document to reflect the new version of Template.md. Just some minor structural changes there.

As soon as you are ready, I'll make a call in the forums to get some more eyes on it before we move it out of draft review to give people a change to see any issues I may have overlooked. But I think we can move it to the first round of community review quite quickly.

@JinShil
Copy link
Contributor Author

JinShil commented Mar 8, 2018

I've made changes to support the new template. At least I think I did; I didn't see anything new besides the Contents. I'd like to move forward with this first, instead of my other PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmodern.

@schveiguy
Copy link
Member

A couple of notes here:

  1. an rvalue property getter that supports @= directly should NOT go through the lowering. This allows harnesses that get around the current limitations. e.g.:
struct Property(T)
{
   T* val;
   T get() { return *val; }
   alias get this;
   auto opOpAssign(string s : "+")(T other) { *val += other; }
}
  1. Your example with UFCS struct properties seems to suggest that
    a. properties on structs must use pointers
    b. implicit address operator will be applied to the parameter

I would suggest that the lowering is done WITHOUT using any additional address operators (i.e. change your example to take S by reference). I would also suggest that you refrain from requiring reference semantics for the setter -- the language will take care of this already if needed, and there may be reasons why someone defines the property on an rvalue.

  1. In other languages such as Swift, there is a nested property feature, which I'm not sure we want to implement or not. It goes like this:
struct S
{
    var x: Int32
}

struct T
{
    var m : Int32
    var s : S
    {
        get {
            print("in getter")
            return S(x: m)
        }
        set {
            print("in setter")
            m = newValue.x
        }
    }
}

var t = T(m: 5)
t.s.x += 6

What this does is print "in getter" and "in setter"

Essentially, it lowers to this code:

var _tmp = t.s
_tmp.x += 6
t.s = _tmp

I'm not sure if we want to address such things. But if we start lowering things like you propose, people will expect this next.

@JinShil
Copy link
Contributor Author

JinShil commented Mar 8, 2018

In other languages such as Swift, there is a nested property feature, which I'm not sure we want to implement or not. It goes like this:

Actually, I would prefer something like that, but that would also render our current @property syntax obsolete, IMO. I suppose a future DIP could propose that as an alternative to @property, but that's a battle I don't want get involved in right now.

@JinShil
Copy link
Contributor Author

JinShil commented Mar 8, 2018

an rvalue property getter that supports @= directly should NOT go through the lowering. This allows harnesses that get around the current limitations.

It looks like I'll have to add another special case for that. That's also additional ammunition for the "prevent anti-pattern proliferation" argument.

@JinShil
Copy link
Contributor Author

JinShil commented Mar 8, 2018

i.e. change your example to take S by reference

Done. Thank you.

I would also suggest that you refrain from requiring reference semantics for the setter -- the language will take care of this already if needed, and there may be reasons why someone defines the property on an rvalue.

I'm sorry, but I don't understand. Could you please elaborate?

@schveiguy
Copy link
Member

Could you please elaborate?

It was a preemptive suggestion -- right now, your proposal doesn't say anything about what happens with a UFCS property that takes its object by value. I would say that is fine, and the lowering should not place any extra restrictions there.

@schveiguy
Copy link
Member

Actually, I would prefer something like that, but that would also render our current @property syntax obsolete, IMO

I wasn't suggesting the syntax. My point was to show what swift does with its properties when they are nested.

In D, It would look something like this:

struct S
{
    int x; 
}

struct T
{
    int m;
    @property S s() { return S(m); }
    @property void s(S newValue) { m = newValue.x; }
}

void main()
{
    T t;
    version(specialPropertyRewrite)
    {   
        // this doesn't do what you want it to right now, but coud be made to
        // lower to the code below.
        t.s.x += 5; 
    }
    else
    {
        // what the property could be lowered to
        auto _tmp = t.s;
        _tmp.x += 5;
        t.s = _tmp;
    }
}   

@JinShil
Copy link
Contributor Author

JinShil commented Mar 10, 2018

@mdparker

@schveiguy's earlier comment about rvalues that forward operator overloads got me thinking more about finding a library alternative. My initial research led me to this post. In that thread, @Biotronic has provided a potential library alternative, and identified some issues in the language that, if addressed, may be able to provided a suitable alternative to this DIP. I don't know yet if I would prefer this DIP or a library alternative, but I need some time to explore the idea. If I still decide to pursue this DIP, I will need to document the merits of the library alternative (or lack thereof), and add another "Special case" to this DIP and the implementation.

So, I need more time, and I don't want to hold up the PR queue. Please select another PR from the queue to go first.

If anyone would like to discuss the library alternative please do so in the forum thread to keep the discussion in this PR relevant to the DIP itself.

@schveiguy
Copy link
Member

Respect the decision. The one drawback that this would solve beyond any library alternative would be type detection.

That is:

auto x = obj.prop;

x's type would be the actual property type, and not some harness. Note that this also has other drawbacks, namely that now you actually have a reference type, whereas with normal properties, x would be a value type.

@quickfur
Copy link
Member

If you ever decide to move forward with this DIP, I would suggest addressing its pros and cons compared with harnesses discussed above. This was one thing I noticed was somewhat lacking in the overview, which talks about the problems with using lvalues, ref, etc., but doesn't quite address the question of why returning a harness / proxy type would still be considered inferior.

@anon17
Copy link

anon17 commented Apr 4, 2018

Remove the section about ufcs properties, currectly they are not well supported by the language: they are not accessible through type alone and are not returned by allMembers and getMember traits, so they can't pass the duck test. Mentioned languages don't have extension properties, and at least C# has special syntax for extension methods and can disambiguate that the function is indeed meant to be extension method, D has no such mechanism and extension properties are ambiguous with module properties. Touching them now will interfere with later fix like deprecation.

- **Requirement 3** - `e1` must never be copied.
- **Requirement 4** - getter and setter each must be called exactly once.

In the general case, to meet these requirements, the binary assignment expression is lowered to the lambda expression `((auto ref _e1) => _e1.prop(_e1.prop() @ e2))(e1)` except under the following conditions:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, but wouldn't this produce erroneous code?

struct A {
  A opOpAssign(string op)(A a) { writeln("foo"); return this; }
  A opBinary(string op)(A a) { writeln("bar"); return this; }
}

struct B {
  A a_;
  @property A prop() { return a_; }
  @property A prop(A a) { return a += a };
}

A a;
B b;
b.prop += a; // oops, prints out bar!

I'm not sure how to produce the correct code, but shouldn't this be lowered to something like:

((auto ref _e1) => { auto tmp = _e1.prop; tmp += e2; _e1.prop = tmp;})(e1)

Copy link
Contributor Author

@JinShil JinShil May 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, I think this line @property A prop(A a) { return a += a }; is wrong. It should be @property A prop(A v) { return a = v; }.

Therefore, the implementation does the correct thing and lowers the b.prop += a to delegate (ref B _e1) => _e1.prop(_e1.prop().opBinary(a)); However, inside prop's setter, a = v gets lowered to A's opOpAssign member, so yes it prints out "bar".

I think what you're implying is there is an ambiguity over A's opBinary member the lowering to the lambda expression. Currently the compiler prints out "foo", but with this DIP, the implementation will print out "bar". That's a breaking change, so I'll have to address that. I suspect we'll have to check if the return value overloads opBinary before deciding whether or not to lower to the lambda. Thank you for identifying this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, yeah, sorry brain fart. But yes it was supposed to be @property A prop(A v) { return a = v; }

And yes. I'm implying that the code is not ambiguous, but uses an op that I have not intended to use as a developer. I defined opOpAssign, and opBinary, in my code I explicitly write an opOpAssign, but it uses opBinary. I think if an opOpAssign is not defined it should not lower.

}
```

Despite the unfortunate inconsistency of this proposal treating rvalue properties and lvalue properties differently, it is necessary to prevent breakage and maintain the status quo.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unfortunate indeed. Is a deprecation path viable here maybe? So if there's code like this:

struct S {
  int value;
  @property ref int prop() { return value; }
  @property ref int prop(int v) { return value = v; }
}

S s;
s.prop += 3;

Then something like: Deprecated. op= on a ref returning property. Please remove the @property attribute from the getter.

But that would require that an @Property name can have a non-@Property name that is an overload, which I believe you cannot do. But maybe good enough reason here to allow this case until the deprecation period is over?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that pattern appears very often in code because it's only necessary to have the ref getter to accomplish the task. What I hope will happen is that after this DIP is approved and implemented, users will refactor their code moving away from ref properties. Then the pattern will become even less likely, and then we can issue a deprecation or change in behavior with less disruption.

f.width = temp;
```

That proposal violates Requirements 2.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it a requirement that f is only referenced once?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the comments starting here in the implementation for an explanation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I believe there is a lowering that will reference once and use the correct operator. Maybe :p I'll play around a bit and see.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, one evaluation, and correct operator. Are there any problems here?

import std.stdio : writeln;

struct B {
    this(this) {
        "copy B".writeln;
    }
    void opBinary(string op)(auto ref B other) { writeln("binary: ", op); }
    void opOpAssign(string op)(auto ref B other) { writeln("opassign: ", op); }
}
struct A {
    B b_;
    this(this) {
        "copy A".writeln;
    }
    @property B prop() { "getter".writeln; return b_; }
    @property void prop()(auto ref B b) { "setter".writeln; b_ = b; }
}

void main() {
    A a;
    B b;
    auto array = [A(), A(), A()];
    auto i = 0;
    lower!("prop", "+=")(array[i++], b);
    i.writeln;
}

void lower(string prop, string op, T, U)(auto ref T lhs, ref U rhs) {
    mixin("auto p = lhs." ~ prop ~ ";");
    mixin("p" ~ op ~ "rhs;");
    mixin("lhs." ~ prop ~ "= p;");
}

@JinShil
Copy link
Contributor Author

JinShil commented Nov 23, 2018

FYI. I am abandoning this DIP and the implementation. I'm afraid my future with D is uncertain at best. If anyone wants to pick of the torch, it's yours. I'll keep the PRs open for now unless someone believes they should be closed.

@12345swordy
Copy link

I am not sure how to pick up this up where you drop off.
@mdparker Any ideas?

@JinShil
Copy link
Contributor Author

JinShil commented Nov 24, 2018

There are two things I wanted to add to this DIP:

  • Explain why a library alternative like this is not a suitable option
  • Address @aliak00's comments

In my research I discovered that there are a number of problems with the current @property implementation, and I considered creating a separate DIP to address those first. I've collected a bunch of notes on the subject and posted them to this gist

Good luck! You'll need it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

Comments