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.

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

Fix S4143: FP with List<T>.Add method calls #2674

Closed
cg110 opened this issue Sep 27, 2019 · 3 comments · Fixed by #2836
Closed

Fix S4143: FP with List<T>.Add method calls #2674

cg110 opened this issue Sep 27, 2019 · 3 comments · Fixed by #2836
Assignees
Labels
Area: C# C# rules related issues. Area: VB.NET VB.NET rules related issues. Type: False Positive Rule IS triggered when it shouldn't be.
Milestone

Comments

@cg110
Copy link

cg110 commented Sep 27, 2019

Description

Using the SonarAnalyzer.CSharp nuget, S4143 flags issues for List (and probably any lists)

Repro steps

Using the code:

List<string> S4143 = new List<string>();
S4143.Add("SomeText");
S4143.Add("\n");
S4143.Add("SomeText");
S4143.Add("\n");

The last 2 Adds are showing with warnings for S4143, as they are Adds they're extending the list rather than overwriting any existing entries.

Expected behavior

The above code shouldn't generate a warning.

Actual behavior

S4143 generates warnings.

Known workarounds

Set the warning to info or suppress.

Related information

  • SonarC# Version: 7.16.0.8981
  • Visual Studio Version: 2017, 15.9.16
  • If running through the Scanner for MSBuild, its version
@andrei-epure-sonarsource andrei-epure-sonarsource added this to the Support milestone Sep 27, 2019
@Corniel
Copy link
Contributor

Corniel commented Oct 3, 2019

I think it is okay that this warning is shown. You could argue that this behaviour should not be applied on a List<string>, but I think that adding string literals to a list this way is a bad practice.

You could argue that the message should than be different, but it tells you (indirectly) why this is a bad practice: a list is intended for keeping track of multiple elements of T (string in this case), not for building dynamic text. (Then you should use StringBuilder.)

@cg110
Copy link
Author

cg110 commented Oct 8, 2019

I'd argue that a list is allowed to have duplicate entries in it, if I wanted unique then I'd be using a dictionary or hashset.

@christophe-zurn-sonarsource
Copy link
Contributor

Hello @cg110 , thanks for reporting this.
Considering the rule, it is indeed a false positive, as the elements added later are not overriding the previous ones.

@christophe-zurn-sonarsource christophe-zurn-sonarsource added Area: C# C# rules related issues. Area: Rules Type: False Positive Rule IS triggered when it shouldn't be. Area: VB.NET VB.NET rules related issues. labels Nov 6, 2019
@christophe-zurn-sonarsource christophe-zurn-sonarsource removed their assignment Nov 6, 2019
@christophe-zurn-sonarsource christophe-zurn-sonarsource changed the title S4143 feels like it generates false positives for List<T> S4143 False positives with List<T>.Add method calls Nov 7, 2019
andrei-epure-sonarsource pushed a commit that referenced this issue Nov 8, 2019
* Add S1128 FP repro for #2747

* Add S4143 FP repro for #2674

* Add S3900 FP repro for #2639
@christophe-zurn-sonarsource christophe-zurn-sonarsource removed this from the Support milestone Nov 15, 2019
@christophe-zurn-sonarsource christophe-zurn-sonarsource added this to the 8.1 milestone Nov 25, 2019
@christophe-zurn-sonarsource christophe-zurn-sonarsource changed the title S4143 False positives with List<T>.Add method calls Fix S4143: FP with List<T>.Add method calls Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C# C# rules related issues. Area: VB.NET VB.NET rules related issues. Type: False Positive Rule IS triggered when it shouldn't be.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants