Error Recovery #1501
Open
Error Recovery #1501
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
NullVoxPopuli
force-pushed
the
squashed-feature-handle-error
branch
11 times, most recently
from
3f62a88
to
63d4ebf
Compare
chancancode
reviewed
Nov 22, 2023
chancancode
reviewed
Nov 22, 2023
chancancode
reviewed
Nov 22, 2023
chancancode
reviewed
Nov 22, 2023
NullVoxPopuli
force-pushed
the
squashed-feature-handle-error
branch
3 times, most recently
from
6714c2f
to
e6e4bd6
Compare
|
@chancancode I don't think we should rollback the rename -- there isn't a 1 to 1 mapping between all the concepts. |
NullVoxPopuli
force-pushed
the
squashed-feature-handle-error
branch
2 times, most recently
from
a41a6e2
to
025c2dd
Compare
This was referenced Dec 5, 2023
NullVoxPopuli
force-pushed
the
squashed-feature-handle-error
branch
3 times, most recently
from
6fce3b9
to
74182cc
Compare
NullVoxPopuli
force-pushed
the
squashed-feature-handle-error
branch
3 times, most recently
from
3bca60d
to
3bad038
Compare
Merged
NullVoxPopuli
force-pushed
the
squashed-feature-handle-error
branch
2 times, most recently
from
8658e80
to
b59b9c3
Compare
Co-authored-by: Godfrey Chan <godfreykfc@gmail.com> Revert "Starting work on error recovery" This reverts commit 3e83045. Added error recovery machine infrastructure The infrastructure doesn't *do* anything yet, but it exists end-to-end. Consolidate stack internals Make the tests work with trace logging on Remove fetchValue(machine register) Clean up stack and debug logging In progress updating stack checks 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. Finish overhauling debug logging 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. Cleanup Tweak stack checks chore(package.json): update devDependencies versions to latest 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 qunit Update setup harness and add local types. chore(vscode): update editor.codeActionsOnSave settings to prioritize source.fixAll over source.formatDocument feat(vscode): add custom inline bookmark mapping for @fixme to highlight and style fixme comments in code feat(vscode): add custom style for @fixme inline bookmarks to make them bold and display in red color editor(vscode): Add commit message editor to vscode refactor(debug): Make nulls and arrays in stack verification more accurate Handle `null`s in the metadata more reliably, and add nullable arrays to the metadata. chore: Clean up unused code Also tweak code for better conformance to modern TypeScript. infra(debugger): Improve and restore the debug tracing infra - 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. infra(debugger): Significant improvement to stack checks and trace logging 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. style: Style tweaks infra(opcodes): Generate opcode types and values 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). style: Style changes refactor(typescript): Replace ContentType and CurriedType enums 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. feat(error-boundary): Basic stack error recovery works This commit also calls the specified handler asynchronously when an error occurs. refactor(opcodes): Nail down opcode behavior Refactors the opcode metadata to be much more precise about how the opcodes behave. This is in service of properly accounting for possible throwing behavior in opcodes. refactor(tests): Prepare for error recovery tests The plan is to put a no-op {{#-try}} around everything. refactor(tests): Further trim down render delegate To maximize the applicability of the no-op error recovery tests. refactor(tests): Further trim down render delegate Render delegates now only need to supply two pieces of DOM information: - the document - the initial element Everything else is derived in the singular `RenderTest` class. chore(checkpoint): Tests pass fix(tests): Support else blocks in glimmer tests Previously, the test suite skipped Glimmer-style templates for any tests that yielded to `else` or `inverse`. In general, the test suite uses a matrix test style to describe templates and invocations abstractly (a "blueprint"), and then compiles the blueprint into each of the styles. It's possible to compile `else` blocks in Glimmer: ```hbs <TestComponent> <:default as |block params|> </:default> <:else> </:else> </TestComponent> ``` This is not purely pedantic: the current test suite fails to test that `yield to="inverse"` actually yields to `<:else>` blocks and this fix therefore improves coverage of the public API. refactor(tests): Separate invoke & template styles The test harness previously didn't provide a way to test attributes passed to classic components. This commit makes it possible to use the Glimmer invocation style, which supports attributes, alongside templates implemented using classic features. Example: ```ts @test({ invokeAs: 'glimmer' }) 'with ariaRole specified as an outer binding'() { this.render.template( { layout: 'Here!', attributes: { id: '"aria-test"', class: '"foo"', role: 'this.ariaRole' }, }, { ariaRole: 'main' } ); this.assertComponent('Here!', { id: 'aria-test', class: classes('ember-view foo'), role: 'main', }); this.assertStableRerender(); } ``` This is a test that uses a classic-style component manager and implementation strategy but uses a Glimmer invocation style. This allows us to test that attributes passed to classic components are preserved. test(error-recovery): Add error recovery suites This commit makes it possible to take any existing test suite and add tests for no-op error recovery. This works by wrapping each template with `{{#-try this.handleError}}` and adding minor infrastructure to merge the wrapping properties with the original properties. This commit adds error recovery testing to initial render tests, but more tests will be added in future commits. The implementation assumes that templates all funnel through `this.renderTemplate` in the test delegate, but this may not be accurate, especially in rehydration tests. chore(eslint): Turn on stricter testing lints Turn on (largely) the full gamut of lints we use in the codebase across the workspace packages, **including in integration tests**. Also restrict test packages in `packages/@glimmer-workspace` from directly importing their parent package via relative imports. feat(error-boundary): DOM clearing works This commit finally gets to the meat of the matter. This (passing) test is a good way to see where we're at. ```ts @render 'error boundaries can happen in nested context'() { const actions = new Actions(); class Woops { get woops() { actions.record('get woops'); throw Error('woops'); } } this.render.template( stripTight` <p> {{this.outer.before}}| {{#-try this.handler}} {{this.inner.before}} |message: [{{this.woops.woops}}]| {{this.inner.after}} {{/-try}} |{{this.outer.after}} </p> `, { woops: new Woops(), outer: { before: 'outer:before', after: 'outer:after', }, inner: { before: 'inner:before', after: 'inner:after', }, handler: (_err: unknown, _retry: () => void) => { actions.record('error handled'); }, } ); actions.expect(['get woops', 'error handled']); this.assertHTML('<p>outer:before||outer:after</p>'); } ``` TL;DR when the error is handled, the surrounding try block is cleared, and execution resumes after the try block. If the error is not handled, the DOM is still cleared, but the error is rethrown at the top-level once evaluation is complete. Next steps: - Make sure that other VM state is cleared properly (especially the block tracking state). - Handle all errors that occur in user code. The current code handles errors thrown while appending content, but there are more cases. The good news is that the infrastructure for handling and propagating errors should Just Work for the remaining cases. - Do a consistent job of making sure that errors that occur in the updating VM are handled properly. For example, if an error occurs in the updating part of an append opcode, that should consistently clear back to the nearest `#-try` block. At the moment, those blocks are not represented in the updating VM. There's more work to do, but the crux of the conceptual and prototyping work needed to support error boundaries is now done. refactor(references): Bake errors in more deeply This commit significantly overhauls the reference architecture to bake errors in at a deeper level. After this commit, references have an additional `error` field that can be set to indicate that a reference is in the error state. The internal reference API was overhauled to explicitly support error cases, most notably via a new `readReactive` function, which returns a `Result<T, UserException>`. If the reference is in the error state, `readReactive` returns an `Err`. There is also an `unwrapReactive` function, which throws an exception if the the reference is in the error state. This replaces the `valueForRef` function. This commit adds new primitives and updates the terminology used in the reactive API to reflect the error recovery changes (and also to align with Starbeam's terminology). - `MutableCell`: a new internal primitive that reflects a single piece of mutable data. - `ReadonlyCell`: a new internal primitive that reflects a single piece of immutable data. - `DeeplyConstant`: replaces the `unbound` collection of APIs. Where `ReadonlyCell` is shallowly immutable, `DeeplyConstant` values produce `DeeplyConstant` property reactives. - `FallibleFormula`: A fallible formula is a read-only function that can throw errors. If the compute function throws an error, the `FallibleFormula` will be in an error state. - `InfallibleFormula`: An infallible formula is a read-only function that cannot throw errors. If the compute function throws an error, the `InfallibleFormula` will be in an error state. - `Accessor`: A read-write formula is an `InfallibleFormula` on the read side and also has an `update` side. At the moment, virtually all uses of `valueForRef` have been updated to use `unwrapReactive`. This is an acceptable (but not ideal) transition path inside of combinators (such as `concatReference`) because they can use `FallibleFormula` to handle the error. The VM, on the other hand, must not use `unwrapReactive` directly (and instead must use the previously committed `vm.deref` and `vm.unwrap` methods to handle errors in a VM-aware way). Ultimately, combinators should also not use `unwrapReactive`, to avoid needing so many `try/catch`es, but that can come after the VM usages have been updated. refactor(test-infra): Restructure matrix tests This commit introduces a new way to write matrix integration tests that doesn't rely on JS inheritance. This makes it easier to mix-and-match different parts of the tests and also makes it easier to add error recovery tests. feat(error-recovery): Recover more VM getErrorMessage And write tests. feat(error): Better rationalize unwindable state feat(errors): Added checkpointing to `Stack` 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. feat(errors): Closing in! There are a few remaining open issues, but this commit gets us really close to the finish line. feat(errors): Test and clarify references This commit cleans up and tests the behavior of references in the error state. Documentation for the semantics is forthcoming. feat(references): More fully test ref API Also improve the labelling infrastructure to be more useful for debugging tools such as the trace logger. feat(debug): Better descriptions/devmode values 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. feat(errors): Support error recovery 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. chore(imports): Strengthen and apply import sort rules feat(errors): Get closer to final naming docs(reactive): Document the new reactivity APIs feat(errors): Make recover() work feat(errors): Finish error recovery + tests refactor(vm): Refactor and document VM 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. feat(errors): Handle errors in more cases 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. Workspaces need a name, especially nested ones Krausest benchmark needs a lint config to properly configure sourceType Delete dom-change-list Cherry-pick files from 'master' branch deps packages format lint sync reduce diff eh eh reduceLock eh eh Tell vite to use our custom meta.env variable woo
* Add @glimmer/debug as a runtime dep for @glimmer/runtime
Not 100% sure if this is intentional or unintentional leakage, but
the imports made its way to the `dist` output.
* Use relative imports for package-self-reference
AFAIK this is supposed to work, but TypeScript appears to be having
issues with it on the Ember side:
```
../glimmer-vm/dist/@glimmer/interfaces/lib/references.d.ts:1:38 - error TS2307: Cannot find module '@glimmer/interfaces' or its corresponding type declarations.
1 import type { DevMode, Result } from '@glimmer/interfaces';
~~~~~~~~~~~~~~~~~~~~~
../glimmer-vm/dist/@glimmer/interfaces/lib/runtime/debug-vm.d.ts:12:8 - error TS2307: Cannot find module '@glimmer/interfaces' or its corresponding type declarations.
12 } from '@glimmer/interfaces';
~~~~~~~~~~~~~~~~~~~~~
```
It is possible that the more correct fix is to update some setting
in the Ember tsconfig (`"moduleResolution": "bundler"`?) but this
sidesteps the issue for now.
* Use a single version of vite
* Don't reference tsconfig that doesn't exist
* lint:fix
* Allow any Vite version, because there are tools in @glimmer-workspace that are out of date / chose the wrong peer range sigil
* lintfix
---------
Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com>
It seems like _something_ should have yelled at us for doing this?
…e difference between a `(mut)` reference (previously – "Invokable" reference, admittedly a very confusing name) and an "accessor" reference.
This was a mistake. The Ember usage that necessitated this feature requires that they are distinct. Specifically `{{action (mut this.someProp)}}` is explicitly a distinctly different usage than `{{action this.someProp}}`.
NullVoxPopuli
force-pushed
the
squashed-feature-handle-error
branch
from
b59b9c3
to
e193d9e
Compare
NullVoxPopuli
force-pushed
the
squashed-feature-handle-error
branch
from
34f2c45
to
4274dbd
Compare
NullVoxPopuli
force-pushed
the
squashed-feature-handle-error
branch
from
4274dbd
to
97ebcd1
Compare
Closed
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.
Supersedes: #1499
Makes #1462 mergable.
Changes
Result<T>This is a breaking change for the VM
however, the whole VM is private API to Ember, and we can easily absorb the changes.
Other projects, Glimmer, GlimmerX...
Are not formally supported, and would have similar work as Ember to integrate these changes.
old / solved problems
Current Problem:
mainwhen built.@glimmer/local-debug-flagshas a lot more in it, and is utilized morepassesis set to 2 in this demo:rebase process
Tasks when trying this again: