Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GEN-35: error-stack implement serde hooks #1558

Draft
wants to merge 55 commits into
base: main
Choose a base branch
from
Draft

Conversation

indietyp
Copy link
Member

@indietyp indietyp commented Nov 30, 2022

🌟 What is the purpose of this PR?

This implements the equivalent of Debug hooks, but for the serde output, we're unable to know if a type implements Serialize directly; therefore, we need to register them via hooks.

πŸ”— Related links

🚫 Blocked by

πŸ” What does this change?

Report gains 2 new functions:

impl Report<()> {
	pub fn install_serde_hook<T: serde::Serialize>();
	pub(crate) fn invoke_serde_hook<T>(closure: impl FnOnce(&Hooks) -> T) -> T;
}

(potentially, we could also provide a way to change how a type is serialized via: fn install_serde_custom_hook<T: serde::Serialize>(hook: impl Fn(&T) -> Box<dyn erased_serde::Serialize>) or something similar).

πŸ“œ Does this require a change to the docs?

Yes, we need to update the docs to reflect the new capabilities.

πŸƒ Next Steps

  1. The fallback format hooks currently violate the contract described in the documentation when used in serde hooks. The fmt::HookContext is currently constructed for every separate invocation. This should instead be persisted (like how we currently carry forward the serde::HookContet)
  2. Hooks are not invoked on Context objects because the current representation does not allow multiple values for a Context. Hooks can return more than a single serialized object. Further investigation πŸ•΅οΈ is needed.
  3. Default hooks are currently not invoked if std or hooks features are missing, behaviour of fmt hooks should be mirrored
  4. Documentation is largely missing
  5. (maybe, if possible): DynamicFn is overly restrictive regarding lifetime. Closures need to have the lifetime for<'a> Fn(&'a T &mut HookContext<T>) -> U + 'a. This is fine for functions but routinely fails for closures, as we cannot explicitly specify lifetimes on closures. Another possibility would be to relax further the lifetime to: for<'a: 'b, 'b> Fn(&'a T &mut HookContext<T>) -> U + 'b, but currently, this isn't possible because one cannot set lifetime bounds in HRTB, another approach might - if possible - be necessary.

πŸ“Ή Demo

[
{
"context": "context A",
"attachments": [
{
"file": "tests/common/snapshots.rs",
"line": 207,
"column": 10
},
"2",
"1"
],
"sources": [
{
"context": "context A",
"attachments": [
{
"file": "tests/common/snapshots.rs",
"line": 201,
"column": 10
},
"4",
"3"
],
"sources": [
{
"context": "root error",
"attachments": [
{
"file": "tests/common/mod.rs",
"line": 4,
"column": 5
},
{
"spans": [
{
"fields": [],
"level": "INFO",
"name": "func_b",
"target": "test_serialize::common::snapshots",
"module_path": "test_serialize::common::snapshots",
"file": "tests/common/snapshots.rs",
"line": 73,
"type": "span"
},
{
"fields": [],
"level": "INFO",
"name": "func_a",
"target": "test_serialize::common::snapshots",
"module_path": "test_serialize::common::snapshots",
"file": "tests/common/snapshots.rs",
"line": 78,
"type": "span"
}
]
},
"6"
],
"sources": []
}
]
},
{
"context": "context A",
"attachments": [
{
"file": "tests/common/snapshots.rs",
"line": 196,
"column": 10
},
"5",
"3"
],
"sources": [
{
"context": "context A",
"attachments": [
{
"file": "tests/common/snapshots.rs",
"line": 194,
"column": 10
},
"7"
],
"sources": [
{
"context": "context A",
"attachments": [
{
"file": "tests/common/snapshots.rs",
"line": 185,
"column": 34
},
"9",
"8"
],
"sources": [
{
"context": "root error",
"attachments": [
{
"file": "tests/common/mod.rs",
"line": 4,
"column": 5
},
{
"spans": [
{
"fields": [],
"level": "INFO",
"name": "func_b",
"target": "test_serialize::common::snapshots",
"module_path": "test_serialize::common::snapshots",
"file": "tests/common/snapshots.rs",
"line": 73,
"type": "span"
},
{
"fields": [],
"level": "INFO",
"name": "func_a",
"target": "test_serialize::common::snapshots",
"module_path": "test_serialize::common::snapshots",
"file": "tests/common/snapshots.rs",
"line": 78,
"type": "span"
}
]
}
],
"sources": []
}
]
},
{
"context": "context A",
"attachments": [
{
"file": "tests/common/snapshots.rs",
"line": 179,
"column": 10
},
"13",
"10",
"16",
"9",
"8"
],
"sources": [
{
"context": "root error",
"attachments": [
{
"file": "tests/common/mod.rs",
"line": 4,
"column": 5
},
{
"spans": [
{
"fields": [],
"level": "INFO",
"name": "func_b",
"target": "test_serialize::common::snapshots",
"module_path": "test_serialize::common::snapshots",
"file": "tests/common/snapshots.rs",
"line": 73,
"type": "span"
},
{
"fields": [],
"level": "INFO",
"name": "func_a",
"target": "test_serialize::common::snapshots",
"module_path": "test_serialize::common::snapshots",
"file": "tests/common/snapshots.rs",
"line": 78,
"type": "span"
}
]
}
],
"sources": []
}
]
},
{
"context": "context A",
"attachments": [
{
"file": "tests/common/snapshots.rs",
"line": 165,
"column": 10
},
"15",
"14",
"10",
"16",
"9",
"8"
],
"sources": [
{
"context": "root error",
"attachments": [
{
"file": "tests/common/mod.rs",
"line": 4,
"column": 5
},
{
"spans": [
{
"fields": [],
"level": "INFO",
"name": "func_b",
"target": "test_serialize::common::snapshots",
"module_path": "test_serialize::common::snapshots",
"file": "tests/common/snapshots.rs",
"line": 73,
"type": "span"
},
{
"fields": [],
"level": "INFO",
"name": "func_a",
"target": "test_serialize::common::snapshots",
"module_path": "test_serialize::common::snapshots",
"file": "tests/common/snapshots.rs",
"line": 78,
"type": "span"
}
]
}
],
"sources": []
}
]
},
{
"context": "context A",
"attachments": [
{
"file": "tests/common/snapshots.rs",
"line": 175,
"column": 10
},
"11",
"9",
"8"
],
"sources": [
{
"context": "root error",
"attachments": [
{
"file": "tests/common/mod.rs",
"line": 4,
"column": 5
},
{
"spans": [
{
"fields": [],
"level": "INFO",
"name": "func_b",
"target": "test_serialize::common::snapshots",
"module_path": "test_serialize::common::snapshots",
"file": "tests/common/snapshots.rs",
"line": 73,
"type": "span"
},
{
"fields": [],
"level": "INFO",
"name": "func_a",
"target": "test_serialize::common::snapshots",
"module_path": "test_serialize::common::snapshots",
"file": "tests/common/snapshots.rs",
"line": 78,
"type": "span"
}
]
}
],
"sources": []
}
]
},
{
"context": "context A",
"attachments": [
{
"file": "tests/common/snapshots.rs",
"line": 171,
"column": 10
},
"12",
"9",
"8"
],
"sources": [
{
"context": "context A",
"attachments": [
{
"file": "tests/common/snapshots.rs",
"line": 170,
"column": 10
}
],
"sources": [
{
"context": "root error",
"attachments": [
{
"file": "tests/common/mod.rs",
"line": 4,
"column": 5
},
{
"spans": [
{
"fields": [],
"level": "INFO",
"name": "func_b",
"target": "test_serialize::common::snapshots",
"module_path": "test_serialize::common::snapshots",
"file": "tests/common/snapshots.rs",
"line": 73,
"type": "span"
},
{
"fields": [],
"level": "INFO",
"name": "func_a",
"target": "test_serialize::common::snapshots",
"module_path": "test_serialize::common::snapshots",
"file": "tests/common/snapshots.rs",
"line": 78,
"type": "span"
}
]
}
],
"sources": []
}
]
}
]
}
]
}
]
}
]
}
]

πŸ’‘ Additional Information

DynamicFn is overly restrictive. This issue can be addressed using the nightly feature closure_lifetime_binder, which enables us to specify all lifetimes explicitly. It is more verbose than regular closures (and CLion hates it), but it solves the problem. I provided a test case (showcased below) for more context. This resolves our issue because taking Fn(&'a T): U, Rust infers (for regular closures) that, if references are taken from T, U might have a lifetime less than 'a, using HRTBs we're unable to create a trait that reflects this behavior as far as I know (this is because HRTBs cannot define lifetime bounds). The experimental feature lets us circumvent the issue by explicitly stating that U will have a lifetime of 'a. I have attached a screenshot of the error.

Report::install_custom_serde_hook(
for<'a, 'b> |value: &'a DoesNotImplementSerialize,
context: &'b mut HookContext<DoesNotImplementSerialize>|
-> ImplementSerialize<'a> {
ImplementSerialize {
a: &value.a,
b: &value.b,
}
},
);

image

@github-actions github-actions bot added the area/libs > error-stack Affects the `error-stack` crate (library) label Nov 30, 2022
@vercel
Copy link

vercel bot commented Dec 6, 2022

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Updated
hash πŸ”„ Building (Inspect) Jan 3, 2023 at 0:55AM (UTC)
hash-design-system πŸ”„ Building (Inspect) Jan 3, 2023 at 0:55AM (UTC)
hashdotdev πŸ”„ Building (Inspect) Jan 3, 2023 at 0:55AM (UTC)

(currently mutable borrow of the `HookContext` provides some challenges)
trunk-io bot pushed a commit that referenced this pull request Jan 31, 2023
## 🌟 What is the purpose of this PR?

#1693 separated `fmt::HookContext` into `hook::HookContext` and `fmt::HookContext`, so that it can be used in #1558. While working, the problem is that documentation doesn't render correctly because `hook::HookContext` is private in public. Therefore unreachable rustdoc is unable to navigate to the type, and the documentation is incomplete.

This PR moves to a macro-based approach to enable better documentation. Instead of using an unreachable public type, we have a macro that implements the necessary common functions. This is by far not ideal, but I found this to be the best way without compromising #1558 or future hooks.
# Conflicts:
#	libs/error-stack/Cargo.toml
#	libs/error-stack/Makefile.toml
#	libs/error-stack/src/fmt.rs
#	libs/error-stack/src/fmt/hook.rs
#	libs/error-stack/src/serde.rs
#	libs/error-stack/tests/common.rs
#	libs/error-stack/tests/snapshots/test_debug__full__alt.snap
#	libs/error-stack/tests/snapshots/test_debug__full__color_mode_color.snap
#	libs/error-stack/tests/snapshots/test_debug__full__hook.snap
#	libs/error-stack/tests/snapshots/test_debug__full__hook_decr.snap
#	libs/error-stack/tests/snapshots/test_debug__full__hook_incr.snap
#	libs/error-stack/tests/snapshots/test_debug__full__hook_multiple.snap
#	libs/error-stack/tests/snapshots/test_debug__full__hook_provider.snap
#	libs/error-stack/tests/snapshots/test_debug__full__multiline.snap
#	libs/error-stack/tests/snapshots/test_debug__full__multiline_context.snap
#	libs/error-stack/tests/snapshots/test_debug__full__norm.snap
#	libs/error-stack/tests/snapshots/test_debug__sources_nested@backtrace-hooks.snap
#	libs/error-stack/tests/snapshots/test_debug__sources_nested@backtrace-pretty-print-hooks.snap
#	libs/error-stack/tests/snapshots/test_debug__sources_nested@hooks.snap
#	libs/error-stack/tests/snapshots/test_debug__sources_nested@pretty-print-hooks.snap
#	libs/error-stack/tests/snapshots/test_debug__sources_nested@spantrace-backtrace-hooks.snap
#	libs/error-stack/tests/snapshots/test_debug__sources_nested@spantrace-backtrace-pretty-print-hooks.snap
#	libs/error-stack/tests/snapshots/test_debug__sources_nested@spantrace-hooks.snap
#	libs/error-stack/tests/snapshots/test_debug__sources_nested@spantrace-pretty-print-hooks.snap
#	libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate@backtrace-hooks.snap
#	libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate@backtrace-pretty-print-hooks.snap
#	libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate@hooks.snap
#	libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate@pretty-print-hooks.snap
#	libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate@spantrace-backtrace-hooks.snap
#	libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate@spantrace-backtrace-pretty-print-hooks.snap
#	libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate@spantrace-hooks.snap
#	libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate@spantrace-pretty-print-hooks.snap
#	libs/error-stack/tests/snapshots/test_serialize__attachment.snap
#	libs/error-stack/tests/snapshots/test_serialize__context.snap
#	libs/error-stack/tests/snapshots/test_serialize__full__attachment.snap
#	libs/error-stack/tests/snapshots/test_serialize__full__context.snap
#	libs/error-stack/tests/snapshots/test_serialize__full__hook_custom.snap
#	libs/error-stack/tests/snapshots/test_serialize__full__hook_custom_owned.snap
#	libs/error-stack/tests/snapshots/test_serialize__full__multiple_sources.snap
#	libs/error-stack/tests/snapshots/test_serialize__full__multiple_sources_at_root.snap
#	libs/error-stack/tests/snapshots/test_serialize__multiple_sources.snap
#	libs/error-stack/tests/snapshots/test_serialize__multiple_sources_at_root.snap
#	libs/error-stack/tests/snapshots/test_serialize__sources_nested.snap
#	libs/error-stack/tests/snapshots/test_serialize__sources_nested@backtrace-hooks.snap
#	libs/error-stack/tests/snapshots/test_serialize__sources_nested@hooks.snap
#	libs/error-stack/tests/snapshots/test_serialize__sources_nested@spantrace-backtrace-hooks.snap
#	libs/error-stack/tests/snapshots/test_serialize__sources_nested@spantrace-hooks.snap
#	libs/error-stack/tests/test_debug.rs
#	packages/libs/error-stack/tests/common.rs
#	packages/libs/error-stack/tests/common/mod.rs
#	packages/libs/error-stack/tests/snapshots/test_debug__full__complex.snap
#	packages/libs/error-stack/tests/snapshots/test_debug__full__hook_multiple.snap
#	packages/libs/error-stack/tests/snapshots/test_debug__full__linear.snap
#	packages/libs/error-stack/tests/snapshots/test_debug__full__linear_ext.snap
#	packages/libs/error-stack/tests/snapshots/test_debug__full__sources.snap
#	packages/libs/error-stack/tests/snapshots/test_debug__full__sources_transparent.snap
#	packages/libs/error-stack/tests/snapshots/test_debug__sources_nested.snap
#	packages/libs/error-stack/tests/snapshots/test_debug__sources_nested@pretty-print.snap
#	packages/libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate.snap
#	packages/libs/error-stack/tests/snapshots/test_debug__sources_nested_alternate@pretty-print.snap
@indietyp
Copy link
Member Author

Converting into draft, because everything is broken right now and the large change in formatting for 0.3 need to be applied to this PR

@nonparibus nonparibus changed the title error-stack implement serde hooks GEN-35: error-stack implement serde hooks Aug 8, 2023
@linear
Copy link

linear bot commented Aug 8, 2023

GEN-35

@TimDiekmann TimDiekmann removed the request for review from a team September 2, 2023 16:51
@nonparibus nonparibus added the priority/3 low Lower priority: nice-to-have label Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/libs > error-stack Affects the `error-stack` crate (library) priority/3 low Lower priority: nice-to-have
Development

Successfully merging this pull request may close these issues.

None yet

4 participants