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

Normative: Prevent DFS invariants from being broken #1669

Merged
merged 1 commit into from
Mar 23, 2020

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Aug 14, 2019

An implementation of HostImportModuleDynamically, could, in theory, call
Evaluate() in the same tick as the call to HostImportModuleDynamically.
If that call to HostImportModuleDynamically was the direct result of
another call to Evaluate(), this may eagerly evaluate a module that
is already queued to be run at a later time.

cc @guybedford

@ljharb ljharb added needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text labels Aug 15, 2019
Copy link

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

This is a really important invariant of the DFS algorithm not to be interrupted during its search by another search, as the algorithm isn't designed to run in parallel with other algorithms.

This is also an important invariant for the top-level await spec too.

spec.html Outdated Show resolved Hide resolved
Copy link
Contributor

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

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

This makes sense to me.

spec.html Outdated Show resolved Hide resolved
@littledan
Copy link
Member

Should we make this a requirement on host environments, in addition to an assertion?

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM. @littledan's suggestion also makes sense, but I think without #735 (or some other acknowledgment of the fact that hosts are actually going to be the ones driving module evaluation), it's unclear where to put such a requirement.

@ljharb ljharb requested a review from a team September 4, 2019 22:34
ljharb pushed a commit to devsnek/ecma262 that referenced this pull request Sep 5, 2019
An implementation of HostImportModuleDynamically, could, in theory, call
Evaluate() in the same tick as the call to HostImportModuleDynamically.
If that call to HostImportModuleDynamically was the direct result of
another call to Evaluate(), this may eagerly evaluate a module that
is already queued to be run at a later time.
@ljharb ljharb added has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Oct 1, 2019
@ljharb ljharb self-assigned this Oct 1, 2019
@ljharb
Copy link
Member

ljharb commented Oct 16, 2019

@devsnek @guybedford any update on the test262 tests for this change, or, alternatively, a persuasive explanation of why this is untestable?

@devsnek
Copy link
Member Author

devsnek commented Oct 16, 2019

I don't think we can create tests that exhaustively prove this invariant is held. We might be able to add a small test for a case where import() runs synchronously(??).

@ljharb
Copy link
Member

ljharb commented Oct 16, 2019

That'd be great!

devsnek added a commit to devsnek/test262 that referenced this pull request Feb 19, 2020
devsnek added a commit to devsnek/test262 that referenced this pull request Feb 19, 2020
@devsnek
Copy link
Member Author

devsnek commented Feb 19, 2020

Test: tc39/test262#2506

devsnek added a commit to devsnek/test262 that referenced this pull request Feb 21, 2020
rwaldron pushed a commit to tc39/test262 that referenced this pull request Mar 23, 2020
@devsnek devsnek added has test262 tests and removed needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Mar 23, 2020
An implementation of HostImportModuleDynamically, could, in theory, call
Evaluate() in the same tick as the call to HostImportModuleDynamically.
If that call to HostImportModuleDynamically was the direct result of
another call to Evaluate(), this may eagerly evaluate a module that
is already queued to be run at a later time.
@ljharb ljharb merged commit 4b6d77d into tc39:master Mar 23, 2020
@devsnek devsnek deleted the fix/evaluate-timing branch March 24, 2020 00:03
pull bot pushed a commit to p-g-krish/v8 that referenced this pull request Apr 2, 2020
https://chromium.googlesource.com/external/github.com/tc39/test262/+log/f6b2ccdd..ae8694b4

