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.

Sign up for GitHub

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

Jump to bottom

Reproduce S2583 FP: indexer does not throw exception #9585

Merged
merged 2 commits into from
Aug 2, 2024
Merged

Reproduce S2583 FP: indexer does not throw exception #9585

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f63f8a4
Reproduce S2583 FP: indexer does not throw exception
zsolt-kolbay-sonarsource Jul 31, 2024
9847611
Fix typo in comment
zsolt-kolbay-sonarsource Jul 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions ...sts/SonarAnalyzer.Test/TestCases/SymbolicExecution/Roslyn/ConditionEvaluatesToConstant.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.IO;
using System.Linq;
using System.Timers;
using System.Collections.Specialized;

namespace Tests.Diagnostics
{
Expand Down Expand Up @@ -4310,3 +4311,20 @@ void Test(List<Repro9425> aList)
}
}
}

// 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
}
}
}
Comment on lines +4315 to +4330
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The one you have here with NameValueCollection. Add a second test case with a custom collection that overrides the indexer.
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#9592

Loading