wren-lang / wren

The Wren Programming Language. Wren is a small, fast, class-based concurrent scripting language.
http://wren.io
MIT License
6.86k stars 550 forks source link

[Feature Request] Syntax sugar for combination of getter+setter #849

Open avivbeeri opened 3 years ago

avivbeeri commented 3 years ago

I imagine this will get rejected, but I need to get it out of my head.

When I'm working on projects in Wren, I find myself making a lot of fields for classes with symmetric getters and setters, in this sort of style:

class Entity {
  construct (pos) {
    _pos = pos
  }

 pos { _pos }
 pos=(v) { pos = v }
}

It would be nice if there was a keyword sugar to generate this more conveniently.

For example:

class Entity {
  construct (pos) {
    _pos = pos
  }

  field pos
}

I think you could also do this for static fields: static field pos

Obviously there are downsides to adding an extra keyword to the language, i'm not sure if there's a way to fix that in a backwards compatible way.

ChayimFriedman2 commented 3 years ago

i'm not sure if there's a way to fix that in a backwards compatible way.

There is, using contextual keywords, but this will complicate both the lexer and the parser a lot. Not worth it IMHO.

ChayimFriedman2 commented 3 years ago

I think this will be better with the following syntax:

class Entity {
  construct new(pos) {
    _pos = pos
  }

  pos = _pos
}

It will also be backwards compatible.

Edit: You forgot the constructor name 😅.

Edit 2: A better title will be "[Feature Request] Syntax sugar for combination of getter+setter". It describes better the goal of this proposal. I went here because of curiosity what the proposed field keyword will do (so maybe we do want a strange title 😆).

mhermier commented 3 years ago

The complexity is not very high, there are a few places where we create asm stubs by hand and it's not really complicated.

The syntax should be investigated a little bit more though.

It is true that you use a more data oriented approach you need a lot of trivial getters/setters.

avivbeeri commented 3 years ago

I believe ruby has an implementation of a feature like this in the works already, with a less problematic syntax

billyquith commented 3 years ago

Hi @avivbeeri. What is the benefit to using that style, as you say? I.e. hiding your member variables, but then exposing them as properties? That might be something you do idiomatically in C++ (but you can set access). What is the reason for doing it in Wren? This is fine:

class Entity {
  construct new (pos) {
   pos = pos
  }
}

Then, later, if you need to hide pos you can rename the hidden version. That way, you only do the work for the variables that have a property interface.

class Entity {
  construct new (pos) {
   _pos = pos
  }

 pos { _pos }
 pos=(v) { _pos = v }
}

Is there a reason not to change that style? I'm pretty new to Wren so I'm still learning it myself.

avivbeeri commented 3 years ago

Your first example there doesn't actually achieve anything: The pos value will just be lost since you assign it to itself. Assuming you meant to write:

class Entity {
  construct new (pos) {
    _pos = pos // this creates a private _pos field
  }
}

Then the _pos is a private value. If it's your only value, manually creating getters and setters is fine.

But for certain applications, you end up having lots of variables, all with public getters and setters, and it gets very repetitive.

For example, in DOME: https://github.com/domeengine/dome/blob/fa067c51cbff3ae4087b8acb681c9507aca58fd4/src/modules/audio.wren#L133

  volume { _volume }
  volume=(volume) { _volume = volume }
  loop { _loop }
  loop=(loop) { _loop = loop }
  pan { _pan }
  pan=(pan) { _pan = pan }

It would be nice if I didn't have to declare each getter and setter manually, when I'm creating identical methods for each one.

In some game projects, you could potentially end up with 6-7 of these, which is both tedious, and error prone

billyquith commented 3 years ago

Ah, ok, _ is mandatory. 🍏 Thanks. I thought it was stylistic like Python. In that case I think it might be best to add this without adding a keyword. Wren seems to try to be very concise. Maybe:

class Entity {
  construct new (pos) {
   _pos = pos
  }

  pos = _pos
}
avivbeeri commented 3 years ago

Indeed. As mentioned above, a potential way of doing this in the works, which doesn't add new keywords, so we can look forward to that.

mhermier commented 3 years ago

