Reevaluation of rule RSPEC-2692

I am utterly flabbergasted by this rule. Again it is not a standard or a convention but an attempt to suggest a usage that isn’t reality.

The IndexOf() method for collections and other such indexed entities exist to determine the index. Contains() actually uses IndexOf() to verify an item exists in a colleciton, but IndexOf is specifically to determine location within the collection.
When one is dealing with a called method that is exporting an item, say like in Xml Serialization, or CodeDom generation, the order or sequence is essential. The individual method has no knowledge of the index whatsoever, so it must verify the current item to see if it is the First item in the collection.

Valid:

const int first = 0;
if (Colleciton.IndexOf(item) > first) {}

Valid:

int index = colleciton.IndexOf(item);
if (index > 0) {}
Valid:
if (Colleciton.OfType<ItemType>().FirstOrDefault() == Item) {}

Valid:

foreach(T item in collection) 
{
   DoSomething(item, colleciton.indexof(item));
}
DoSomething (item, index) { 
  if (index > 0) writeline();
  output(item);
}

Invalid:

Colleciton.IndexOf(item) > 0

All of those are synonymous save for the fact that the only one that is readable and clear on what the purpose is, is the one that SonarQube fails.

Every other option is atrocious code that only exists because of this ID10T error rule. If someone doesn’t know that C# is 0 based, they have no business developing in it. yes people make mistakes all the time, but having a rule that is some made up convention is not helpful.
There are many circumstances when operating in a unidirectional output stream in which knowledge of first or last come in to play, and since C# a 0 based system, when someone compares the index of an item in the list to 0, that is never a bug, that is an intentional check on whether or not the item is the first item in the list.

I have provided all of the “Valid” ways to circumnavigate this bug but still do the exact thing your bug is saying we shouldn’t do: compare the index to positive numbers. Without indexof i challenge you to provide me a way that is not a code smell for determining the “first” item in a collection, but I would instead suggest you re-evaluate - or better yet remove - this rule .

Thanks
Jaeden “Sifo Dyas” al’Raec Ruiner

Hello Jaeden,

Since you have a lot of valid arguments to why this rule doesn’t apply to your case, isn’t it an option for you to either: (a) define your own ruleset where this rule is ignored, or (b) resolve the “bug” with “resolve as false positive”?

Choosing either a or b will depend on how often you use the construct in your code.

Regards,

Joël

1 Like

That is just what I was about to say! :slight_smile:

And I would prefer option b: better safe than sorry, right?

1 Like

If I understand correctly, you are iterating on a collection, and from the inside of the loop you use IndexOf to check if the item is the first of the iteration.

This looks like a really bad pattern to me, because it turns what should be a O(n) operation into a O(n²) operation, since IndexOf is performing a linear search. There are other ways to write this code, for instance:

  • You can pass the index from the loop to the subfunction, or even better, a boolean that indicates if you are the first element in the loop.
  • You can process the first element outside of the loop, then do a loop from the second element to the end.

Your users and polar bears on melting ice will thank you for the increase efficiency :slight_smile:

3 Likes

My argument is two fold:
First and foremost, scanning rules that apply to security, safe coding practices, best coding practices, and overall adherence to industry conventions is the fundamental purpose behind code scanning software, like SonarQube. There are countless examples I can provide where SonarQube, or any other comparative system, just can’t understand what is happening.

public static bool IsEmpty<T>(this IEnumerable<T> collection) {
  return (collection == null) || !collection.Any();
}

That is an example of many extensions i have to speed up the process of certain evaluations that are rampant in C# development. The joy is, there is little to no performance impact because of the C# optimizer. (believe me, i’ve checked)

however, in a situation like this:

void DoSomething(List<string> list) 
{
  if (!list.IsEmpty()) 
{
   Console.Write($"{list.Count}: ");
    Console.Writeline(String.Join(", ", list));
 }
}

SQ has no idea that IsEmpty() is actually evaluating for null, so it flags the “list.Count” as possible NRE. There are two problems with this:
A) It cannot be null because of the nature of IsEmpty() so therefore SQ is wrong.
B) The error should be at the “if (list.IsEmpty())” line, not the internal reference at list.Count. Regardless of what IsEmpty does, if list is null at list.Count it was null at the IsEmpty call, so the identification of where the error resides is a bit disconcerting.

A similar example revolves around the null-dereferencer with regards to nullable types:

FileInfo fi = GetFile(name);
if ((fi?.Exists).GetValueOrDefault()) 
{
return fi.OpenRead();
}

