This repository has been archived by the owner on Feb 12, 2022. It is now read-only.
Make explicit which path conditions apply to a Generator #2224
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hermanventer
approved these changes
Jul 9, 2018
| @@ -43,7 +43,8 @@ function joinGenerators( | |||
| generator1: Generator, | |||
| generator2: Generator | |||
| ): Generator { | |||
| let result = new Generator(realm, "joined"); | |||
| // TODO #2222: Check if `realm.pathConditions` is correct here. | |||
There was a problem hiding this comment.
It is correct. The path conditions are restored when evaluateNodeForEffects returns, so the path would be whatever it was when the fork (that is now being joined) has occurred.
src/methods/widen.js
Outdated
| @@ -97,7 +97,8 @@ export class WidenImplementation { | |||
| e2.createdObjects | |||
| ); | |||
| let createdObjects = new Set(); // Top, since the empty set knows nothing. There is no other choice for widen. | |||
| let generator = new Generator(realm, "widen"); // code subject to widening will be generated somewhere else | |||
| // TODO #2222: Check if `realm.pathConditions` is correct here. | |||
There was a problem hiding this comment.
It is correct here for the same reason as above.
src/realm.js
Outdated
| @@ -195,7 +195,14 @@ export function construct_empty_effects( | |||
| realm: Realm, | |||
| c: Completion = new SimpleNormalCompletion(realm.intrinsics.empty) | |||
| ): Effects { | |||
| return new Effects(c, new Generator(realm, "construct_empty_effects"), new Map(), new Map(), new Set()); | |||
| // TODO #2222: Check if `realm.pathConditions` is always correct here. Consider making it a required parameter. | |||
There was a problem hiding this comment.
The path conditions here should probably be empty. Picking up the current path conditions from the Realm might be the reason why composition does not work.
Release notes: None It used to be a case that the Generator constructor simply harvested whatever was currently set in the realm as the current path condition. This makes it explicit, and fixes some places where likely the wrong path condition was picked up. At least one place caused an issue in InstantRender that is now fixed (but I couldn't extract a small repro). Leaving behind a number of `TODO #2222`s in the code to review once more.
Still leaving behind some #2222 TODOs for further investigation.
NTillmann
force-pushed
the
GeneratorPathConditions
branch
from
c946d93
to
b16f7b8
Compare
There was a problem hiding this comment.
@NTillmann is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Release notes: None
It used to be a case that the Generator constructor simply
harvested whatever was currently set in the realm as the current
path condition.
This makes it explicit, and fixes some places where likely the wrong
path condition was picked up. At least one place caused an issue in
InstantRender that is now fixed (but I couldn't extract a small repro).
Leaving behind a number of
TODO #2222s in the code to review once more.