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

removal of closures and functions that contain assert macros cause false positives #502

Open
0xalpharush opened this issue Jun 15, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@0xalpharush
Copy link

Necessist will warn that a test is passing with code remove for the following examples:

  • A closure that contains an assertion
fn test1() {
std::thread::.spawn(move || {
                       ...
                        assert!(
                          ...
                        );
}

  • A function call that contains an assertion
fn test2() {
   let x = ...
   let y = ...
    my_assertion_helper(x, y);
}
fn my_assertion_helper(x, y) { assert!(...); }

Both of these mutations do not indicate that a test statement is unnecessary, and I think they should be ignored since assert is ignored.

@smoelius
Copy link
Collaborator

For both of these cases, I think the right thing to do is walk the function/closure bodies.

Closures are a little tricky, though. For the case of spawning a thread (e.g., the closure body is always executed), I think walking the closure body is probably the right policy.

But if the closure is executed conditionally, the situation becomes more complicated. See, e.g., the example in the README: https://github.com/trailofbits/necessist#example

FWIW, I outlined a strategy for choosing what to remove/ignore in this PR: #474

I think what I wrote here is largely consistent with that PR, though closures require more thought.

@smoelius smoelius added the enhancement New feature or request label Jun 15, 2023
@smoelius

This comment was marked as resolved.

@smoelius

This comment was marked as resolved.

@smoelius
Copy link
Collaborator

smoelius commented Sep 1, 2024

As of version 0.7.0, Necessist walks functions declared within the same files as the tests that call them. So the second bullet should be mitigated in most circumstances.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants