Skip to content

refactor: provide more descriptive panic messages#968

Closed
rvolosatovs wants to merge 2 commits intobytecodealliance:mainfrom
rvolosatovs:refactor/panic
Closed

refactor: provide more descriptive panic messages#968
rvolosatovs wants to merge 2 commits intobytecodealliance:mainfrom
rvolosatovs:refactor/panic

Conversation

@rvolosatovs
Copy link
Member

@rvolosatovs rvolosatovs commented Mar 30, 2023

Refs #967

From:

'IndexMap: key not found'

To:

interface with index 7 of type with index 54 missing from imported instances

While the message is still pretty cryptic, at least the domain the panic originates from is somewhat clear

Note, that the second panic occurs when constructing metadata of a component importing wasi-http as http (e.g. in https://github.com/bytecodealliance/wit-bindgen test cases)

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
…ssage

Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@rvolosatovs rvolosatovs changed the title refactor: provide more descriptive panic message refactor: provide more descriptive panic messages Mar 30, 2023
@alexcrichton
Copy link
Member

Thanks! There's quite a lot of panicking statements throughout wit-component though and ergonomically I don't think it's worth going through all of them and improving the messages (not that this does that, of course, more that I don't think it necessarily makes sense to continue this trend). That being said panics are internal errors that users, theoretically, should never encounter. In that sense a user shouldn't ever have to decipher a panic message and instead they'd open an issue where developers would diagnose (perhaps with the more useful messages here).

Given all that I think the best thing to do would either be:

  • Fix the underlying bugs and not update the panic messages for now.
  • Change the panics here to errors and instead return an error if the conditions are violated.

Does that sound reasonable?

@peterhuene
Copy link
Member

If no one has yet looked at fixing #967, I should have some time soon to dive into it after I get a bunch of feature work on cargo component in.

@alexcrichton
Copy link
Member

I believe this was addressed in #974 so I'm going to close this. Thanks for the PR though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants