Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to make object immutable. #1157

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

mhermier
Copy link
Contributor

@mhermier mhermier commented Mar 16, 2023

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:

  • It is opt-in for foreign and primitives, and should be manually checked.
  • Due to implementation detail static member variables do not responds to the their class being frozen.
  • Fn/Closure are immutable with real up-values. Like class static member variables, accessing global up-values do not responds to their function being frozen.

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

@PureFox48
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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.

Wren lacks the ability to declare variable constants.
The foolwing patch allow to make objects immutable, giving a similar
functionnality.

NOTE: The implemetation while functionnal for `ObjectInstance` is opt-in for
      foreign and primitive methods.
@mhermier
Copy link
Contributor Author

Updated to the new API with secret.

@mhermier mhermier changed the title Add Object.freeze, Object.isFrozen and Object.validateIsFrozen. Allow to make object immutable. Mar 17, 2023
@HallofFamer
Copy link

HallofFamer commented May 4, 2023

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
Copy link
Contributor Author

mhermier commented May 4, 2023 via email

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.

3 participants