Error Recovery #1462
Closed
Error Recovery #1462
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
Co-authored-by: Godfrey Chan <[email protected]>
This reverts commit 3e83045.
The infrastructure doesn't *do* anything yet, but it exists end-to-end.
These changes will make it easier to iterate on the low-level VM, which will be necessary in order to add error recovery in appropriate opcodes.
The logging should be significantly more useful, and the metadata is now extensible to support arbitrary assertions. I already added stack type assertions to the metadata, but these are currently still implemented in terms of the old stack change check. The improvements to metadata already improve the logs (because they facilitate improved deserialization of immediate values in operands). The next step is to assert the stack types and also log the (formatted) stack parameters and return values in the log. All of this facilitates quicker iteration on the in-progress error recovery PR.
The devDependencies in package.json have been updated to their latest versions to ensure compatibility and take advantage of any bug fixes or new features. This includes updating various Babel plugins and presets, Rollup, ESLint, Prettier, Puppeteer, QUnit, rimraf, and TypeScript.
Update setup harness and add local types.
…urate Handle `null`s in the metadata more reliably, and add nullable arrays to the metadata.
Also tweak code for better conformance to modern TypeScript.
- Make the debug infrastructure work again - Have the "before" hook return the "after" hook so it can close over state it needs. - Snapshot and formalize internal debug state. - Properly handle errors in tracing so that log groups are properly closed. - Improve the tracing output: - properly visualize the state of the element builder - properly visualize the block stack - improve visualization of the scope - Streamline the interaction between the VM and debugger The next commit uses all of these changes to significantly improve stack verification and debugging metadata.
…gging This commit overhauls opcode metadata: Previously, many opcodes opted out of stack changes because their changes were too dynamic (e.g. Pop reduces the stack size by a parameter, which was previously too dynamic to be checked). This commit enables dynamic stack checks, which are functions that take the runtime op and debug VM state and dynamically determine the stack change. This makes it possible to check the stack for every opcode except `EnterList`, `Iterate`, `PrepareArgs` and `PopFrame` (the reasons for these exceptions is documented in `opcode-metadata.ts`). Dynamic checking is now used in: - Concat (pops N from the stack based on an operand) - Pop (same) - BindDynamicScope (binds a number of names from an operand) A previous commit enabled operand metadata. That infrastructure allows the trace logger and compilation logger to print friendly versions of opcodes. This commit makes the metadata pervasively more accurate, piggy-backing on the previous commit, which made nullable operands more accurate. This deserialization process serves as a sort-of verification pass. If an opcode is expecting an array, for example, and the operand deserializes to a string, it will fail. It currently fails confusingly (and not reliably) when the deserializer fails to deserialize, but this can and should be improved in the future. Enabling pervasive stack checks caused quite a few tests to fail. For the most part, getting the tests to pass required improving the accuracy of the opcode metadata.
Previously, the source of truth for opcodes was an array literal and a matching interface for that literal. All other types were generated using TypeScript reflection. However, the TypeScript reflection was fairly elaborate and slowed down type feedback when working with the types. This commit moves the source of truth to bin/opcodes.json and generates: - @glimmer/interfaces/lib/generated/vm-opcodes.d.ts - @glimmer/vm/lib/generated/opcodes.ts - @glimmer/debug/lib/generated/op-list.ts It adds a lint rule to avoid code casually importing directly from these generated files, preferring import paths that re-export the generated types and values. The schema in `bin/opcodes/opcodes.schema.json` validates `bin/opcodes.json`. An npm script (`generate:opcodes`) updates all three files, but can also be used to update a single file or review the generated files. The generator script is written in TypeScript and this commit adds `esyes` (https://github.com/starbeamjs/esyes) to generate the `bin/opcodes.mts` file. (esyes requires node 21, but that doesn't affect users of Glimmer, just contributors).
There are better, more ergnomic, and more broadly supported ways to handle enum-like patterns in TypeScript now. TL;DR This commit replaces enums with literal types and type unions.
This commit also calls the specified handler asynchronously when an error occurs.
| @@ -20,6 +19,8 @@ export const DebugOpList = [ | |||
| null, | |||
| null, | |||
| null, | |||
| null, | |||
There was a problem hiding this comment.
what is the purpose of these nulls?
| op.labels(() => { | ||
| expr(op, handler); | ||
| op(Op.PushTryFrame, op.target('CATCH')); | ||
| // ReplayableIf( |
There was a problem hiding this comment.
don't forget that this is commented out!
NullVoxPopuli
added a commit
that referenced
this pull request
Oct 29, 2023
|
I extracted the lint dependency updates to here: #1474 |
The VM uses the `Stack` class (and friends) to manage its internal state. This commit adds checkpointing to `Stack` so that the VM can easily roll back to a point before a try frame began.
There are a few remaining open issues, but this commit gets us really close to the finish line.
This commit cleans up and tests the behavior of references in the error state. Documentation for the semantics is forthcoming.
Also improve the labelling infrastructure to be more useful for debugging tools such as the trace logger.
Improve the overall infrastructure of debug labels, and introduce a minifier-friendly approach to dev-mode values that captures the notion of values that should *always* be present in development but never in production.
This commit makes it possible for a code to reset the error state in a reference in order to attempt error recovery. When a reference's error is reset, its tag is invalidated, which will result in consumers of that reference recomputing its value.
This commit removes gratuitous differences between different modes in an attempt to clarify how the system works. It also begins some documentation on how blocks and frames work.
This commit migrates more cases from unwrapReactive to readReactive. Ideally we'd have explicit tests for each `readReactive` case, but there are already some blanket error recovery tests and I'm moving forward with these changes as long as all of the existing tests and blanket error tests pass.
|
I'm going to be helping rebase, but:
|
chancancode
reviewed
Nov 20, 2023
There was a problem hiding this comment.
What's up with this diff?
There was a problem hiding this comment.
– and, in general, now that this is in (from what I understand) a ready for merge/review state, can you back out all these unrelated config/workflow changes into a separate PR and land them first or separately later? Don't want to merge a big experimental new feature (which has a non-zero chance of needed to be reverted later) with a lot of unrelated changes.
chancancode
requested changes
Nov 20, 2023
There was a problem hiding this comment.
I don't think I'll be able to successfully review this in the current state in the amount of time I had allocated for the task. (To be fair, it was still marked as draft, but I was recently told it's ready.)
Let me know when it is cleaned up with the unrelated changes backed out; especially for the formatting changes, I would strongly prefer to have them landed ahead of time and this rebased on top (which, I think @NullVoxPopuli had already started doing).
NullVoxPopuli
added a commit
that referenced
this pull request
Nov 21, 2023
NullVoxPopuli
added a commit
that referenced
this pull request
Nov 21, 2023
This was referenced Nov 21, 2023
Closed
NullVoxPopuli
added a commit
that referenced
this pull request
Nov 21, 2023
NullVoxPopuli
added a commit
that referenced
this pull request
Nov 21, 2023
* Copy bin folder from #1462 * Sync from remote * update lockfile * Do not typecheck vm-babel-plugins, because doing so provides no value
This was referenced Nov 21, 2023
Open
|
Closing due to conflicts and #1501 supersedes |
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.
This pull request implements error recovery in the VM.
More information to come.