Skip to content
This repository was archived by the owner on Feb 11, 2026. It is now read-only.

derive: make error messages slightly more readable, add manually-driven test#133

Merged
tertsdiepraam merged 1 commit intouutils:mainfrom
BenWiederhake:dev-derive-panic-no-unwrap
Apr 14, 2025
Merged

derive: make error messages slightly more readable, add manually-driven test#133
tertsdiepraam merged 1 commit intouutils:mainfrom
BenWiederhake:dev-derive-panic-no-unwrap

Conversation

@BenWiederhake
Copy link
Copy Markdown
Contributor

@BenWiederhake BenWiederhake commented Apr 13, 2025

Closes a bit of #129.

Using a fully-fledged compile-error testsuite is a bit overkill, but we still want to make sure that the derive crate generates reasonable error messages. That's what this "example" is for. In the following, there are blocks of lines, one marked as POSITIVE and multiple lines marked as NEGATIVE. The committed version of this file should only contain POSITIVE. In order to run a test, comment out the POSITIVE line, and use a NEGATIVE line instead, and manually check whether you see a reasonable error message – ideally the error message indicated by the comment. One way to do this is:

$ cargo build --example test_compile_errors_manually

Example output with the type that I did in #129:

$ cargo build --example test_compile_errors_manually
   Compiling uutils-args v0.1.0 (/home/user/workspace/uutils-args)
error: proc-macro derive panicked
  --> examples/test_compile_errors_manually.rs:24:10
   |
24 | #[derive(Arguments)]
   |          ^^^^^^^^^
   |
   = help: message: expected '=' after '[' in flag pattern

It's not a particularly nice error message, but it's more readable than the current called `Option::unwrap()` on a `None` value.

@BenWiederhake BenWiederhake force-pushed the dev-derive-panic-no-unwrap branch from 9919060 to b2e78d3 Compare April 13, 2025 18:59
@BenWiederhake
Copy link
Copy Markdown
Contributor Author

Changes since last push: cargo fmt

Copy link
Copy Markdown
Collaborator

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a clear improvement, but maybe not enough to close the issue for two reasons.

The first is that I think a proper compile-test framework sounds pretty good for this crate, because it is so focused on derive-based functionality.

The second is that we would ideally use the spans from syn to generate error messages, so that the errors don't just get reported on the #[derive(Arguments)] but on the parts that are failing.

@BenWiederhake
Copy link
Copy Markdown
Contributor Author

Ah, well, then I'll add compile-fail-tests in the next PR :)

@tertsdiepraam tertsdiepraam merged commit fb6209a into uutils:main Apr 14, 2025
@tertsdiepraam
Copy link
Copy Markdown
Collaborator

I unlinked the issue, but this is of course merge-worthy!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants