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

Add missing checks to Promise.resolve and Promise.reject. #379

Merged
merged 1 commit into from
Dec 13, 2015

Conversation

cscott
Copy link
Collaborator

@cscott cscott commented Dec 3, 2015

These checks were overlooked when @@species was resolved from these methods.

Discovered by running the ES2015 test262 test suite against es6-shim.

@ljharb
Copy link
Collaborator

ljharb commented Dec 3, 2015

Ah, thanks - could we add test coverage for these to this repo also?

@ljharb
Copy link
Collaborator

ljharb commented Dec 11, 2015

These checks are already covered by IsConstructor inside PromiseCapability - I added 3d5bc93 to be sure.

Can you share the test262 test conditions that cause a failure without this PR's changes?

@cscott
Copy link
Collaborator Author

cscott commented Dec 12, 2015

It's the built-ins/Promise/resolve/context-non-object-with-promise.js test case which is failing. It passes undefined as the context object, but a valid Promise as the argument, so it never gets to the check in IsConstructor. I added PR #381 to demonstrate how test262 test cases could be run on es6-shim, which should let you reproduce the failure.

ljharb added a commit that referenced this pull request Dec 12, 2015
@ljharb
Copy link
Collaborator

ljharb commented Dec 12, 2015

The only difference I see in the test262 tests is that they're voiding the constructor property on the promise they pass in - however, looking at es6-shim's code path for resolve/reject, they both immediately do effectively new PromiseCapability(this), which in its first line throws if IsConstructor fails for the argument, which it would for any non-function "this" value.

I definitely see the test failing in #381 but I'm mystified as to why.

@ljharb
Copy link
Collaborator

ljharb commented Dec 12, 2015

aha, although Promise.reject already works fine, Promise.resolve has an ES.IsPromise check inside it.

@ljharb
Copy link
Collaborator

ljharb commented Dec 12, 2015

@cscott Why does resolve have that check but not reject?

@ljharb
Copy link
Collaborator

ljharb commented Dec 12, 2015

I'm pursuing that question with the committee members, but in the meantime, if you could freshly rebase this on top of latest master I'd love to merge it ASAP. Thanks!

These checks were overlooked when @@species was resolved from these
methods.

Discovered by running the ES2015 test262 test suite against es6-shim.
@cscott
Copy link
Collaborator Author

cscott commented Dec 12, 2015

Rebased.

ljharb added a commit that referenced this pull request Dec 13, 2015
[Fix] Add missing checks to `Promise.resolve` and `Promise.reject`
@ljharb ljharb merged commit 42c0217 into master Dec 13, 2015
@ljharb
Copy link
Collaborator

ljharb commented Dec 13, 2015

Thanks!

@paulmillr paulmillr deleted the promise-checks branch June 25, 2016 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants