Simplify treatment of getter/setter bindings in private properties transform

[overlookmotel]

I've reviewed the whole class private methods transform. Holy cow! It took a lot of code to do that transform, much more than I expected.

It all looks really good to me, except for one thing. PrivateProp has got very complicated now that it has to represent properties, methods, and getter/setters.

In particular, the logic around checking on getters/setters which binding is which (binding or binding2?) is a bit convoluted.

I have a suggestion:

Turn PrivateProp into an enum:

enum PrivateProp<'a> {
    InstanceProp(BoundIdentifier<'a>),
    StaticProp(BoundIdentifier<'a>),
    InstanceMethod(BoundIdentifier<'a>),
    StaticMethod(BoundIdentifier<'a>),
    InstanceGetter(BoundIdentifier<'a>),
    InstanceSetter(BoundIdentifier<'a>),
    InstanceGetterSetter((BoundIdentifier<'a>, BoundIdentifier<'a>)),
    StaticGetter(BoundIdentifier<'a>),
    StaticSetter(BoundIdentifier<'a>),
    StaticGetterSetter((BoundIdentifier<'a>, BoundIdentifier<'a>)),
    Accessor,
}

This enum encodes all the info from is_static, method_kind and is_accessor in its discriminant.
It's the same size as the previous struct version.

There's no ambiguity about which binding is which for getters/setters, so can set it correctly once in the entry phase of transform, and then it doesn't have to swap binding and binding2 around depending on method_kind over and over during the traversal of class body.

Also it'd remove the need to have "I can unwrap this Option because I know it must have a value because this is a setter" etc logic. Whether it does have a setter binding or not is statically encoded by the type.

I hope that after that change, a lot of the other logic can be simplified. Pretty much all the methods have different branches for properties / methods / getters / setters, and for instance / static, so it makes some sense, I think, to represent all these options in an enum and then match on it.

@Dunqing Do you think this would make things clearer? Or just make it even more complex?

[Dunqing]

It looks great! It can make the implementation clearer.

Do you wanna split different discriminant transformations into separate functions further based on this? This is what I wanted to do before, but it seems to produce numerous repeat codes because they have slight differences.

[overlookmotel]

Glad you like it. But when I asked "would make things clearer?", it was a genuine question. I think it will reduce complexity, but I'm not completely sure. Maybe the complexity just moves somewhere else, but it's still complex!

One thing I don't like in current implementation (including class properties code I wrote) is when we have some invariants that we know, but we've not managed to encode those invariants in the type system. So we have stuff like this:

oxc/crates/oxc_transformer/src/es2022/class_properties/class_bindings.rs

Lines 124 to 127 in fb2acd8

	// `static_private_fields_use_temp` is always `true` for class expressions.
	// Class declarations always have a name binding if they have any static props.
	// So `unwrap` here cannot panic.
	self.name.as_ref().unwrap()

oxc/crates/oxc_transformer/src/es2022/class_properties/private_field.rs

Lines 494 to 495 in fb2acd8

	// At least one of `get_binding` or `set_binding` is always present
	get_binding.unwrap_or_else(|| set_binding.as_ref().unwrap()),

oxc/crates/oxc_transformer/src/es2022/class_properties/private_field.rs

Lines 523 to 525 in fb2acd8

	// Unwrap is safe because `is_method` is false, then static private prop is always have a `get_binding`
	// and `set_binding` and they are always are the same.
	let prop_binding = get_binding.cloned().unwrap();

Probably our assumptions are correct and these can never panic, but it's a bit fragile. Maybe we change some code elsewhere and it breaks our assumptions, and we don't remember to update this code to match that change. The advantage of Rust's type system is that, if we use it well, we can get the compiler to "check our work" at compile time, and tell us if we make a mistake. But we're not really using the type system to its fullest here.

I think probably it's not feasible to represent everything in class transforms via static types, without ending up with crazy generics everywhere - which is probably even worse! But the main motivation for my suggestion in this issue is to at least reduce the number of assumptions which are only checked at runtime via unwrap.

[overlookmotel]

Do you wanna split different discriminant transformations into separate functions further based on this? This is what I wanted to do before, but it seems to produce numerous repeat codes because they have slight differences.

I've read the whole implementation, but of course you know it better than me.

I imagine we'd have to decide on a case-by-case basis depending on the logic, and how much repeat code there is. Maybe:

1. Try to split into separate functions as much as we can.
2. Where that produces too much repeat code, pass &PrivateProp into the function and match on it there:

// We know `prop` is a method in this function
let (binding, is_static) = match prop {
    InstanceMethod(binding) => (binding, false),
    StaticMethod(binding) => (binding, true),
    _ => unreachable!(),
};

If we want go hard on the static typing approach, we could also introduce separate enums which only cover a subset of cases:

/// Reference to a `PrivateProp` which is an instance getter/setter
enum PrivateInstanceGetterSetterRef<'a, 't> {
    InstanceGetter(&'t BoundIdentifier<'a>),
    InstanceSetter(&'t BoundIdentifier<'a>),
    InstanceGetterSetter((&'t BoundIdentifier<'a>, &'t BoundIdentifier<'a>)),
}

impl<'a, 't> From<&'t PrivateProp> for PrivateInstanceGetterSetterRef<'a, 't> {
    fn from(prop: &'t PrivateProp) -> Self {
        match prop {
            InstanceGetter(get_binding) => PrivateInstanceGetterSetterRef::InstanceGetter(get_binding),
            InstanceSetter(set_binding) => PrivateInstanceGetterSetterRef::InstanceSetter(set_binding),
            InstanceGetterSetter((get_binding, set_binding)) => {
                PrivateInstanceGetterSetterRef::InstanceGetterSetter((get_binding, set_binding))
            }
            _ => panic!(),
        }
    }
}

/// Reference to a `PrivateProp` which is a static getter/setter
enum PrivateStaticGetterSetterRef<'a, 't> {
    // ... same kind of thing ...
}

fn handle_anything(prop: &PrivateProp<'a>) {
    match prop {
        InstanceGetter(_) | InstanceSetter(_) | InstanceGetterSetter(_) => {
            let getter_setter = PrivateInstanceGetterSetterRef::from(prop);
            handle_instance_getter_setter(getter_setter);
        }
        StaticGetter(_) | StaticSetter(_) | StaticGetterSetter(_) => {
            let getter_setter = PrivateStaticGetterSetterRef::from(prop);
            handle_static_getter_setter(getter_setter);
        }
        InstanceProp(binding) => handle_prop(binding, /* is_static */ false),
        StaticProp(binding) => handle_prop(binding, /* is_static */ true),
        InstanceMethod(binding) => handle_method(binding, /* is_static */ false),
        StaticMethod(binding) => handle_method(binding, /* is_static */ true),
        Accessor(_) => { /* no idea! */ }
    }
}

fn handle_instance_getter_setter<'t>(getter_setter: PrivateInstanceGetterSetterRef<'a, 't>) {
    // `getter_setter` is definitely an instance getter/setter,
    // NOT a method, NOT a property, and NOT a static getter/setter.
    // Put all common code for instance getters/setters here, rather than repeating it in multiple places.
    match getter_setter {
        InstanceGetter(get_binding) => { ... },
        InstanceSetter(set_binding) => { ... },
        InstanceGetterSetter((get_binding, set_binding)) => { ... },
        // It can't be anything else
    }
}

fn handle_static_getter_setter<'t>(getter_setter: PrivateStaticGetterSetterRef<'a, 't>) {
    // Same for static getters/setters
}

This is similar to the match argument { match_expression!(Argument) { let expr = argument.to_expression(); } } pattern.

Maybe this is taking the static typing too far!

TLDR... I don't think there's a "one size fits all" answer to that question. Sometimes it'll split naturally into separate functions without repeat code, sometimes it won't.