Patch PackageLink.SerializedDefaultAttributes Scriptability to None

[kennethloeffler]

PackageLink.SerializedDefaultAttributes was introduced in #381. Roblox's API dump incorrectly lists this property's read security as None:

                {
                    "Category": "Link",
                    "Default": "",
                    "MemberType": "Property",
                    "Name": "SerializedDefaultAttributes",
                    "Security": {
                        "Read": "None",
                        "Write": "NotAccessibleSecurity"
                    },
                    "Serialization": {
                        "CanLoad": true,
                        "CanSave": true
                    },
                    "ThreadSafety": "ReadSafe",
                    "ValueType": {
                        "Category": "DataType",
                        "Name": "BinaryString"
                    }
                },

Attempting to read this property from Lua results in the following error:

Unable to get property SerializedDefaultAttributes, type BinaryString

This can cause problems downstream, like in rojo-rbx/rojo#841.

This PR fixes this by patching the scriptability of PackageLink.SerializedDefaultAttributes to None. It's a rather manual solution though... it'd be nice to guard against this kind of incorrectness in the API dump in rbx_reflector. A long time ago, generate_reflection measured the scriptability of properties by attempting to read/write them directly in Roblox Studio. This has me wondering whether rbx_reflector should reintroduce that mechanism.

[Dekkonot]

For what it's worth, this isn't new behavior by Roblox. They list MeshPart.MeshId as having None Read security too, as an example:

                {
                    "Category": "Appearance",
                    "Default": "",
                    "MemberType": "Property",
                    "Name": "MeshId",
                    "Security": {
                        "Read": "None",
                        "Write": "NotAccessibleSecurity"
                    },
                    "Serialization": {
                        "CanLoad": true,
                        "CanSave": true
                    },
                    "ThreadSafety": "ReadSafe",
                    "ValueType": {
                        "Category": "DataType",
                        "Name": "Content"
                    }
                },

I think it's worth considering we actually want NotAccessibleSecurity properties in the database even if they're "only" inaccessible to write or read. After a quick search through the API dump, I'm not sure we actually do since they're all non-scriptable anyway.

[kennethloeffler]

I think they absolutely need to stay in the database... for example, users of Lune possibly depend on these properties being present, and they will be broken if we just remove them. I believe exhaustiveness is generally a nice property to have here (and might even be required for good behavior from rbx_binary and rbx_xml... but I haven't really checked this yet), and a property's presence in the DB should not be determined by its accessibility to Lua. The small inaccuracies wrt scriptability are just a bit annoying when we have to manually patch them. So, maybe we should think about introducing something like https://github.com/rojo-rbx/rbx-dom/commit/5bca08fec3f5708a29811dceb11957b92eafe81f to rbx_reflector

[Dekkonot]

What if we were to just assume a property was not readable or writable if either one was set to be inaccessible? It keeps things in the database, but it also handles properties of this sort. It's possible that it'd have consequences down the line if Roblox does something weird but right now it doesn't have side effects beyond adjusting the scriptability of a property, which is exactly what this patch (and any subsequent patches) do.

I'm not 100% opposed to adding a step like we had before, but I think it's going to give us relatively little useful information so it's worth considering the alternatives.

[kennethloeffler]

What if we were to just assume a property was not readable or writable if either one was set to be inaccessible?

This assumption doesn't always hold - for example, MeshPart.HasSkinnedMesh, MeshPart.HasJointOffset, MeshPart.JointOffset,and MeshPart.MeshId all have "Write": "NotAccessibleSecurity" and "Read": "None", and this is accurate to their actual behavior.