ae8694b Copy "invalid options" test from RelativeTimeFormat to NumberFormat/DateTimeFormat by André Bargull · 2 days ago
299cd74 Promise.any: fix "invoke-then.js" test by Rick Waldron · 2 days ago
9ccd3a7 Test revoked callable [[ProxyTarget]] by Alexey Shvayka · 2 days ago
91b867b Fix non-callable [[ProxyTarget]] test by Alexey Shvayka · 2 days ago
e8e3aaa Test revoked [[ProxyHandler]] by Alexey Shvayka · 2 days ago
8b610fb Test revoked [[ProxyTarget]] by Alexey Shvayka · 2 days ago
432adbb Adding case where 'static' is used as a field name by Caio Lima · 4 days ago
4bf836c Merge pull request #2533 from tc39/promise-any by Leo Balter · 5 days ago
22be03d Promise.any: lint fixes by Rick Waldron · 5 days ago
d53f45d Promise.any: remove unnecessary static resolve def by Rick Waldron · 7 days ago
499b748 Promise.any: review fixes, 2 by Rick Waldron · 7 days ago
b21b0c1 Promise.any: additional "resolve from rejection" tests by Rick Waldron · 7 days ago
a05fb94 Promise.any: feature flags, 2 by Rick Waldron · 7 days ago
7fbce5a Promise.any: simplify promise creation by Rick Waldron · 7 days ago
5c68b60 Promise.any: cleanup in @@species tests by Rick Waldron · 7 days ago
d9265df Promise.any: additional then + resolve tests by Rick Waldron · 7 days ago
1c74850 Promise.any: feature flags by Rick Waldron · 7 days ago
5d3eafc Promise.any: review fixes by Rick Waldron · 7 days ago
f70e3e3 Features: remove duplicate "Promise.any" by Rick Waldron · 7 days ago
6edaba3 Promise.any: updates, corrections and new tests. by Rick Waldron · 7 days ago
e0f0c78 Promise.any: empty iterable rejects with AggregateError by Rick Waldron · 7 days ago
272e9ab Promise.any: make async operation test actually async by Rick Waldron · 7 days ago
47b3858 Promise.any: expected rejection shouldn't end with error message. by Rick Waldron · 7 days ago
55b22d8 Promise.any: string iterable should not be rejected (adds error message for clarity) by Rick Waldron · 7 days ago
e0abeaa Promise.any: convert sync test to async test to ensure run to completion by Rick Waldron · 7 days ago
860e02a add iter-assigned tests by chicoxyzzy · 7 days ago
9567abd Add iter-arg tests by chicoxyzzy · 7 days ago
094ddc7 add species-get-error test by chicoxyzzy · 7 days ago
e3d48f2 add Invocation of the instance's `then` method test by chicoxyzzy · 7 days ago
afe3f0b add reject-immed test by chicoxyzzy · 7 days ago
adcd162 add is callable test by chicoxyzzy · 7 days ago
88d058b Add returns promise test by chicoxyzzy · 7 days ago
be1bf63 Add Promise and Promise.any properties tests by chicoxyzzy · 7 days ago
23d7f0b add Invocation of the constructor's `resolve` method test by chicoxyzzy · 7 days ago
f0fd4e0 update features.txt by chicoxyzzy · 7 days ago
aca1084 Add context tests by chicoxyzzy · 7 days ago
715964b Add Capability Executor tests by chicoxyzzy · 7 days ago
96cf757 Harness: properly format negative zero by Alexey Shvayka · 7 days ago
1e63ce0 Improve String.prototype.@@replace poisoned stdlib test by Alexey Shvayka · 7 days ago
9b71a7c Fix inconsistent indentation in YAML by Alexey Shvayka · 7 days ago
ad046ce Avoid using Array.isArray directly by Alexey Shvayka · 7 days ago
8fe71e1 Drop JSON.stringify stack overflow tests by Alexey Shvayka · 7 days ago
dfc7ecc AggregateError: If NewTarget is undefined, let newTarget be the active function object (#2537) by Rick Waldron · 9 days ago
db6f630 Adding test coverage for 'OptionalChain'.PrivateIdentifier case (#2534) by Caio Lima · 9 days ago
344612b Fix: Proxy set, if trap is undefined (#2536) by Rick Waldron · 9 days ago
79146e5 Bring back `verifyProp` param to `isWritable` by Alexey Shvayka · 9 days ago
3bf630c Add for/in test by Alexey Shvayka · 9 days ago
33b9bba Add Reflect.ownKeys test by Alexey Shvayka · 9 days ago
b273aff Add Object.values test by Alexey Shvayka · 9 days ago
5874ca4 Add Object.keys test by Alexey Shvayka · 9 days ago
c370276 Add Object.getOwnPropertySymbols test by Alexey Shvayka · 9 days ago
62c9541 Add Object.getOwnPropertyNames test by Alexey Shvayka · 9 days ago
021b8f2 Add Object.getOwnPropertyDescriptors test by Alexey Shvayka · 9 days ago
b94190e Add Object.entries test by Alexey Shvayka · 9 days ago
2abfc8c Remove unused `verifyProp` paramter from `isWritable` by Alexey Shvayka · 9 days ago
86b9409 Increase unlikely array length by Alexey Shvayka · 9 days ago
17fc109 Fix isWritable throwing RangeError on Array "length" by Alexey Shvayka · 9 days ago
1eff480 Add toJSON stack overflow test by Alexey Shvayka · 9 days ago
2255a0f Add replacer stack overflow test by Alexey Shvayka · 9 days ago
d2b5f63 Improve "info" meta of replacer with deleted property test by Alexey Shvayka · 9 days ago
dc21d6b Add "lastIndex" restore test by Alexey Shvayka · 9 days ago
fe2dfe9 Add "lastIndex" init test by Alexey Shvayka · 9 days ago
fe4e96d Remove duplicate test regexp/u-dec-esc.js by Ross Kirsling · 9 days ago
b0bb917 add dfs tests for tc39/ecma262#1669 by Gus Caplan · 9 days ago
b59d079 Add note on arbitrary large integer by Alexey Shvayka · 9 days ago
669250d RegExp: Test Quantifier with large integer by Alexey Shvayka · 9 days ago
8dccb69 Replace "\b" with "\u0008" by Alexey Shvayka · 9 days ago
2377131 Test \b escape inside CharacterClass in Unicode RegExp by Alexey Shvayka · 9 days ago
7117cdd Test astral literals within inverted CharacterClass by Alexey Shvayka · 9 days ago
2cae203 Add functional replacer with empty result test by Alexey Shvayka · 9 days ago
8e41e8b Add "lastIndex" length abrupt coercion test by Alexey Shvayka · 9 days ago
0485b83 Add "lastIndex" length coercion test by Alexey Shvayka · 9 days ago
807afd9 Make "length" coercion test more precise by Alexey Shvayka · 9 days ago
3f6b961 Make "index" integer coercion test more precise by Alexey Shvayka · 9 days ago
53d16ac Add "index" integer coercion test with functional replacer by Alexey Shvayka · 9 days ago
e3e0e0f Add named capture groups abrupt lookup tests by Alexey Shvayka · 9 days ago
c9e1c1c Add named capture groups abrupt coercion tests by Alexey Shvayka · 9 days ago
4d3db14 Add named capture groups coercion tests by Alexey Shvayka · 9 days ago
dbbe2e7 Add "0" string coercion test with global RegExp by Alexey Shvayka · 9 days ago
50d1419 Make string coercion tests more precise by Alexey Shvayka · 9 days ago
cf583c9 Add poisoned stdlib test by Alexey Shvayka · 9 days ago
67e58de Fix typo in file name by Alexey Shvayka · 9 days ago
fea38b7 Remove extra operation from "info" by Alexey Shvayka · 9 days ago
13082b0 Add Array#concat test by Alexey Shvayka · 9 days ago
679ad48 Add Array#reduceRight test by Alexey Shvayka · 9 days ago
2716290 Add Array#fill test by Alexey Shvayka · 9 days ago
66913bf Add Array#lastIndexOf test by Alexey Shvayka · 9 days ago
4d91ea0 Add Array#indexOf test by Alexey Shvayka · 9 days ago
5885db1 Add Array#copyWithin test by Alexey Shvayka · 9 days ago
2724ddc Dependency: test262-harness@7.3.0 by Rick Waldron · 9 days ago
f72db7e Add [[Call]] tests by Alexey Shvayka · 9 days ago
5c3ea18 Add [[Construct]] tests by Alexey Shvayka · 9 days ago
aa53649 Add [[HasProperty]] tests by Alexey Shvayka · 9 days ago
6155fca Add [[Get]] tests by Alexey Shvayka · 9 days ago
2d60dc0 Add [[Set]] tests by Alexey Shvayka · 9 days ago
983b1ac Add [[OwnPropertyKeys]] tests by Alexey Shvayka · 9 days ago
cc6d48d Add [[PreventExtensions]] tests by Alexey Shvayka · 9 days ago
3987d3c Add [[IsExtensible]] tests by Alexey Shvayka · 9 days ago
d32db7a Add [[SetPrototypeOf]] tests by Alexey Shvayka · 9 days ago
20c2ce3 Add [[GetPrototypeOf]] tests by Alexey Shvayka · 9 days ago
b9377e7 Add [[GetOwnProperty]] tests by Alexey Shvayka · 9 days ago
d46e72d Add [[Delete]] tests by Alexey Shvayka · 9 days ago
6b9929a Add [[DefineOwnProperty]] tests by Alexey Shvayka · 9 days ago
2c432e3 Add coverage for EvaluateNew by Leo Balter · 2 weeks ago
b29b1da Add tests for new identifier characters per Unicode version (#2532) by Mathias Bynens · 2 weeks ago
991e05d Move: AggregateError belongs in the NativeErrors directory (#2528) by Rick Waldron · 2 weeks ago
25c9e33 Update RegExp property escape tests per Unicode v13.0.0 (#2526) by Mathias Bynens · 3 weeks ago
22cd9fe Repair filter test that was testing concat (#2522) by Steven Cole · 3 weeks ago
b0cb75f Repair indexOf test that was testing lastIndexOf (#2521) by Steven Cole · 3 weeks ago
36882a2 Fix: AggregateError/newtarget-proto-fallback. Closes gh-2515 (#2518) by Rick Waldron · 3 weeks ago
800870c Fix DST sensitivity in Date/parse/without-utc-offset.js. (#2523) by Ross Kirsling · 3 weeks ago
7631789 add graaljs test runner by Gus Caplan · 4 weeks ago
56ae1b9 switch to esvu to streamline engine installs by Gus Caplan · 4 weeks ago
ca13f22 Fix lint.exceptions, I hope? by Ross Kirsling · 4 weeks ago
70bbce9 Add lint exception. by Ross Kirsling · 4 weeks ago
df7a0b0 Remove errant LF from CR line ending test again. by Ross Kirsling · 4 weeks ago
ec6239f typo (#2512) by Claude Pache · 5 weeks ago

Bug: v8:7834, v8:9808, v8:10379, v8:10380, v8:10381, v8:10382, v8:10383, v8:10272
Change-Id: I9c46af4f0d72a2f1e032ec1c80d40ca096ce9af9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2133311
Reviewed-by: Shu-yu Guo <syg@chromium.org>
Commit-Queue: Frank Tang <ftang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#66970}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants