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

Make errors property of AggregateError non-enumerable #31

Merged
merged 6 commits into from Aug 3, 2019

Conversation

@chicoxyzzy
Copy link
Member

commented Jul 24, 2019

Per July 2019 TC39 meeting consensus

@bakkot

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2019

Also need to update the AggregateError constructor itself.

@chicoxyzzy chicoxyzzy force-pushed the chicoxyzzy:non-enumerable-aggregateerror branch from 96937ec to 4152413 Jul 24, 2019

spec.html Outdated Show resolved Hide resolved

@chicoxyzzy chicoxyzzy force-pushed the chicoxyzzy:non-enumerable-aggregateerror branch from 4152413 to c0d0771 Jul 24, 2019

spec.html Outdated Show resolved Hide resolved
@bakkot

bakkot approved these changes Jul 25, 2019

@chicoxyzzy

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

cc @rbuckton for review since AggregateError is also used in https://github.com/tc39/proposal-explicit-resource-management.

spec.html Outdated
1. Return *undefined*.
</emu-alg>
<p>The `"length"` property of a `Promise.any` resolve element function is 1.</p>
</emu-clause>
</emu-clause>

<emu-clause id="sec-promise.any-throwaggregateerror">

This comment has been minimized.

Copy link
@chicoxyzzy

chicoxyzzy Jul 25, 2019

Author Member

I think that this shouldn't be under Promise.any section. I should make it more general

spec.html Outdated
1. Let _error_ be a newly created `AggregateError` object.
1. Perform ! CreateDataProperty ( _error_, `"errors"`, _errorsArray_ ).
1. Return ThrowCompletion ( _error_ ).
1. Return ThrowAggregateError(_errors_).

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 25, 2019

Member

An alternative would be Perform ? ThrowAggregateError, which might better match the intended usage and also the naming (ie, throw AggregateError)

This comment has been minimized.

Copy link
@chicoxyzzy

chicoxyzzy Jul 25, 2019

Author Member

Does Perform also return a value?

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 25, 2019

Member

No, but the ? will ReturnIfAbrupt.

This comment has been minimized.

Copy link
@bakkot

bakkot Aug 3, 2019

Collaborator

Hm, I prefer Return ThrowAggregateError(_errors_); it seems a little silly to have a If argument is an abrupt completion test when you know that it will always be an abrupt completion. But whatever; I'll defer to the editors.

@@ -132,7 +138,8 @@ <h1>AggregateError ( errors, message )</h1>
1. Let _errorsIterator_ be ? GetMethod(_errors_, @@iterator).
1. Let _errorsList_ ? IterableToList(_errors_, _errorsIterator_).
1. Let _errorsArray_ be CreateArrayFromList(_errorsList_).
1. Perform ! CreateDataProperty(_O_, `"errors"`, _errorsArray_).
1. Let _errorsDesc_ be the PropertyDescriptor { [[Value]]: _errorsArray_, [[Writable]]: *true*, [[Enumerable]]: *false*, [[Configurable]]: *true* }.

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 25, 2019

Member

maybe these two steps should be their own abstract op, so it can be reused in both places?

This comment has been minimized.

Copy link
@chicoxyzzy

chicoxyzzy Jul 25, 2019

Author Member

I just found other places in current ecma262 spec text where such abstract operation could be reused (plus one usage for message property 3 lines below these two lines). It will need a lot arguments though. IMO duplication here is not a big problem but maybe worth a separate PR to the spec repo

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 25, 2019

Member

Fair point.

This comment has been minimized.

Copy link
@rbuckton

rbuckton Jul 25, 2019

"CreateBuiltinDataPropertyOrThrow"? Naming based on "CreateBuiltinFunction" and "CreateDataPropertyOrThrow", given that the majority of "built in" data properties are non-enumerable by default.

<emu-alg>
1. Let _errorsArray_ be CreateArrayFromList(_errors_).
1. Let _error_ be a newly created `AggregateError` object.
1. Let _errorsDesc_ be the PropertyDescriptor { [[Value]]: _errorsArray_, [[Writable]]: *true*, [[Enumerable]]: *false*, [[Configurable]]: *true* }.

This comment has been minimized.

Copy link
@rbuckton

rbuckton Jul 25, 2019

Is there a reason not to just use ? Construct(%AggregateError%, « errors »), since the %AggregateError% constructor already has the defined semantics?

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 25, 2019

Member

what about the message argument?

This comment has been minimized.

Copy link
@rbuckton

rbuckton Jul 25, 2019

We don't seem to care about message elsewhere in the spec as the actual message is implementation dependent and may even be localised, yet this isn't formalized anywhere in the spec for the numerous "throw a TypeError exception" cases. I was wondering if we need to be more formal about this in general. Something like:

  1. Let message be a host implementation defined message.
  2. Perform ? Throw(%Error%, « message »).

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 25, 2019

Member

i suspect creation of errors typically avoids using Construct precisely to avoid the need for that formalism.

This comment has been minimized.

Copy link
@bakkot

bakkot Jul 25, 2019

Collaborator

Not just message, but also stack, line, and any other non-standard properties.

@chicoxyzzy

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2019

I think that the PR is ready to merge now. PTAL

spec.html Show resolved Hide resolved
@ljharb

ljharb approved these changes Aug 3, 2019

@chicoxyzzy chicoxyzzy merged commit 865847c into tc39:master Aug 3, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@chicoxyzzy chicoxyzzy deleted the chicoxyzzy:non-enumerable-aggregateerror branch Aug 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.