wren-lang / wren

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

Allow to make object immutable. #1157

Open mhermier opened 1 year ago

mhermier commented 1 year ago

Hi,

Here is an attempt to provide immutable values to wren by simulating a freeze API to implement some ideas from #1156.

The following API is added:

class Object {
  foreign freeze()
  foreign isFrozen
  foreign setFrozen(isFrozen)
  foreign setFrozen(isFrozen, secret)
}

All regular object should be covered, via the interception of ObjInstance write access. Thought, implementation is not perfect:

Edit: Updated to handle/test functions. Edit: Updated to a new show new API.

PureFox48 commented 1 year ago

Well, it certainly seems promising.

It would be an important feather in Wren's cap if we had a decent story on immutability which didn't cost too much.

I think freeze (courtesy of JS) is a good word to use - it's not too long and does what it says on the tin.

Presumably Object.validateIsFrozen is just there to test the internal API - we shouldn't need it should this idea get to the production stage.

However, there might be a case for adding an Object.unfreeze method even though it probably wouldn't be used much. I'm thinking here of scenarios where you have a global list (say) which you can freeze and then be safe in the knowledge that some complicated method or function can access it but not mutate it. You might then want to unfreeze it so you can change a few elements, refreeze it and call the method/function again.

Another point which occurs to me is that the List.sort method couldn't be applied to a frozen list but, as that's implemented entirely in Wren, we'd need to add a manual check for that.

I'm not sure how important it is to be able to freeze the static fields of a class or to freeze a function/fiber object. If there isn't any easy way to do this, perhaps we should just exempt them from the immutability API.

mhermier commented 1 year ago

Object.validateIsFrozen() is there in an attempt to provide a universal error. Eg: List.sort (which I forgot, and probably others) should use it to validate early before it enters the C List.swap(_,_) function.

I can imagine there are scenarios that would require to Object.unfreeze(), but I think I want to avoid it unless there is a requirement. Security wise, I think I would want to add a key/lock mechanism (on the wren side only). Thought it would be quite heavy, since it would mean creating a key Object (which would also need to be discussed).

var res = createResource()
var key = res.freeze()
// use the frozen resource
res.unfreeze(key)
// mutate the resource
key = res.freeze()
// use the frozen resource again

To limit the number of symbol we could use Object.isFrozen=(_) instead, but that doesn't play nice with a key/lock mechanism. That said it should be rare enough so we can accept the oddity?

var res = createResource()
var key = res.isFrozen = true
// use the frozen resource
res.isFrozen = key
// mutate the resource
key = res.isFrozen = true
// use the frozen resource again
PureFox48 commented 1 year ago

I'm not keen on Object.isFrozen=(_) as it's not very intuitive. If we need it, I'd prefer to stick with Object.validateIsFrozen() or perhaps something a little shorter such as Object.checkIsFrozen().

Looking at the C side and list_addCore in particular, I'm wondering whether it's really necessary for that function to call VALIDATE_VALUE_IS_NOT_FROZEN(args[0]) as that's only going to be called by the compiler when compiling list literals and I don't think it will be possible for the list to be frozen at that stage.

Turning now to what's currently implemented for the List class on the Wren side, would it be feasible to move part of the implementation of the addAll(other) method to the C side so VALIDATE_VALUE_IS_NOT_FROZEN(args[0]) is only called once rather than for each item added? This could only be done if other were aList which is a simple check. For other sequences, we'd have to iterate through them and add items individually as we do now.

The * operator calls that method and the + operator doesn't at present but could be changed so that it does if other were a List. Otherwise we could leave their implementations on the Wren side.

mhermier commented 1 year ago

I'm currently testing another possible API that is more promising.

class Object {
  foreign freeze() // Eternal freeze
  foreign isFrozen
  foreign setFrozen(isFrozen) // Reversible freeze (`true` is the default secret)
  foreign setFrozen(isFrozen, secret) // Reversible freeze with user secret
}

If addAll is proven to be a bottleneck in practice, we may need to add a specialized addAll_ that expect natives. But in deeps, it is not directly related to this change.

PureFox48 commented 1 year ago

FWIW, Object.freeze() is not reversible in JS though that doesn't mean we can't make it so in Wren as there are clearly scenarios where it would be useful.

mhermier commented 1 year ago

Updated to the new API with secret.

HallofFamer commented 1 year ago

Its an interesting idea, though I have two questions:

  1. What was the rationale behind choosing a Ruby style frozen variable approach, rather than introducing a keyword like const or val that handles variable mutability at compiler level?
  2. The approach seems to add 2 new fields to the object header struct Obj, is there any performance implication of this? Or put it another way, how expensive is it to add fields to object header? For instance, currently instance variables are only stored in struct ObjInstance, what if the the fields array is moved from struct ObjInstance to struct Obj instead? Will it be a significant slowdown?
mhermier commented 1 year ago
  1. Introduction of const keyword requires compiler to be more intelligent or requires additional symbols to route non const to const by default.

  2. It has an impact, but there is room for it so at least it as no memory impact.