LGTM.com - false positive: py/procedure-return-value-used

[dgw]

I'm not sure how the analyzer is even finding the function whose return value it insists is being used, from a test suite file that's not referenced in any way from the flagged code file.

Obviously this code uses the return value of a function. In fact, its return value may be None. But there's no way to tell that it's always None, because the function called is dynamically determined.

https://lgtm.com/projects/g/sopel-irc/sopel/snapshot/b921e6fe0662d3f24cb468c0008465b037682294/files/sopel/plugins/rules.py?sort=name&dir=ASC&mode=heatmap#x5dd3ccc00f1e0e0:1

[tausbn]

So, just to clarify, are you saying that the function identified in the alert never actually appears in the call you link to (which would indicate that we have resolved the calls incorrectly somehow), or simply that it isn't the only function that is called there?

In the former case, this is definitely a bug we would need to address. In the latter case, the alert seems fair: it's saying you have a piece of code that expects to assign the return value of a call to a variable, but that for this particular function, the value that is implicitly returned by the called function is None.

In other words, the test function is violating the expected interface (that handler functions return an exit code).

Now, I don't know if None is a sensible exit code. If it is, perhaps the handler function in the test should explicitly return None. This would remove the alert.

Also, I should note that this alert only appears if all calls that appear at this position turn out to be procedures (to the extent that we are able to determine this). In this case, it would appear that our analysis thinks test_rule_execute.handler is the only function that ever gets called here. If this is true, then that suggests that the tests are incomplete -- that there are no tests that check the exit code functionality. (This may also be caused by incompleteness in our analysis. We may simply be failing to analyse correctly the tests that look at the exit code -- I have not investigated this.)

I hope that clears up the alert and the reason it appears! 🙂

In the longer term, we may decide to exclude results coming from tests entirely (as these may only implement the bare minimum to get the tests to pass, and thus be deficient in ways irrelevant to the thing that is actually being tested) but I can't give a definite date for when this would happen. In the meantime, if you're bothered by the alert, you can always suppress it.

[Exirel]

First, thank you for looking at this, I know it takes time and effort, and I really appreciate. Your really helped making things clear for me!

that there are no tests that check the exit code functionality

It's complicated. I'll try to give you some context on what the code does, and why we are surprised by this error. Before I do, just so you know, I fixed the warning by testing the returned value in that one particular test that raises the LGTM error, so technically, we are good. However, I believe you might want to know more, just to be extra sure.

So, what does this portion of code do?

The Rule class registers 2 things:

• when to do something (some condition for functional reason)
• what to do: you give it a function or a procedure, and it will execute it when the condition are met - this is the handler attribute

To test that, in each separated test, we have locally defined handlers, and each is implemented to fit the current test case, and nothing more. As you said yourself:

as these may only implement the bare minimum to get the tests to pass, and thus be deficient in ways irrelevant to the thing that is actually being tested

That's the catch I think: we happened to use locally defined functions, they are not reused anywhere else, so we don't test them, we just use them to test something else. That's tricky! It's probably an edge case. I'll let you decide if it's a bug or not, and I hope I gave you enough information to make an opinion.

On our side, I fixed the alarm thanks to your suggestion. Even better, I fixed it knowing why it was raised, so it feels quite safe, that's great! Again, thank you! ❤️