What should happen if static analysis can't determine correctness?

[redbeardcreator]

In other words, if you can only tell at runtime if a usage is correct, should the test fail, warn, or pass?

For instance, TestSessionRegenFalse checks if the function session_regenerate_id() is used. It uses Node::isFunction() to determine this. The call to isFunction() returns true if a function is called via a variable. Therefore, the test fails if a variable function is used and the other criteria are not met. In other words, $func(), $func(false) and $func(1) all fail the test.

In the same test, if the parameter to session_regenerate_id() is not the literal value true, the test fails. Even if the variable will always be true.

In the first case, I'd argue that the best behavior would be to pass the test; session_regenerate_id() will not likely be called via a variable. It could also be argued that this should issue a warning since it is possible that the variable will point to session_regenerate_id(). I don't believe it should always fail as it does now. It seems to me that false positives would be far too common.

In the second case, the behavior for a variable will almost always be incorrect, or at least potentially incorrect. Even if the parameter is a variable, it should always be passed true and only true. A variable will be wrong more often that right.

[enygma]

Well, that's one of the major downfalls of static scanners. They can only test the code statically and at rest, not as it is executed. I could see how if it was something like $func = 'session_generate_id'; $func(); this kind of checking could be useful, but I think it's an edge case that may not be worth it.

Maybe the isFunction check needs to be a bit smarter to evaluate if the function call is variable or not...

[redbeardcreator]

I'm just wondering if there might be a way to mitigate the weakness somewhat with separate "this is definitely bad" and "this is potentially bad" buckets.

As for isFunction(), I think it would be pretty easy to add that check. Right now, I think it just bails with true if it's a variable function.