Better errors for schema merging

[tormeh]

This is a follow-up to https://github.com/oxidecomputer/typify/pull/521 . I finally got time to look at it all. I could polish it further without feedback, but at this point all the boilerplate is mostly done and I'm starting to encounter design questions. So I thought it best to submit it for review so I don't fly blind.

[tormeh]

• What are the design questions you've encountered?

Mostly how much context to include. In particular, calls to try_merge_with_subschemas have a mutable SchemaObject. If you want to know what this value was when the function is called you have to clone it before the call, so that even if there is no error you're still doing clones for error handling purposes. I opted against this, but I imagine opinions differ on the right thing to do here.

• What would you think about a single Error type rather than several?

Absolutely a valid choice, although a bit annoying to change now 😅. I chose this approach because it provides the most structure, but it does require a bit of maintenance whenever something is changed... On the other side of this spectrum is stuff like Anyhow, where the error type is essentially a string. I think both have their own charm. A library should IMO never expose stringly errors like Anyhow to applications (because they can't be matched on), but these errors are never shown to the caller, so it's not that important, I guess. My intention with how I've done it is to provide almost a stack trace of what went wrong, along with the data that the different layers were called with. This should provide a great debugging experience.

As I recall, the goal here is to improve the the debugging story when understanding a merge failure. It looks like the only output that would change is the new log message in the "Subschemas with other stuff" case. Is that right?

That's what the code does, yes. There used to be an unwrap where all the errors surfaced by crashing the progenitor. Originally I wanted to print something there, but it seems to be gone now. Would it be interesting to print something somewhere else? Or potentially do something else with the error?

[ahl]

I appreciate you taking another run at this. I'm not convinced that this is going to be helpful enough in debugging a merge failure to warrant the ongoing burden of the cacophony of errors and variants.

What would you think about a single Error type rather than several?

Absolutely a valid choice, although a bit annoying to change now

Indeed, but it's going to be much more annoying for me to change later.

This should provide a great debugging experience.

Can you show that? Stepping back: could we start from a specific condition that's currently a challenge to debug--I expect there are many--and focus on what would make it easier to debug. If we want those conditions to be easier to debug and stay easier to debug, we'll want tests that preserve the behavior.