Generic property descriptors confusing to create

[jedel1043]

There is currently no support for generic property descriptors. According to the spec:

"A generic Property Descriptor is a Property Descriptor value that is neither a data Property Descriptor nor an accessor Property Descriptor."

This type of descriptor appears on some places in the spec:

https://tc39.es/ecma262/#sec-setintegritylevel

and some descriptors assign DataDescriptor attributes without defining a Value:

https://tc39.es/ecma262/#sec-regexpalloc
https://tc39.es/ecma262/#sec-arraysetlength

However, the current design of PropertyDescriptor forces every descriptor to be either DataDescriptor or AccesorDescriptor, making the api to create a generic descriptor confusing (you need to make a DataDescriptor without value or an AccesorDescriptor without get and set). There are some alternatives to solve this:

1. Preserve the current design and make a method to create a generic descriptor. This partially solves the problem but now the types don't represent exactly what type of descriptor you have.

2. Extend the current enum design with a Generic variant and lift Properties to be inside GenericDescriptor, possibly creating another struct in the way. This preserves the meaning of the types but now the match complicates a little more. You would have to do PropertyDescriptor::{properties, PropertyType::Data(data_desc)} to get the internal DataDescriptor.

3. Remove (Accesor/Data)Descriptor and use Option<Field> for every field exclusive to either descriptor. This completely removes the use of match but now we would need to obtain the type of a descriptor on runtime, possibly impacting the performance.
   The advantage of this approach is that now those strange cases with attributes without an explicit Value are possible to represent. (@raskad made possible to represent this on Fix DataDescriptor Value to possibly be empty #1419).

Please let me know what you think and if you have any other solution.

[raskad]

With #1419 we have the option to check if DataDescriptor holds a value. That enables us explicitly check for these specific edge cases when we need to. I think that should enable us to accurately represent all the states that are described in the spec.

Do you think that may be sufficient for the time being?

[jedel1043]

With #1419 we have the option to check if DataDescriptor holds a value. That enables us explicitly check for these specific edge cases when we need to. I think that should enable us to accurately represent all the states that are described in the spec.

Do you think that may be sufficient for the time being?

Yes, that would be the first solution I propose. The problem with that is now a DataDescriptor is not exactly a DataDescriptor, but a maybe? DataDescriptor, and the ambiguity of creating a generic descriptor from an AccessorDescriptor or a DataDescriptor is still there. However, this makes the fix easier; just create a new_generic(attributes) function to unificate the generation of generic descriptors and

fn is_generic(&self) -> bool {
        match self {
            Self::Data(data) => !data.attributes.writable() && !data.has_value,
            Self::Accessor(a) => a.get.is_none() && b.set.is_none(),
        }
    }

Personally I don't like this solution because it goes against the reasons for having an enum; now we need to check at runtime if a descriptor is generic or not, the enum should remove the need for that.

[jedel1043]

Now that I examined the spec in detail, there are two places where a DataDescriptor is created with a Value but without the writable property (not [[writable]] = false, [[writable]] = None if I translate the spec into Rust)

https://tc39.es/ecma262/#sec-createglobalfunctionbinding
https://tc39.es/ecma262/#sec-ordinarysetwithowndescriptor

So, we would need to refactor every flag to be Option<bool> if we want to follow the spec.

[Razican]

Here I'm in favour of option 2. If I understand it correctly, generic descriptors can have a value, but can be empty. That can be represented by an Option, but the rest must have a value.

I think we should use the Rust type system to our advantage to make sure that invalid values cannot be created.

Edit: I would like to know the thoughts of @HalidOdat or @RageKnify