Reproduce S2583 FP: indexer does not throw exception #9585
Conversation
zsolt-kolbay-sonarsource
requested review from
Tim-Pohlmann and
sebastien-marichal
github-actions
bot
assigned Tim-Pohlmann and sebastien-marichal and unassigned zsolt-kolbay-sonarsource and Tim-Pohlmann
|
|
There was a problem hiding this comment.
LGTM!
I will let @Tim-Pohlmann to merge this as I might miss some SE knowledge.
| // https://github.com/SonarSource/sonar-dotnet/issues/9580 | ||
| public class Repro_9580 | ||
| { | ||
| public void IndexerReturnsNullInsteadOfThrowingException(NameValueCollection collection) | ||
| { | ||
| var element = collection["key"]; | ||
| if (element != null) | ||
| { | ||
| Console.WriteLine(element.ToString()); | ||
| } | ||
| if (collection.Count == 0) // Noncompliant - FP: the indexer with string argument returns null if the key is not found rather than throwing an exception, | ||
| { // so at this point we can't know for sure that the collection is not empty. | ||
| Console.WriteLine("Empty!"); // Secondary - FP | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is an FP indeed (I didn't knew this behavior exists, this is super inconsistent...).
However, I do think the problem in #9580 is the call to Refresh.
The underlying issue is, that we need to be more careful with our learnings if the collection in question is not of a type the Symbolic Execution engine is aware of.
I'd make two reproducers:
- The one you have here with
NameValueCollection. Add a second test case with a custom collection that overrides the indexer. - A reproducer with a custom collection that has a custom method to add items to the collection (like
Refresh). Please check before-hand if a UT for a case like this exists already. Especially take a look at the S4158 test cases, there might be something in there.
There was a problem hiding this comment.
Reproduces #9580