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

Do not show tracepoints outside of root by default #4190

Closed
wants to merge 2 commits into from

Conversation

MDLC01
Copy link
Contributor

@MDLC01 MDLC01 commented May 19, 2024

This PR changes the default way tracebacks are displayed to exclude tracepoints that are within a package. This can be reverted to show full tracebacks using by passing --diagnostic-location source to the CLI.

The goal is to reduce the noise for the end user when compiling a Typst file via the CLI.

I'm not sure if "diagnostic location" is the right name for that. I'm not sure either how to properly test that.

@laurmaedje
Copy link
Member

Can you show an example of a before/after? Without that, it's hard to judge if/why this is desirable.

@MDLC01
Copy link
Contributor Author

MDLC01 commented May 22, 2024

Consider the following file.

#import "@preview/example:0.1.0": div
// `div` just divides its two arguments, which is not possible for strings.
#div("this", "errors")

Full error message with --diagnostic-location source (same as before the PR):

error: cannot divide string by string
  ┌─ @preview/example:0.1.0\util\math.typ:4:17
  │
4 │ #let div(x, y) = x / y
  │                  ^^^^^

help: error occurred in this call of function `div`
  ┌─ \\?\D:\typst\test.typ:3:1
  │
3 │ #div("this", "errors")
  │  ^^^^^^^^^^^^^^^^^^^^^

Full error message without --diagnostic-location source:

error: cannot divide string by string
  ┌─ \\?\D:\typst\test.typ:3:1
  │
3 │ #div("this", "errors")
  │  ^^^^^^^^^^^^^^^^^^^^^

@PgBiel
Copy link
Contributor

PgBiel commented May 25, 2024

I have a feeling this makes the error less intuitive, at least if this is the default behavior. I could see a new user being confused by this error (whereas it could be the package author's fault instead of the user's fault).

Maybe a way to indicate the tracepoint was suppressed would be good.

@MDLC01
Copy link
Contributor Author

MDLC01 commented May 26, 2024

My main idea was that package authors can panic to signal a problem to the user without having a huge stack trace. But you are right that an unintended error within a package might cause a weird error message. I think indicating that some tracepoints where removed, and can be shown with --diagnostic-location source, would indeed be better. I'll try to implement that.

@laurmaedje
Copy link
Member

I think the same reasoning can apply if the panic is in a utility file in your project. While the package boundary is common, it's not necessarily the clear-cut boundary.

@MDLC01
Copy link
Contributor Author

MDLC01 commented Jun 2, 2024

I don't have as much time as I would hope to continue working on this PR for now. I may return to it later.

@MDLC01 MDLC01 closed this Jun 2, 2024
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.

None yet

3 participants