This is a pattern that can be used to solve both possibilities of “fi” being null, and the file it is pointing to not existing. In the If block scope, “fi” can never be null in that situation, because if it is null, the null-dereferencer operation will return a nullable (bool?) and the GetValueOrDefault() on Nullable returns false as the default value for bool. The only time fi.OpenRead() will ever be reach is if “fi” is not null, and the file it points to exists.

Both of these situations are the reason behind having workarounds for a false positive. Either by using a pragma warning disable, or marking it as resolved in SQ itself, those options exists for these kinds of situations, and I would never want that rule removed. It has saved my ass more times than I can count, because in the tens of thousands of lines of code we write in our lifetimes, simple oversights like NRE’s can happen and that helps to ensure safe coding.

However, with that in mind, scanning software like SQ should not be used as a platform to decree personal opinions as “code smells” or “warnings”. Personally, in my “opinion” the code:

if (myBool == false)

Is an atrocious code smell. it should written:

if (!myBool)

But regardless, that should never be a RULE to provide a warning. At the remote best it could be listed as a “message” that can be resolved by the VS IDE, but not a minor/major/critical style rule that can block development. Because it’s an opinion.
Also rules should not be contradictory or illogically based. Like the Capitalization rule.

ABCdefg -> Valid Identifier
ABCDefg -> Invalid Identifier

3 Caps in a row is all the SQ engine permits. However, there is an edge case:

ProdDB -> Invalid because DB is common so use Db
ItemID -> Invalid because ID is common so use Id

These rules are in conflict with each other, and thus either pick one or the other, not both. Having both equates to a double standard which is another way of saying no standard at all. DB and ID are both under 3 characters and therefore should be allowed. These are opinion based rule guidelines. It is an arbitrary line in the sand that benefits companies or coding projects with simpler acronyms for their objects.
Sonar Source would very easily always abide by this rule:

SSAccount
SQRuleDef

But New York University wouldn’t:

NYUStudents
NYUFaculty

There is no confusion in the multi-caps in either of those two scenarios, but given the 3 capitals rule, NYU is forced to not use their acronym as it would look normally, while SonarSource is. That has the suggestion of impropriety in the form self-benefiting, opinion-based, rule guidelines.
If a capitalization rule should exist, I’m okay with that, but it must be a single standard. For instance, I adopted a stricter interpretation of the rule:
No two caps in a row ever, except for the long time standard of Interface naming convention:

public interface IHaveProperty -> okay
public class SSRuleDef -> not okay
public class JCrew -> not okay

Then it becomes a single standard instead of an arbitrary opinion drawn at 3 characters instead of 4 or 2.

This leads too the second point of the argument which is simple: many times Developers are not in charge of corporate practices. A company wanting to “certify” any code their employees develop will often times adopt such third party systems as their metric for insuring standardization of code. This is not a new thing nor a bad thing. However, it’s CEOs and auditors, and other such non-technically inclined people who enact such standardization procedures as company policy, and they take such things at face value. Thus by defining an “opinion” as a rule - claiming it to be a standard - along with SonarSource’s history as a recognized authority on such standards, those kinds of people take a lot to convince that SonarSource is wrong in any given circumstance.
The method IndexOf() universally provides the index of an item in a colleciton. What that value is compared to has no bearing upon the method, when or how it is used. I would go so far as to agree with the presence of the rule if there wasn’t an existing .Net standard for the Contains() method. A rule saying IndexOf() shouldn’t be compared to positive numbers because you are skipping the first “n” elements is the same as saying IndexOf() should only ever be used to determine if an item is present in the collection. Yet, we already have a method for that, called Contains() (which ironically enough performs an IndexOf() > -1 comparison). With the existence of Contains() any direct usage of IndexOf is automatically intentional regardless of the value it is being compared to. In fact, the rule should be that if the code says IndexOf(x) > -1, that should be a warning indicating they should be using Contains() instead of IndexOf(). Forcing extra lines of code to circumnavigate an opinion based rule like this ends up defeating the purpose of clean code. As well, forcing developers to have to convince their non-technically inclined bosses that the code is right and an exemption needs to be made is an undue burden on the developers under the guise of standardizing cleaner code.

