fix: add warning for missing test_fuzz attributes #685
Conversation
|
|
|
The reasons for the failures are not your PR. I will fix them and then get back to you. |
smoelius
left a comment
smoelius
left a comment
There was a problem hiding this comment.
Thanks a lot for doing this.
If you rebase onto the new master, I think the errors will go away.
macro/src/lib.rs
Outdated
| }); | ||
|
|
||
| let (impl_items, modules) = map_impl_items(&generics, trait_path.as_ref(), &self_ty, &items); | ||
| let warning = if modules.is_empty() { |
There was a problem hiding this comment.
Checking modules.is_empty() is clever!
But rather than add code with a deprecated attribute, I think you can just eprintln! a warning. You might model after this:
test-fuzz/cargo-test-fuzz/src/lib.rs
Line 1217 in 914dd30
Would it be a huge imposition to add a test to verify that the warning is emitted?
There was a problem hiding this comment.
@HiteshMittal07 Did you use a deprecated attribute because you knew it would point to the location of the test_fuzz_impl attribute?
There was a problem hiding this comment.
@smoelius Actually, the intention was to emit a compiler-level warning, since the issue title was "test_fuzz_impl should warn when no test_fuzz attributes are found."
HiteshMittal07
force-pushed
the
improvement/test-fuzz-attribute-check
branch
from
e4fcc4c to
2b8e069
Compare
macro/src/lib.rs
Outdated
|
|
||
| let (impl_items, modules) = map_impl_items(&generics, trait_path.as_ref(), &self_ty, &items); | ||
| if modules.is_empty() { | ||
| eprintln!("warning: no `test_fuzz` attributes found in this `impl` block"); |
There was a problem hiding this comment.
| eprintln!("warning: no `test_fuzz` attributes found in this `impl` block"); | |
| eprintln!("Warning: No `test_fuzz` attributes found in `impl` block"); |
Capitalizing "Warning' and "No" is for consistency with existing warnings. Removing "this" is because, when I see it in the logs, it is unclear what "this" refers to: https://github.com/trailofbits/test-fuzz/actions/runs/21429362910/job/61705010100?pr=685#step:15:16
Ideally, the warning would include information about where the test_fuzz_impl attribute occurs in the source code, but that could be a separate, future change.
If you could, please apply this suggestion and then squash your commits down to one. Then I will merge.
There was a problem hiding this comment.
@smoelius I’ve applied the suggestion and squashed my commits into one. From my side, it’s ready to merge. Please let me know if there are any further suggestions.
| use testing::{CommandExt, fuzzable}; | ||
|
|
||
| #[test] | ||
| fn warning() { |
HiteshMittal07
force-pushed
the
improvement/test-fuzz-attribute-check
branch
from
151b004 to
8ae9f47
Compare
Replace deprecated-based warning with stderr warning Add test for missing test_fuzz warning
HiteshMittal07
force-pushed
the
improvement/test-fuzz-attribute-check
branch
from
8ae9f47 to
7237b68
Compare
3 participants
Summary
Adds a compile-time warning when no #[test_fuzz] attributes are found in an impl block.
Why?
Previously, missing test_fuzz attributes failed silently. This warning helps users notice the issue early without breaking compilation.
Related Issue
Fixes #676