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

[0xalpharush]

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]

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: https://github.com/trailofbits/necessist/pull/474

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

[smoelius]

Regarding the second bullet, I am jotting down some notes about how it could work. These ideas are by no means set in stone.

• A type LocalFunction is added to this trait: https://github.com/trailofbits/necessist/blob/b88071cc659a93721b86c0330eb94661d39cdb93/frameworks/src/parsing.rs#L58
• A method call_of_local_function is added to this trait: https://github.com/trailofbits/necessist/blob/b88071cc659a93721b86c0330eb94661d39cdb93/frameworks/src/parsing.rs#L74 The function returns an Option<<Self::Types as AbstractTypes>::LocalFunction<'ast>>.
• A method visit_local_function is added to the same trait.

The above are used as follows:

• When the GenericVisitor encounters a call, it asks the backend whether the call is of a local function (i.e., a function defined in the same test file). If so, the GenericVisitor adds the local function to a queue.
• Periodically, the GenericVisitor pops a local function from the queue and asks the backend to visit it.

A crucial point is that logic that is common among the backends is in the GenericVisitor, and is not duplicated among the backends.

[smoelius]

More notes to myself...

#1128 added a test_span_maps field to the GenericVisitor: https://github.com/trailofbits/necessist/blob/a0bb2c1ef93178874cdc329e638fb030f2500f36/backends/src/generic_visitor.rs#L21

Intuitively, this field maps a test name to the statement and method call spans associated with the test.

When the second bullet is implemented, and Necessist walks local function bodies, this field will have to change, because a span in a local function body could be associated with more than one test.

Example to clarify the intuition:

#[test]
fn it_works() {
    ...
    utility_function();
    ...
}

#[test]
fn it_still_works() {
    ...
    utility_function();
    ...
}

fn utility_function() {
    ...
    // Spans relevant to both `it_works` and `it_still_works`
    ...
}

[smoelius]

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.