In the end, my complaints about Rules in SonarQube all revolve around the simple delineation of opinion versus a logical standard. Arbitrary opinions are basically a small group defining their way is better, and that is not always the case. My ideas are not the best, nor are my company’s, or SonarSource’s. My times SonarQube has flagged an actual standard in my code and I was immediately convinced, adopting the standard without hesitation. Standards and conventions are put in place so that all of us developers can seamlessly interact with each other’s code. If we start attempting to enforce our personal opinions they cease being standards and become a dictatorial pissing contest.

Thanks
J"SD"A’RR

PS - I am aware that IndexOf is costly in larger collections, and I rarely use it in my own original code. However, in this circumstance I’m extending third party objects which I don’t have any control over. In such circumstances, I would have modified the objects to implement IEnumerable<> so that I wouldn’t have to OfType<>() every collection when I want to employ a lambda expression. Not to mention I would have employed far more Interface Patterns in the development of the original Library, but alas I didn’t write it, so I have to deal with it as it is. That being said, the situation I’m dealing with is for code generation, so users aren’t likely to ever be frustrated by possible performance issues. The collections in question are likely never to have more than a few dozen, or a hundred items max, so the loss from using IndexOf is negligible.

Hello Jaeden,

Thank you for your exhaustive reply. I can see where you are coming from. Convincing the non-technical people that we know what we are doing is a large part of our jobs (as it is a big part of theirs). Having a technical tool that both parties trust is an excellent way to avoid unneccesary discussions. Therefore, a tool that is so critical for a seemless process should not be opinionated.

The type of problems this rule finds are code smells. The nature of a code smell is that it indicates that something may be amiss and increases the chance of a bug to be introduced. The way these smells are detected are based on rules that surfaced by years of experience with code bases. And whether the smells is solved can either be obvious or based on gut feeling.
For example, a complex code structure may be more understandable and maintainable by the software developers in a company; than the same method broken up in several smaller methods. However, the general rule is to not write large complex methods.

I do not think that the IndexOf rule is opinionated as much as the number of consecutive capitals in a name, because there is no arbitrary value that determines whether the rule applies. You are either comparing to a positive numer, or not. There is no magic number for which the IndexOf result can be compared to. The comparison that is allowed comes from the definition of the IndexOf method.

Now, I’m not saying that this rule came to be in a similar way. Evidence that backs up rules like these would be nice. But I can imagine that there are projects that suffered from bugs that were introduced by an incorrect comparison of the IndexOf result.
You could say that C# developers should know they are programming in a 0-indexed programming language, but sometimes companies transfer to a new technology, or have developers coming from a different domain where languages are 1-indexed. Just as sometimes you don’t have much influence on who enact the standardization procedures, you also may not have influence on who makes up the development team.

I am glad that there is a tool that can identify such easily made errors. I also believe that discussing such matters with non-technical inclined people is a good way to share knowledge, show that you are knowledgeable and thus gain trust from those people. But I also realize that this may require time that no-one has. So perhaps it’s my idealistic view of the world :wink:

I’m very interested in the outcome of this topic.

Regards,

Joël

1 Like

Hello @JaedenRuiner and @deltajoel

Thanks for the talk. Unfortunately, this thread is quite hard to follow because it refers to too many things. It started from s2692 “IndexOf” checks should not be for positive numbers and ended talking about s2259 Null pointers should not be dereferenced and naming rules (s100 and s101)

This topic is about RSPEC-2692. I will ignore anything else, please open a new topic for feedback regarding any other rules. We need to keep this forum usable and useful by and for everyone. Let’s separate concerns, please.

I agree. The rule is not a Bug rule, but just a Code Smell rule (there’s a low possibility it is a bug, but a high possibility it will lead to some confusion). And regarding comparison, you can do that by using == or != (the rule complains about > which is too vague).

FirstOrDefault should work

The options you mentioned are valid just because of a limitation we have in our C# analyzer : we don’t follow the assignment values (as opposed to our Java analyzer, which does follow constant value propagation inside the method). In some (maybe distant) future, all your examples should behave the same.

You say that we should remove the rule. You did not answer @deltajoel :

This does not really answer:

You then give an example of how a Bug rule (NRE detection) is useful and acceptable for false positives because of how many true positives it catches.

There is a huge difference between Bug and Vulnerability rules and Code Smell rules. Bugs and vulnerabilities are not really debatable. Code smells are.

I don’t see any reason for not disabling this Code Smell rule from your quality profile. You hate the rule and it annoys you every time. Please disable it. You won’t see it anymore. You won’t miss it, it’s clear.

I’ll also talk internally if it makes sense to remove it from SonarWay or not - if your suggestion will get many votes, maybe we will.

1 Like