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

Fix abrupt checks in DisposableStack.prototype.use #142

Closed
wants to merge 1 commit into from

Conversation

davidot
Copy link

@davidot davidot commented Jan 24, 2023

Currently in DisposableStack.prototype.use it calls GetDisposeMethod without checking for an abrupt completion.
This means that just checking method against undefined does not guarantee it to be a valid method.
This then breaks the type of method for AddDisposableResource and arguably the first check.

By checking for an abrupt completion on GetDisposeMethod first, we can then also gurantee that AddDisposableResource can no longer fail so we can assert this does not happen.

Just to go through the steps:
We call AddDisposableResource and know that: value is not null or undefined and in fact an Object, method is not undefined, present and Callable (via GetDisposeMethod).

So in AddDisposableResource
We take step 2.b, then 2.b.i passes as we have checked this in 4.a of DisposableStack.prototype.use.
Then in step 2.b.ii we call CreateDisposableResource.
Here we know that method is present and callable from GetDisposeMethod via GetMethod thus we go to step 2.a and pass it, returning a normal completion.

Before this change a abrupt completion from GetDisposeMethod could potentially be given to
AddDisposableResource breaking the type of method.
Once GetDisposeMethod is known to not be a abrupt completion there are also no more ways for 
AddDisposableResource to fail.
@rbuckton
Copy link
Collaborator

Thanks! I'd already made these changes in the ECMA262 PR (https://arai-a.github.io/ecma262-compare/?pr=3000), and have even streamlined them more in https://tc39.es/proposal-async-explicit-resource-management/#sec-disposablestack.prototype.use following some minor changes to the AddDisposableResource AO. I'm thinking of pulling those changes back into this repo, but that would be a different change than the one you are proposing here.

@rbuckton
Copy link
Collaborator

This should be handled by #143, but I'll leave this open until after plenary.

@davidot
Copy link
Author

davidot commented Jan 25, 2023

Thanks! I'd already made these changes in the ECMA262 PR (arai-a.github.io/ecma262-compare/?pr=3000), and have even streamlined them more in tc39.es/proposal-async-explicit-resource-management/#sec-disposablestack.prototype.use following some minor changes to the AddDisposableResource AO. I'm thinking of pulling those changes back into this repo, but that would be a different change than the one you are proposing here.

Ah I didn't know that was not quite the same, feel free to close this PR then!

@rbuckton
Copy link
Collaborator

Fixed by #143.

@rbuckton rbuckton closed this Jan 31, 2023
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

Successfully merging this pull request may close these issues.

None yet

2 participants