I found foo = _foo noisy for something that could be automated, unless it is a rename

PureFox48 commented 3 years ago

A problem with having a combined getter/setter syntax in a dynamically typed language such as Wren is that you can't then type-check the incoming setter value.

For example, I have a lot of code which follows this sort of pattern:

foo { _foo }

foo=(f) {
    if (!(f is Foo)) Fiber.abort("Argument must be a Foo.")
    _foo = f
}

Sometimes rather than aborting I'll just change the type of the argument to something more suitable, stringifying a Num say if I'm expecting a String.

It's rare for me not to care about the type of the incoming value so, even if we had a syntax for this, I'm not sure how much use it would be when writing robust production code.

Incidentally, one thing I would find useful is an isnot operator to cut down on the parentheses in the above code.

mhermier commented 3 years ago

This is not a replacement for the existing syntax. And this can be used until the urge for specific types are required. Thought I would not encourage type checking at all, unless really required.

In your example, I would change the flow of if to positive, so there is only a final default handling and possibly a single Fiber.abort(). Not that it should drastically change performance, but I find it to be nice flow with only one error handling at the end.

PureFox48 commented 3 years ago

I realize that this wouldn't be a replacement for what we do already, just another option. As such I don't really have a problem with it even though I'd rarely use it myself.

I guess it's a matter of taste how you arrange the code and sometimes I find it easier to do it the way you suggested. But the Wren code in wren_core.wren does at least seem to do it the way I usually do and get the type checking out of the way first.

clsource commented 3 years ago

This reminds me of @synthesize in Objective-C https://programmer.help/blogs/objective-c-property-synthesize-and-dynamic.html

Objc recommends that we access the properties of an object through the set/get method. Obviously, manually adding the declaration and implementation of set/get methods for each attribute is a very low cost-effective duplication of effort. Therefore, objc provides some keywords to help us simplify this process. That's exactly what it is.

@synthesize specifies the implementation of the set/get method. By default, @synthesize generates a member variable with the same name as the target of the set/get.

I don't know if this could be added in as a method?

class Entity {
  synthesize {
    // Will generate set/get methods for these properties
    return [_post]
  }
  construct (pos) {
    _pos = pos
    this.synthesize
  }
}

With this approach, it can be inherited from Object and all classes can override the method both instance or static. This is just syntax sugar for adding the getter and setters before interpretation of the class.

mhermier commented 3 years ago

It is not possible for multiple reasons for now. And in addition I find it not safe that it can be changed dynamicaly on each invocation. For safety it should be a grammar modification.

ChayimFriedman2 commented 3 years ago

I would like to add that changing classes dynamically goes against the philosophy of Wren:

Wren is strictly class-based. Every object is an instance of a class. Classes in turn have a well-defined declarative syntax, and cannot be imperatively modified. In addition, fields in Wren are private to the class—they can only be accessed from methods defined directly on that class.

