Refactor CESK machine to track environments through new Suspended state
#1928
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
byorgey
force-pushed
the
refactor/suspend
branch
5 times, most recently
from
3635216
to
ed4136c
Compare
|
I know this is a massive refactoring---and no, there's not really any nice way to split it up into more incremental changes that I know of. This was the kind of refactoring where I started making changes and then finally got it to compile again a week later. I'm not necessarily expecting that anyone will be able to audit every line of code. What would be especially helpful is to just check out this branch and play the game for a bit, see if you can break it. I've fixed everything that I knew to check but there are bound to be some lurking bugs. |
|
Every time I think this is ready I find another bug... 😠 |
xsebek
reviewed
Jun 16, 2024
xsebek
reviewed
Jun 16, 2024
byorgey
force-pushed
the
refactor/suspend
branch
2 times, most recently
from
60fa220
to
67e1245
Compare
xsebek
reviewed
Jun 16, 2024
byorgey
force-pushed
the
refactor/suspend
branch
from
67e1245
to
9f4071f
Compare
byorgey
force-pushed
the
refactor/suspend
branch
from
e9db496
to
c903d79
Compare
byorgey
force-pushed
the
refactor/suspend
branch
from
3657edc
to
540fcf9
Compare
|
I think I have (finally!) squashed all the bugs I know about... I played through some of the early classic game (set up automated tree + bit farms, built a robot to automatically construct drill bits by fetching materials from depot robots, etc.) with no issues. Anyone else who would like to take a look at this before we merge it is most welcome! Thanks to @xsebek for the good feedback already. |
byorgey
force-pushed
the
refactor/suspend
branch
from
fc23cde
to
a8e263a
Compare
|
OK, I'm going to merge this now I think... hold onto your hats... 🎩 |
mergify bot
pushed a commit
that referenced
this pull request
Jun 19, 2024
Closes #1948; see that issue for a much more in-depth discussion. Depends on merging #1928 first. A lot of this PR consists in deleting code that is either (1) ugly or (2) overly clever! 🥳 The short version is that the `Store` used to incorporate some laziness + memoization: when a cell was first allocated, it was an unevaluated thunk; it then got evaluated the first time it was referenced. However, this wasn't really needed to handle recursive definitions (which is the only thing we were using it for). Getting rid of it means we can get rid of a lot of weird ugly code needed to wrap free variables in extra calls to `force` and so on. The new and improved `Store` just stores `Value`s, period. A special `VBlackhole` value was added, to be used while evaluating a recursive `let`. Note that `VRef` is no longer really used, but I left it there for use in implementing #1660 . Once we have mutable references we can use them + delay/force to implement lazy cells.
This was referenced Jun 30, 2024
mergify bot
pushed a commit
that referenced
this pull request
Jul 1, 2024
Fixes #2007. After #1928, we handle exceptions a little differently, so it no longer works to simply set the base's CESK machine to an `Up` (exception) state no matter what. Instead we have to first check if the base is running, and if so set it to `Up` as before; if not, do nothing (except clear the REPL prompt).
mergify bot
pushed a commit
that referenced
this pull request
Jul 6, 2024
The bug which necessitated the workaround was fixed in #1928.
xsebek
added a commit
that referenced
this pull request
Aug 9, 2024
```diff
def intersperse
= \n. \f2. \f1.
- if (n > 0) {f1; if (n > 1) {f2} {}; intersperse (n - 1) f2 f1} {} end
+ if (n > 0) {f1; if (n > 1) {f2} {}; intersperse (n - 1) f2 f1} {}
+ end
```
* caused by #1928 where def was made more similar to let
mergify bot
pushed a commit
that referenced
this pull request
Aug 9, 2024
```diff
def intersperse
= \n. \f2. \f1.
- if (n > 0) {f1; if (n > 1) {f2} {}; intersperse (n - 1) f2 f1} {} end
+ if (n > 0) {f1; if (n > 1) {f2} {}; intersperse (n - 1) f2 f1} {}
+ end
```
You can test it with:
```sh
swarm format -i data/scenarios/Challenges/_arbitrage/solution.sw
```
* caused by #1928 where def was made more similar to let
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The big idea of this PR is to add a new type of CESK machine state,
Suspended, which is a state the base robot automatically goes into after completing execution of a program entered at the REPL. It is as if the base robot is still within the local context of any definitions etc. entered previously, just printed out an "intermediate result", and is now waiting to find out what the next term to be executed will be. This allows us to treatdefas syntax sugar forletand results in a much cleaner way to manage the context of definitions, which in turn allows us to remove a whole lot of special cases and fixes several annoying bugs. See #1087 for more explanation and discussion.Things that are deleted (!) by this PR:
TDef(defis now just syntax sugar forlet) (Add the scope of definition to its constructor #997)VResult(we no longer need to return anything other than plain values)FUnionEnv,FDiscardEnv,FLoadEnv(we no longer need machinery to collect up definitions while evaluating terms)ProcessedTerm(just a plainTSyntax(i.e.Syntax' Polytype) is now enough)Module(it only existed to package up some contexts with typechecked terms, but we don't need them anymore)RobotContextandtopContext(we don't need to store those things separately any more, they are just stored in theCESKmachine where they belong)Additions/changes:
Requirementmodule is split intoRequirements.TypeandRequirements.Analysisto avoid circular module importsSuspendedstate for the CESK machinedefandtydefare now allowed anywhere (even nested inside other definitions etc.) sincedef x = y end; zis just syntax sugar forlet x = y in z(see Nested definitions #349)def, sincedefis now a synonym forlet, and consecutivedefs therefore do not require abind.Closes #1087. Fixes #681 (hence also #736, #1796). Fixes #1032. Closes #1047. Closes #997. Fixes #1900. Fixes #1466. Fixes #1424.
See also #636.