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

Non-obvious user code execution #7

Open
ptomato opened this issue Apr 10, 2024 · 7 comments
Open

Non-obvious user code execution #7

ptomato opened this issue Apr 10, 2024 · 7 comments

Comments

@ptomato
Copy link

ptomato commented Apr 10, 2024

If I'm reading the proposal correctly, the following code snippet would execute the getter:

const obj = {
  get foo() {
    console.log('user code execution!');
    return 5;
  },
};

// elsewhere

const { foo: void, ...obj1 } = obj;
// or
let obj2;
({ foo: void, ...obj2 } = obj);

I'm not convinced that discarding a binding should cause this kind of action-at-a-distance in the same way that assigning a binding does. Would it be possible to make a destructuring discard binding just skip over that property, instead of executing the getter and discarding the result?

@rbuckton
Copy link
Collaborator

If your goal is to avoid action-at-a-distance, you'll still run afoul of Proxy traps that can cause side-effects.

I don't believe we should special case void here as it means going from { x: void, y: Number } to { x: Number, y: Number } could result in different behavior at runtime. True, it may be bad to depend on such side-effects, but I'd prefer the path where side-effects are consistently applied vs. one where side-effect semantics are more complex.

@rbuckton
Copy link
Collaborator

If you consider alternate representations of void, they all indicate the property should be read:

  • Using _ in an AssignmentPattern:
    let _, obj2;
    ({ foo: _, bar: _, ...obj2 } = obj);
  • Using a custom matcher:
    const Any = { [Symbol.customMatcher]() { return true; } };
    
    const { foo: Any, bar: Any, ...obj2 } = obj;
  • Using {} or not {}:
    const { foo: {} or not {}, bar: {} or not {}, ...obj2 } = obj;

All of these will trigger getters. void should be consistent here.

@ljharb
Copy link
Member

ljharb commented Apr 10, 2024

The analogy i'd apply is void f() still runs the function, it doesn't short-circuit.

@ptomato
Copy link
Author

ptomato commented Apr 11, 2024

To be clear, I don't feel that strongly about this, so if the answer is just no, that's fine with me.

But I do see void f() and const { x: void } = obj as two different things. I don't see side effects as an integral part of discard bindings the way I see them as an integral part of the void operator. (Without side effects, I don't think the void operator has any reason to exist, and also it's pretty obvious from reading void f() that f() is called right there; not hidden and nonlocal like in this case with discard bindings.)

A counter-analogy: While developing Temporal we were encouraged, particularly by implementations, to delete as many observable calls into user code as possible, in cases where the return value wasn't used or could be derived in another way (e.g., prevent Temporal.Now.plainDate(someCalendar).add({ days: 0 }) from calling someCalendar's dateAdd() method.)

@ljharb
Copy link
Member

ljharb commented Apr 11, 2024

ok, void obj.foo then :-p

i totally agree that observable calls into user code should be minimized! but if you've made a getter and destructuring from an object with one, i think there's already expected user code.

@ptomato
Copy link
Author

ptomato commented Apr 11, 2024

ok, void obj.foo then :-p

Yeah, fair. That is also nonlocal.

I was thinking of it more in terms of, the void operator would be how you'd more explicitly request the user code call, if the discard binding didn't do it:

const { foo: void, ...rest } = obj;
void obj.foo;  // yes, I really do want to execute the getter

but if you've made a getter and destructuring from an object with one, i think there's already expected user code.

I buy that that's expected in some use cases, yes. For the use case given in the explainer, "explicitly exclude properties from rest bindings without needing to introduce throw-away temporary variables for that purpose", I would not expect or want the excluded getter to be called. Like, "I want an object with only data properties, consisting of all the properties of obj except foo" I would otherwise write as a loop over the keys of obj, skipping foo, precisely because const { foo: _, ...rest } = obj; would execute the getter.

(But maybe that's a symptom of how my brain has been warped by diving too deep into JS's corners! I bet many people wouldn't anticipate a getter there, or write that loop, or care at all.)

@ljharb
Copy link
Member

ljharb commented Apr 11, 2024

I definitely see that possible interpretation; i love the idea of preventing getter execution, but in JS i've somewhat resigned myself to the reality that the only way to prevent that is to never define getters :-)

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

No branches or pull requests

3 participants