(from https://wren.io/performance.html#fixed-object-layout).

clsource commented 3 years ago

In that case then using synthesize as a keyword (similar to static) maybe?

class Entity {
  synthesize pos
  static synthesize message
  construct (pos) {
    this.pos = pos
  }
}
ChayimFriedman2 commented 3 years ago

I like the idea. It's like the proposed field keyword but less boilerplate-y.

But maybe we want an other keyword.

mhermier commented 3 years ago

var would be a good candidate, thought I would reserve it for future to allow non '_' prefixed member names.

ChayimFriedman2 commented 3 years ago

I don't think we want non _ prefixed field names. That's redundant IMO. I would vote for property.

BTW, you don't even have a name for the constructor and you don't even need this. if you rename the parameter:

class Entity {
  property pos
  construct new(position) {
    pos = position
  }
}

Edit: That reminds C#'s automatic properties ({ get; set; }) to me.

mhermier commented 3 years ago

this is not required as long as you declare your properties/field/... before hand. Otherwise I think name resolution kick outside of the class scope.

ChayimFriedman2 commented 3 years ago

I'm not sure I understand you correctly, but if you mean that the compiler can't find methods that declared after reference, you're wrong. Edit: Try it:

class C {
  static f() {
    System.print(g)
  }
  static g { "Hello" }
}
C.f()

The name resolution algorithm is fairly simple: if there isn't a local variable with the same name, it looks at the name: for names starting with a lower letter a method on this is assumed, otherwise a global variable is assumed (https://wren.io/classes.html#implicit-this).

mhermier commented 3 years ago

It does work unless you have an external variable of the same name. The upper scope resolution kicks in. So you have either to move declaration first or use this.

clsource commented 3 years ago

Other ideas would be

class Entity {
  // Only synthezise getter
  $pos
  // Synthezise getter and setter
  static $message$
  construct (pos) {
    pos = pos
  }
}

Or these

class Entity {
  // Only synthezise getter
  pos: get
  // Synthezise getter and setter
  static message: get, set
  construct (pos) {
    pos = pos
  }
}
class Entity {
  // Only synthezise getter
  pos :get
  // Synthezise getter and setter
  static message: attribute
  construct (pos) {
    pos = pos
  }
}
class Entity {
  // Only synthezise getter
  pos ${get}
  // Synthezise getter and setter
  static message ${attribute}
  construct (pos) {
    pos = pos
  }
}

Or since the keyword as would be added too

class Entity {
  // only getter
  pos is
  // Synthezise getter and setter
  static message as is
  construct (pos) {
    pos = pos
  }
}

Or using only Wren 0.3 keywords

class Entity {
  // only getter
  pos for {}
  // Synthezise getter and setter
  static message for {is}

  construct (pos) {
    pos = pos
  }
}

or maybe

class Entity {
  // only getter
  pos is {return}
  // Synthezise getter and setter
  static message is {return, in}

  construct (pos) {
    pos = pos
  }
}

It would be the same as:

class Entity {
  pos {_pos}

  static message {__message}
  static message = (value) {__message = value}

  construct (pos) {
    pos = pos
  }
}

In my personal opinion the latest one is most readable.

class Entity {
  // only getter
  pos is {return}

  // only setter
  size is {in}

  // Synthezise getter and setter in static property
  static message is {return, in}

  construct (pos) {
    pos = pos
  }
}
ChayimFriedman2 commented 3 years ago

It does work unless you have an external variable of the same name. The upper scope resolution kicks in. So you have either to move declaration first or use this.

Still I'm not sure I understand you but if you mean for example:

var a = "Outer"
class C {
  f() {
    System.print(a)
  }
  a { "Method" }
}

Then that depends on the captialness of the name.

@clsource I think this is slightly out of scope. Getter+setter combination is common, only getter/only setter are not common nor boilerplatey.

clsource commented 3 years ago

How about this syntax? Just using the available is and in keywords

class Entity {
  pos is in
  construct (pos) {
    pos = pos
  }
}
ChayimFriedman2 commented 3 years ago

First, in the constructor, this.pos = pos.

Second, redundant. A keyword property is the best IMO.

clsource commented 3 years ago

Ok here is more elaborate.

class Entity {
  message {in}
  construct new(message) {
    this.message = message
  }
}

var hello = Entity.new("world")
System.print(hello.message)
ChayimFriedman2 commented 3 years ago

Why not just

class Entity {
  property message
  construct new(msg) {
    message = msg
  }
}

var hello = Entity.new("world")
System.print(hello.message)

Clearer, doesn't it?

clsource commented 3 years ago

But it would require a new keyword to be created. My guess is that this feature have more possibilities to be included if does not requires a new keyword and uses the already available keywords.

I think that the keywords: is, as, this and in can be used to form a phrase that denotes an understandable meaning.

class Entity {
  message in this
  construct new(message) {
    this.message = message
  }
}

var hello = Entity.new("world")
System.print(hello.message)
ChayimFriedman2 commented 3 years ago

Let's ask @ruby0x1 about that.

mhermier commented 3 years ago

I was thinking that var could be the correct word, but then I thought about private member aliasing. And since the natural way to declare it, would be to use = I think it could be confusing.

We need a keyword of some sort, so the grammar stays LL(1). I'm not a big fan of post keywords since it will make aliasing quite complicated.

ChayimFriedman2 commented 3 years ago

I prefer property but I understand the wish to not add a keyword.