Object-docs (consolidated)

[jmagaram]

I'm not sure I did what you wanted but I merged all the object doc strings I did into this PR. It was a hassle getting them all merged. So I think if you accept this then all the others can be closed.

[glennsl]

In addition to the comments below, and as pointed out in the previous PRs, all the get and set functions are unsafe. It might be outside the scope of this PR to rename them to add the Unsafe suffix, but I think the docstrings should still clearly say that they're unsafe, and explain why.

[glennsl]

Same here. And with the potential functions assign2, assign3 and such of course.

[jmagaram]

I recommend removing it and not introducing assign2, assign3, etc. As you noted other places the developer can chain calls to assign and get the same result. But I'll do what you recommend. What should I do?

[glennsl]

You shouldn't do what I recommend, but I agree :)

[jmagaram]

I don't understand. Who decides this? We both agree to remove it. @zth need your input here.

[glennsl]

Who actually makes the decisions, and how, is just one of many mysteries around ReScript that you'll have to accept. The project has a history of poor management, though it's getting better! But still quite mysterious, unfortunately.

But for this library I suppose it's ultimately whoever has commit access, which is definitely @zth, and I'm sure @cknitt as well.

[jmagaram]

Could you explain why they are unsafe? The getters will fail if given null and undefined. Anything else? When will setters fail?

[jmagaram]

Hmm. Actually I don't think you can call this with a null or undefined object. So I don't know when the getters fail.

[glennsl]

Could you explain why they are unsafe?

Sure, sorry. If it was that obvious these probably wouldn't exist in this form anyway.

Here are examples where get and set should fail, but don't. Compared to properly typed access:

let obj = {"foo": 42}

let x: int = obj["foo"]
// let x: string = obj["foo"] // Error: This has type: int, Somewhere wanted: string
let x: option<string> = get(obj, "foo") // Should error, as above, because it's actually an int

// obj["foo"] = "bar" // Error: This has type: string, Somewhere wanted: int
set(obj, "foo", "bar") // Should error, as above, because it's actually an int
// let x: string = obj["foo"] // Error: This has type: int, Somewhere wanted: string
let x: int = obj["foo"] // Just to show that the type is still int, despite now containing a string

[jmagaram]

In several places I have suggested the getter return type unknown but have gotten zero thoughtful responses to that suggestion. I think that solves some of the issues because you can't unsafely cast to whatever you want. I prototyped some of it in a PR around the Type module, where I attempted to fix some real bugs like the fact that after you classify something you get a Type.object type and NOT something you can actually use as input to this module. And when you classify somethings as a symbol you get a Type.symbol and not something you can use as input in this module. The main feedback I got there was "just write docstrings". I want this to be a more collaborative process. It can't all be about the docstrings while serious functionality and safety and naming-problems (like getSymbol) exist.

I agree the setter is a problem when setting a property on an object ReScript thinks it knows the type of. Hadn't thought of that. Do you really think this warrants the Unsafe suffix? Or maybe just a usage warning/note in the docs?

[jmagaram]

I attempted to write the docs for Object.createWithProperties but don't understand it. It seems the second parameter must be a "property descriptor" like this. I can't get this to work when the first parameter is anything but Object.empty(). So I can't do something like start with an {"a":1} and add properties to it. Can someone write some sample code to show me how, or explain it? The Object.create seems to clone the first parameter. I Googled for a bit and gave up.

Object.createWithProperties(Object.empty(), {"b": {"value": 42, "writable": true}}),

[glennsl]

In several places I have suggested the getter return type unknown but have gotten zero thoughtful responses to that suggestion. I think that solves some of the issues because you can't unsafely cast to whatever you want.

It also removes the convenience and reason for these existing. These functions should probably just be removed.

I agree the setter is a problem when setting a property on an object ReScript thinks it knows the type of. Hadn't thought of that. Do you really think this warrants the Unsafe suffix? Or maybe just a usage warning/note in the docs?

As I've said elsewhere, I think everything unsound should be marked as Unsafe, because they break the type system guarantees that we rely on so much. And if doing so breaks someone's code because they were unaware of it being unsafe, then good! Now they're at least aware and can do something about it!

[jmagaram]

In several places I have suggested the getter return type unknown but have gotten zero thoughtful responses to that suggestion. I think that solves some of the issues because you can't unsafely cast to whatever you want.

It also removes the convenience and reason for these existing. These functions should probably just be removed.

If the functions are removed then I wasted a lot of time on this. I just started documenting the ArrayBuffer stuff and that is dangerous too. If it is a waste of time I want to know now. I think we should provide safe alternatives wherever we can and there are none in the Object module. I thought it was safe when I saw the option<'a> but I was wrong. And then we can provide any number of shoot-self-in-foot alternatives so convenience doesn't suffer. This is all nearly zero lines of code and the docs are simple.

obj->get("code")->Unknown.toString->Option.getWithDefault("nope!") // most safe, least convenient
obj->get("code")->Unknown.toString->Option.getExn // a little scary
obj->get("code")->Unknown.toStringUnsafe // very scary

obj->getString("code")->Option.getWithDefault("nope!")
obj->getStringUnsafe("code") // least safe, most convenient

The other place where I care about this is Error.t. I still can't get the "code" property out of a custom exception. I'd like to see a safe getter there.

Need some decisions here. At a minimum I like the idea of beefing up the Type/Unknown module and sprinkling safe getters around. Then one short trip to the Unknown module and you can get your data out. And then if unsafe getters are really critical we can put them directly on Object or Error.t.

I'm going to wait for @zth before marking everything as Unsafe.

[glennsl]

You're forgetting that the language has built-in syntax for object access too:

let x: string = obj["code"]

And this is entirely safe for well-defined object types.

The other place where I care about this is Error.t. I still can't get the "code" property out of a custom exception. I'd like to see a safe getter there.

This isn't going to help you with that though. But I like the idea of having these kinds of accessors in the Type/Unknown module instead, so that they're available for all types, not just objects.

[jmagaram]

I'm not forgetting anything. I'm fully aware this Object module is a kludge for weird interop scenarios. That's why my docs include language like "ReScript provides type-safe compile-time support for JavaScript objects that may be more convenient/safe. See ..."

[zth]

Reviewing and looking to merge this soon. Could this be rebased perhaps?

[zth]

@jmagaram closing this since it's merged (after a rebase) in f5ae1be (although I messed up the commit text some). Thanks for doing this one, large effort! Sorry for leaving it hanging for so long.