Skip to content
This repository was archived by the owner on Feb 12, 2022. It is now read-only.

Review Generator's path conditions #2222

Closed
NTillmann opened this issue Jul 7, 2018 · 0 comments
Closed

Review Generator's path conditions #2222

NTillmann opened this issue Jul 7, 2018 · 0 comments
Assignees
@hermanventer
Labels
abstract bug Instant Render priority: low

Comments

@NTillmann
Copy link
Contributor

NTillmann commented Jul 7, 2018 •
edited
Loading

Since #2224, when creating a Generator, one must make it explicit which path conditions hold for the generator.

However, this needs to be cleaned up.
Look for all comments of the form TODO #2222 in the code.
NTillmann added a commit that referenced this issue Jul 7, 2018
@NTillmann
Make explicit which path conditions apply to a Generator
c946d93 
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.
NTillmann added a commit that referenced this issue Jul 10, 2018
@NTillmann
Make explicit which path conditions apply to a Generator
ddbb3a5 
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.
NTillmann added a commit that referenced this issue Jul 10, 2018
@NTillmann
Tweaking comments reflecting code review feedback.
b16f7b8 
Still leaving behind some #2222 TODOs for further investigation.
facebook-github-bot pushed a commit that referenced this issue Jul 10, 2018
@NTillmann @facebook-github-bot
Make explicit which path conditions apply to a Generator (#2224)
ca9246c 
Summary:
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.
Pull Request resolved: #2224

Differential Revision: D8779258

Pulled By: NTillmann

fbshipit-source-id: a0f3dea4b03eaa71b12943f813eb7fc440d29fca
@hermanventer hermanventer mentioned this issue Aug 10, 2018
Closed
facebook-github-bot pushed a commit that referenced this issue Aug 12, 2018
@facebook-github-bot
Do full joins (#2402)
7157849 
Summary:
Release note: Rewrote the joining logic to always do a full join at every join point

Closes #2151 #2222 #2279

I've spent a lot of time in the last few months trying to sort out problems that arise from effects being applied too many or too few times. Fixing these feel a bit like playing wack a mole and in the end no fix goes unpunished.

Stepping back a bit from the fray, it seems to me that the root cause of all this pain is the fact that joins of different kinds of completions get delayed.

Before we had path conditions and the simplifier this seemed like a rather good thing since exceptional paths did not contribute values to the normal paths and we thus had fewer abstract values to deal with and fewer places where Prepack would grind to a halt.

In the current state of things, however, it seems perfectly possible to join in all branches at every join point. I've had to decrease some limits, in particular the number of times we go around a loop with conditional exits. I've also had to make the test runner impose a limit on how many times the simplifier can invoke Path.implies.

Nevertheless, the tests seem to pass and hopefully this will also fix quite a lot of bugs that have been unresolved for many months already.
Pull Request resolved: #2402

Differential Revision: D9236263

Pulled By: hermanventer

fbshipit-source-id: 92a25b591591297afeba536429226c5a0291f451
facebook-github-bot pushed a commit that referenced this issue Aug 15, 2018
@facebook-github-bot
Weekly release v0.2.48: Fuzz tester and join logic refactor
90c3aa2 
Summary:
Fixes #2151 #2222 #2279 #2393 #2399 #2404 #2411 #2414 #2415
Added a fuzz testing tool
Added test cases
Turn crash in JSON.stringify into a diagnostic
Adds a `arrayNestedOptimizedFunctionsEnabled` flag to enable nested optimized functions derived from Array.prototype methods (like `map`) and Array.from
Refactor assignment on partial or possibly deleted property
Rewrote the joining logic to always do a full join at every join point
Removed last remnants of delayUnsupportedRequires

Reviewed By: cblappert

Differential Revision: D9349841

fbshipit-source-id: 74a16dbb015777d59d23fdfde77abbe2489c292a
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@hermanventer hermanventer

Labels
abstract bug Instant Render priority: low
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@hermanventer @NTillmann