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

Editorial: Fix wrong assertion in IsLessThan #2456

Merged
merged 1 commit into from
Jul 13, 2021

Conversation

h2oche
Copy link
Contributor

@h2oche h2oche commented Jul 13, 2021

In step 4.d, 4.e of 7.2.13 IsLessThan algorithm, current spec use ! assertion on ToNumeric call. However, it seems that abrupt completion can be returned from ToNumeric calls in step 4.d and step 4.e. px and py can be symbol objects and ToNumber call in ToNumeric algorithm emits an abrupt completion, which is a TypeError exception. Here is JS code triggering assertion failure in current specification:

Object.defineProperty(Symbol.prototype, Symbol.toPrimitive, { value: undefined });
Object(Symbol()) <= ""; // should throw TypeError, but assertion fail in spec

According to test262 and chrome browser, the above code should throw TypeError exception. Changing ! to ? in step 4.d and 4.e seems to be a plausible fix.

Related test262 cases: https://github.com/tc39/test262/blob/main/test/built-ins/Symbol/prototype/Symbol.toPrimitive/redefined-symbol-wrapper-ordinary-toprimitive.js

@ljharb
Copy link
Member

ljharb commented Jul 13, 2021

This sounds correct to me; good catch.

@ljharb ljharb requested review from michaelficarra, syg, bakkot and a team July 13, 2021 04:21
Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this just looks like a mistake made during the refactoring from "Abstract Relational Comparison", prior to that it used ?

@bakkot
Copy link
Contributor

bakkot commented Jul 13, 2021

this just looks like a mistake made during the refactoring from "Abstract Relational Comparison", prior to that it used ?

I'm not sure what you're looking at; #2378 didn't touch this. This was introduced in #2007.

Anyway, the analysis and fix in the OP seems correct.

@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Jul 13, 2021
@ljharb ljharb force-pushed the AbstractRelationalComparison branch from 0dd806c to 2b2b456 Compare July 13, 2021 05:13
@ljharb ljharb merged commit 2b2b456 into tc39:master Jul 13, 2021
mathiasbynens pushed a commit to mathiasbynens/ecma262 that referenced this pull request Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land. spec bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants