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

Error messages are dreadful #34

Closed
adetaylor opened this issue Oct 14, 2020 · 7 comments · Fixed by #374
Closed

Error messages are dreadful #34

adetaylor opened this issue Oct 14, 2020 · 7 comments · Fixed by #374
Labels
good first issue Good for newcomers

Comments

@adetaylor
Copy link
Collaborator

See #33 for an example of where we should make an effort here. I need to figure out the way cxx does this and follow that as best practice.

@adetaylor adetaylor added the good first issue Good for newcomers label Nov 3, 2020
@adetaylor
Copy link
Collaborator Author

#165 is another example of a dreadful error message.

@adetaylor
Copy link
Collaborator Author

#171 changed things so that we all parsing and conversion is now done by the code generator, and the procedural macro does basically nothing. We therefore need to focus on ensuring that the output from the code generator is much better in case of trouble.

@adetaylor
Copy link
Collaborator Author

Recent work (e.g. #219 but also previous work) has meant we now try to get past any unparseable items, and do codegen for all the rest.

We therefore need two strategies here:

  • For fatal errors, display a terrific error message in the codegen phase.
  • For non-fatal errors (e.g. skipped types or functions):
    • Display a terrific warning
    • Ideally also insert something into the code, so that if people use that function they get a compile error (as opposed to the function being entirely absent). What I'm thinking is that, for a skipped function or type, we generate a constant (with lots of helpful rustdoc), and possibly vice-versa if we need to display messages for skipped constants (not yet a thing).

Other considerations:

  • Ideally we want a Span equivalent in the C++ header. bindgen doesn't currently communicate that to us. We'd probably want to get this into some rustdoc generated by bindgen.

@adetaylor
Copy link
Collaborator Author

I'm going to mark this as fixed once #357 lands, because although the error reporting needs continued improvement, the heavy lifts are done here. As of #357 the state of play is:

  • In (virtually all) cases where we can't generate an API, we proceed to generate other APIs rather than bombing out.
  • We will display a message during the codegen phase.
  • Wherever we can identify the symbol name that we couldn't generate, we will insert a ZST into the output with docs attached indicating why we couldn't generate bindings. For anyone using rust-analyzer or similar this should be very helpful.
  • We still don't have any notion of a 'span' from bindgen, so we can't indicate where in the original C++ the problem arose.
  • Many of the actual messages aren't very good but can be improved piecemeal.

@adetaylor
Copy link
Collaborator Author

#357 has landed, but actually I want to do a little more first. #357 doesn't generate markers that particular methods are problematic.

@adetaylor
Copy link
Collaborator Author

And... there's another thing we need to here. The "errors" added by remove_ignored.rs aren't yet contextualized, and should be.

@adetaylor
Copy link
Collaborator Author

#383 is an alternative 'fix' for that last bit...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant