{"id":381659,"date":"2024-06-29T03:47:52","date_gmt":"2024-06-29T03:47:52","guid":{"rendered":"http:\/\/savepearlharbor.com\/?p=381659"},"modified":"-0001-11-30T00:00:00","modified_gmt":"-0001-11-29T21:00:00","slug":"","status":"publish","type":"post","link":"https:\/\/savepearlharbor.com\/?p=381659","title":{"rendered":"PVS-Studio checks the code quality in the .NET Foundation projects: LINQ to DB<\/span>"},"content":{"rendered":"
<\/div>\n
\n
\n
\n
\n

The .NET Foundation is an independent organization, created by Microsoft, to support open-source projects around the DotNet platform. Currently, the organization gathered many libraries under its wing. We have already tested some of these libraries with the help of PVS-Studio. The next project to check with the analyzer — LINQ to DB. <\/p>\n

<\/figcaption><\/figure>\n

Introduction<\/h3>\n

LINQ to DB<\/a> is a database access framework based on LINQ. LINQ to DB has collected the best of its predecessors. It allows you to work with various DBMS, whereas LINQ to SQL back in the day allowed you to work only with MS SQL. It’s not as heavy and complicated as LINQ to SQL or Entity Framework. LINQ to DB provides more control and quick access to data. The framework is not that big: it’s written in C# and contains more than 40,000 lines of code.<\/p>\n

LINQ to DB is also one of the .NET Foundation projects. We have previously checked the projects of this organization: Windows Forms<\/a>, Xamarin.Forms<\/a>, Teleric UI for UWP<\/a>, etc.<\/p>\n

A little less conversation, a little more action! Let’s check the LINQ to DB code taken from the official repository on GitHub<\/a>. With the help of our PVS-Studio<\/a> static analyzer, we will see if everything is fine with the LINQ’s successor.<\/p>\n

Deja Vu<\/h3>\n

Let me start, probably, with the most common cases that every developer encountered at least once: duplicate code.<\/p>\n

V3001<\/a> There are identical sub-expressions ‘genericDefinition == typeof(Tuple<,,,,,,,>)’ to the left and to the right of the ‘||’ operator. TypeExtensions.cs 230<\/p>\n

public static bool IsTupleType(this Type type) {   ....   if (genericDefinition    == typeof(Tuple<>)         || genericDefinition == typeof(Tuple<;,>)         || genericDefinition == typeof(Tuple<,,>)         || genericDefinition == typeof(Tuple<,,,>)         || genericDefinition == typeof(Tuple<,,,,>)         || genericDefinition == typeof(Tuple<,,,,,>)         || genericDefinition == typeof(Tuple<,,,,,,>)         || genericDefinition == typeof(Tuple<,,,,,,,>)         || genericDefinition == typeof(Tuple<,,,,,,,>))   {     return true;   }   .... } <\/code><\/pre>\n
The first message of the analyzer caught my eye. Those who use tuples infrequently may think that this is a common consequence of copy-paste. Without hesitation, we can assume that a developer missed a comma in the last line of the Tuple<,,,,,,,><\/em> condition. However, even the Visual Studio’s functionality showed me I was wrong. <\/p>\n
Tuples in C# are divided into 8 types according to the number of elements. 7 of them differ only in a different number of elements, from 1 to 7, respectively. In this case, they correspond to the first seven lines in the condition. And the last one, Tuple<,,,,,,,><\/em>, includes 8 or more elements. <\/p>\n
As a result, when trying to write Tuple<,,,,,,,,><\/em>, Visual Studio tells that there is no such tuple. Turns out that in the example above, there is an extra check for the variable correspondence with the Tuple<,,,,,,,><\/em> type, and not the missing comma, as it seemed initially.<\/p>\n
But the next analyzer warning that caught my eye, has already raised a couple of questions.<\/p>\n
V3003<\/a> The use of ‘if (A) {…} else if (A) {…}’ pattern was detected. There is a probability of logical error presence. Check lines: 256, 273. SqlPredicate.cs 256<\/p>\n
public ISqlPredicate Reduce(EvaluationContext context) {   ....   if (Operator == Operator.Equal)   {     ....   }   else   if (Operator == Operator.NotEqual)   {     search.Conditions.Add(       new SqlCondition(false, predicate, true));     search.Conditions.Add(       new SqlCondition(false, new IsNull(Expr1, false), false));     search.Conditions.Add(       new SqlCondition(false, new IsNull(Expr2, true), true));     search.Conditions.Add(       new SqlCondition(false, new IsNull(Expr1, true), false));     search.Conditions.Add(       new SqlCondition(false, new IsNull(Expr2, false), false));   }   else   if (Operator == Operator.LessOrEqual ||        Operator == Operator.GreaterOrEqual)   {     ....   }   else if (Operator == Operator.NotEqual)   {     search.Conditions.Add(       new SqlCondition(false, predicate, true));     search.Conditions.Add(       new SqlCondition(false, new IsNull(Expr1, false), false));     search.Conditions.Add(       new SqlCondition(false, new IsNull(Expr2, false), false));   }   else   {     ....   }   .... } <\/code><\/pre>\n
According to the analyzer, there are two branches with the same conditions in the fragment. That’s why the second condition is always false. By the way, this is also indirectly indicated by another analyzer message: V3022<\/a> Expression ‘Operator == Operator.NotEqual’ is always false. SqlPredicate.cs 273.<\/p>\n
In the example, we see the repetition of the Operator == Operator.NotEqual<\/em> condition. These two condition branches perform slightly different operations. So, the question is — which of the branches do the developers really need? After a little analysis of the Reduce<\/em> function I assume that most likely the developers need exactly the first branch. The one that has comparison with Operator.NotEqual<\/em>. Its functionality is more similar to the Equal<\/em> and LessOrEqual<\/em>. Unlike its twin, the second branch with NotEqual<\/em> has absolutely identical functionality with the else<\/em> branch. Here is a link<\/a> to the original file for comparison, pay attention to 245-284 lines.<\/p>\n
V3008<\/a> The ‘newElement’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1320, 1315. ConvertVisitor.cs 1320<\/p>\n
internal IQueryElement? ConvertInternal(IQueryElement? element) {   ....   switch (element.ElementType)   {     ....     case QueryElementType.WithClause:     {       var with = (SqlWithClause)element;        var clauses = ConvertSafe(with.Clauses);        if (clauses != null && !ReferenceEquals(with.Clauses, clauses))       {         newElement = new SqlWithClause()         {           Clauses = clauses         };          newElement = new SqlWithClause() { Clauses = clauses };       }       break;     }     ....   }   .... } <\/code><\/pre>\n
In this code fragment, the author, apparently, could not decide on the style. They couldn’t choose the one and left both options. That’s exactly what the analyzer detected. I would recommend picking one and remove the unnecessary assignment. The analyzer issued the same message one more time:<\/p>\n
V3008<\/a> The ‘Stop’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 25, 24. TransformInfo.cs 25<\/p>\n
public TransformInfo(Expression expression, bool stop, bool @continue) {   Expression = expression;   Stop       = false;   Stop       = stop;   Continue   = @continue; } <\/code><\/pre>\n
Now it’s a different story. Here the *Stop *variable is first assigned with the *false *value and immediately after in the next line — with the stop<\/em> value of parameter. Logically, in this case it is necessary to remove the first assignment since it is not used and is instantly overwritten by the argument value.<\/p>\n
Where did the variable go?<\/h3>\n
V3010<\/a> The return value of function ‘ToDictionary’ is required to be utilized. ReflectionExtensions.cs 34<\/p>\n
public static MemberInfo[] GetPublicInstanceValueMembers(this Type type) {   if (type.IsAnonymous())   {     type.GetConstructors().Single()                                    .GetParameters()                                    .Select((p, i) => new { p.Name, i })                                    .ToDictionary(_ => _.Name, _ => _.i);   }   .... } <\/code><\/pre>\n
What was the developer’s intent with this fragment? It seems that there’s a variable missing, to which you need to assign the result of this expression execution. Otherwise, the logic of action is unclear. During further execution of the GetPublicInstanceValueMembers<\/em> function, there is no call of such expression. The developer’s intent is unknown. Maybe this code fragment is in progress, so we need to wait for its further development.<\/p>\n
V3025<\/a> Incorrect format. A different number of format items is expected while calling ‘AppendFormat’ function. Arguments not used: 1st. ExpressionTestGenerator.cs 663<\/p>\n
void BuildType(Type type, MappingSchema mappingSchema) {   ....   _typeBuilder.AppendFormat(     type.IsGenericType ? @\" {8} {6}{7}{1} {2}<{3}>{5}   {{{4}{9}   }} \" : @\" {8} {6}{7}{1} {2}{5}   {{{4}{9}   }} \",     MangleName(isUserName, type.Namespace, \"T\"),     type.IsInterface ? \"interface\"                       : type.IsClass ? \"class\"                                      : \"struct\",     name,     type.IsGenericType ? GetTypeNames(type.GetGenericArguments(), \",\")                         : null,     string.Join(\"\\r\\n\", ctors),     baseClasses.Length == 0 ? \"\"                              : \" : \" + GetTypeNames(baseClasses),     type.IsPublic ? \"public \"                    : \"\",     type.IsAbstract && !type.IsInterface ? \"abstract \"                                           : \"\",     attr,     members.Length > 0 ? (ctors.Count != 0 ? \"\\r\\n\" : \"\") +                           string.Join(\"\\r\\n\", members)                         : string.Empty); } <\/code><\/pre>\n
In this fragment we see the string formatting. The question is, where did the first argument call go? In the first formatted line, a developer used indexes from 1 to 9. But either the developer didn’t need an argument with the index 0, or they forgot about it.<\/p>\n
V3137<\/a> The ‘version’ variable is assigned but is not used by the end of the function. Query.cs 408<\/p>\n
public void TryAdd(IDataContext dataContext, Query query, QueryFlags flags) {   QueryCacheEntry[] cache;   int version;   lock (_syncCache)   {     cache   = _cache;     version = _version;   }   ....   lock(_syncCashe)   {     ....     var versionsDiff = _version - version;     ....     _cache   = newCache;     _indexes = newPriorities;     version  = _version;   }  } <\/code><\/pre>\n
We’re in a tricky situation here. According to the diagnostic message, a value is assigned to the local version<\/em> variable without ever using this value by end of the function. Well, one thing at a time. <\/p>\n
At the very beginning, the value from _version<\/em> is assigned to the version<\/em> variable. During code execution, the version<\/em> value does not change. It’s only called once to calculate the difference with _version<\/em>. And at the end, _version<\/em> is assigned to the version<\/em> again. The presence of lock<\/em> statements implies that during the execution of a code fragment, outside the block with the _version<\/em> variable, changes can occur in parallel from outside the function. <\/p>\n
In this case, it is logical to assume that at the end it was necessary to swap version<\/em> with _version<\/em>. Still, it seems strange to assign a global value to a local variable at the end of a function. The analyzer issued similar message one more time: V3137 The ‘leftcontext’ variable is assigned but is not used by the end of the function. ExpressionBuilder.SqlBuilder.cs 1989<\/p>\n
One loop iteration.<\/h3>\n
V3020<\/a> An unconditional ‘return’ within a loop. QueryRunner.cs 751<\/p>\n
static T ExecuteElement(   Query          query,   IDataContext   dataContext,   Mapper      mapper,   Expression     expression,   object?[]?     ps,   object?[]?     preambles) {   using (var runner = dataContext.GetQueryRunner(query, 0, expression, ps,     preambles))   {     using (var dr = runner.ExecuteReader())     {       while (dr.Read())       {         var value = mapper.Map(dataContext, runner, dr);         runner.RowsCount++;         return value;       }     }      return Array.Empty.First();   } } <\/code><\/pre>\n
It’s natural to use the while (reader.Read())<\/em> construct if you need to select the multiple rows from database. But here in the loop, we see the return<\/em> without any conditions, which means that only one row is needed. Then the question is — why use a loop? In our case, there is no need for the while<\/em> loop. If you need only the first element from the database, you can use a simple if<\/em>.<\/p>\n
Repeat actions make perfection<\/h3>\n
The cases with repeated checks are still present.<\/p>\n
V3022<\/a> Expression ‘version > 15’ is always true. SqlServerTools.cs 250<\/p>\n
internal static IDataProvider? ProviderDetector(IConnectionStringSettings css,   string connectionString) {   ....   if (int.TryParse(conn.ServerVersion.Split('.')[0], out var version))   {     if (version <= 8)       return GetDataProvider(SqlServerVersion.v2000, provider);      using (var cmd = conn.CreateCommand())     {       ....       switch (version)       {         case  8 : return GetDataProvider(SqlServerVersion.v2000, provider);         case  9 : return GetDataProvider(SqlServerVersion.v2005, provider);         case 10 : return GetDataProvider(SqlServerVersion.v2008, provider);         case 11 :         case 12 : return GetDataProvider(SqlServerVersion.v2012, provider);         case 13 : return GetDataProvider(SqlServerVersion.v2016, provider);         case 14 :         case 15 : return GetDataProvider(SqlServerVersion.v2017, provider);         default :           if (version > 15)             return GetDataProvider(SqlServerVersion.v2017, provider);           return GetDataProvider(SqlServerVersion.v2008, provider);       }     }   }   .... } <\/code><\/pre>\n
You saw a code fragment. Did you notice an error? The analyzer says that in this example, the *version > 15 *condition is always true, which is why the return GetDataProvider(SqlServerVersion.v2008, provider<\/em>) string is unreachable code. But let’s take a closer look at the ProviderDetector<\/em> function.<\/p>\n
Firstly, I suggest paying attention to the version <= 8<\/em> condition. It means that further code cannot be executed if the version of SQLServer is 8 or earlier. But if we look down, we see the case 8<\/em> branch in the switch<\/em> statement. This branch executes identical code. The fragment is an unreachable code, because the 8th version can no longer be used due to the condition above. And since it still executes the same code, then you can safely remove this branch from switch<\/em>.<\/p>\n
Secondly, let’s talk about the analyzer’s message. As we already said, all versions earlier than or equal to 8th will not go beyond the first condition. Versions from 9th to 15th are caught in the switch<\/em> branches. In this case, we get into the default<\/em> branch when the condition version > 15<\/em> is met. It makes the check of the same condition inside the default<\/em> branch meaningless. <\/p>\n
But the question remains: what do we need to write in GetDataProvider<\/em> — v2017<\/em> or v2008<\/em>? If we look at the rest of the switch<\/em> branches, we can assume the following: the older the version, the SQLServer’s release year is higher as well. In this case, let’s use SQLServerVersion.V2017<\/em>. The correct version of this code should look like this:<\/p>\n
internal static IDataProvider? ProviderDetector(IConnectionStringSettings css,   string connectionString) {   ....   if (int.TryParse(conn.ServerVersion.Split('.')[0], out var version))   {     if (version <= 8)       return GetDataProvider(SqlServerVersion.v2000, provider);      using (var cmd = conn.CreateCommand())     {       ....       switch (version)       {         case  9 : return GetDataProvider(SqlServerVersion.v2005, provider);         case 10 : return GetDataProvider(SqlServerVersion.v2008, provider);         case 11 :         case 12 : return GetDataProvider(SqlServerVersion.v2012, provider);         case 13 : return GetDataProvider(SqlServerVersion.v2016, provider);         case 14 :         case 15 : return GetDataProvider(SqlServerVersion.v2017, provider);         default : return GetDataProvider(SqlServerVersion.v2017, provider);       }     }   }   .... } <\/code><\/pre>\n
Now let’s take a look at a simpler example of the V3022<\/a> diagnostic’s triggering in this project.<\/p>\n
V3022<\/a> Expression ‘table == null’ is always true. LoadWithBuilder.cs 113<\/p>\n
TableBuilder.TableContext GetTableContext(IBuildContext ctx, Expression path,    out Expression? stopExpression) {   stopExpression = null;    var table = ctx as TableBuilder.TableContext;    if (table != null)     return table;    if (ctx is LoadWithContext lwCtx)     return lwCtx.TableContext;    if (table == null)   {     ....   }   .... } <\/code><\/pre>\n
What do we have here? The table<\/em> variable is compared to null<\/em> twice. The first time, the condition checks the variable for an inequality with null<\/em>. When the condition is met, the exit from a function takes place. This means that the code below the branch of the condition is executed only when table<\/em> =<\/em> null<\/em>. No actions are performed on the variable until the next check. As a result, when the code reaches the table<\/em> ==<\/em> null<\/em> condition, this check always returns true<\/em>.<\/p>\n
Diagnostics of V3022<\/a> issued a few more useful warnings. We will not review them all in the article, but we encourage authors to check the project themselves and see all the warnings of the PVS-Studio analyzer.<\/p>\n
V3063<\/a> A part of conditional expression is always true if it is evaluated: field.Field.CreateFormat != null. BasicSqlBuilder.cs 1255<\/p>\n
protected virtual void BuildCreateTableStatement(....) {   ....   if (field.Field.CreateFormat != null)   {     if (field.Field.CreateFormat != null && field.Identity.Length == 0)     {       ....     }   }   .... } <\/code><\/pre>\n
In the code snippet above, you can see that field.Field.CreateFormat<\/em> is checked twice for null<\/em>. But in this case, the second check is performed directly in the branch of the first check. Since the first check is a success, so when the checked value has not changed, it is not necessary to compare the field.Field.CreateFormat<\/em> value with null<\/em> for the second time.<\/p>\n
null as something to die for<\/h3>\n
V3022<\/a> Expression ‘rows’ is always not null. The operator ‘?.’ is excessive. SQLiteSqlBuilder.cs 214<\/p>\n
protected override void BuildSqlValuesTable(   SqlValuesTable valuesTable,   string alias,   out bool aliasBuilt) {   valuesTable = ConvertElement(valuesTable);   var rows = valuesTable.BuildRows(OptimizationContext.Context);    if (rows.Count == 0)   {     ....   }   else   {     ....      if (rows?.Count> 0)     {      ....     }      ....   }   aliasBuilt = false; } <\/code><\/pre>\n
According to the analyzer, in the line of this code fragment, the *if (rows?.Count > 0) *check for null<\/em> is unnecessary, since rows<\/em> cannot be null<\/em> at that moment. Let’s figure it out why. The result of the BuildRows<\/em> function is assigned to the rows<\/em> variable. Here is the code fragment of the function:<\/p>\n
internal IReadOnlyList BuildRows(EvaluationContext context) {   if (Rows != null)     return Rows;   ....   var rows = new List();   if (ValueBuilders != null)   {     foreach (var record in source)     {       ....        var row = new ISqlExpression[ValueBuilders!.Count];       var idx = 0;       rows.Add(row);        ....     }   }   return rows; } <\/code><\/pre>\n
Since BuildRows<\/em> cannot return null<\/em>, then, according to the analyzer, check for null<\/em> is redundant. But if BuildRows<\/em> had returned null<\/em> — what is meant by rows rows?.Count > 0<\/em> condition — then at the moment of the rows.Count == 0<\/em> condition check, the NullReferenceException<\/em> would have been thrown. In the of such a condition, you would also need to do a null<\/em> check to avoid an error. Until then, the current code looks suspicious and checking for null<\/em> is redundant.<\/p>\n
We got to the message, which made me think hard and do a couple of checks.<\/p>\n
V3042<\/a> Possible NullReferenceException. The ‘?.’ and ‘.’ operators are used for accessing members of the ‘_update’ object SqlUpdateStatement.cs 60<\/p>\n
public override ISqlTableSource? GetTableSource(ISqlTableSource table) {   ....   if (table == _update?.Table)     return _update.Table;   .... } <\/code><\/pre>\n
A small fragment, a condition and exit from the function.<\/p>\n
So, the analyzer has detected that update<\/em> is accessed in two ways — with the null-conditional operator and without it. You might think that the condition is met only if _update<\/em> doesn’t equal null<\/em> and both parts of the equality are the same. But. Big fat BUT.<\/p>\n
In the case when table<\/em> and _update<\/em> equal null<\/em>, then _update?.Table<\/em> returns null<\/em>. That meets the condition. Then when trying to call _update.Table<\/em> you will get NullReferenceException<\/em>. If we can return null<\/em>, as ISqlTableSource?<\/em> tells us in the function declaration, then we should write return _update?.Table<\/em> to avoid an error.<\/p>\n
Conclusion<\/h3>\n
The LINQ to DB project is large and complex, which makes it more exciting to check it. The project has a very large community, and we were lucky to get some interesting warnings. <\/p>\n
If you want to know whether your code base have similar errors, you can try PVS-Studio<\/a> on your project.<\/p>\n<\/div>\n<\/div>\n<\/div>\n
<\/div>\n

 \u0441\u0441\u044b\u043b\u043a\u0430 \u043d\u0430 \u043e\u0440\u0438\u0433\u0438\u043d\u0430\u043b \u0441\u0442\u0430\u0442\u044c\u0438  https:\/\/habr.com\/ru\/articles\/589773\/<\/a><\/p>\n","protected":false},"excerpt":{"rendered":"
<\/div>\n
\n
\n
\n
\n
The .NET Foundation is an independent organization, created by Microsoft, to support open-source projects around the DotNet platform. Currently, the organization gathered many libraries under its wing. We have already tested some of these libraries with the help of PVS-Studio. The next project to check with the analyzer — LINQ to DB. <\/p>\n
<\/figcaption><\/figure>\n
Introduction<\/h3>\n
LINQ to DB<\/a> is a database access framework based on LINQ. LINQ to DB has collected the best of its predecessors. It allows you to work with various DBMS, whereas LINQ to SQL back in the day allowed you to work only with MS SQL. It’s not as heavy and complicated as LINQ to SQL or Entity Framework. LINQ to DB provides more control and quick access to data. The framework is not that big: it’s written in C# and contains more than 40,000 lines of code.<\/p>\n
LINQ to DB is also one of the .NET Foundation projects. We have previously checked the projects of this organization: Windows Forms<\/a>, Xamarin.Forms<\/a>, Teleric UI for UWP<\/a>, etc.<\/p>\n
A little less conversation, a little more action! Let’s check the LINQ to DB code taken from the official repository on GitHub<\/a>. With the help of our PVS-Studio<\/a> static analyzer, we will see if everything is fine with the LINQ’s successor.<\/p>\n
Deja Vu<\/h3>\n
Let me start, probably, with the most common cases that every developer encountered at least once: duplicate code.<\/p>\n
V3001<\/a> There are identical sub-expressions ‘genericDefinition == typeof(Tuple<,,,,,,,>)’ to the left and to the right of the ‘||’ operator. TypeExtensions.cs 230<\/p>\n
public static bool IsTupleType(this Type type) {   ....   if (genericDefinition    == typeof(Tuple<>)         || genericDefinition == typeof(Tuple<;,>)         || genericDefinition == typeof(Tuple<,,>)         || genericDefinition == typeof(Tuple<,,,>)         || genericDefinition == typeof(Tuple<,,,,>)         || genericDefinition == typeof(Tuple<,,,,,>)         || genericDefinition == typeof(Tuple<,,,,,,>)         || genericDefinition == typeof(Tuple<,,,,,,,>)         || genericDefinition == typeof(Tuple<,,,,,,,>))   {     return true;   }   .... } <\/code><\/pre>\n
The first message of the analyzer caught my eye. Those who use tuples infrequently may think that this is a common consequence of copy-paste. Without hesitation, we can assume that a developer missed a comma in the last line of the Tuple<,,,,,,,><\/em> condition. However, even the Visual Studio’s functionality showed me I was wrong. <\/p>\n
Tuples in C# are divided into 8 types according to the number of elements. 7 of them differ only in a different number of elements, from 1 to 7, respectively. In this case, they correspond to the first seven lines in the condition. And the last one, Tuple<,,,,,,,><\/em>, includes 8 or more elements. <\/p>\n
As a result, when trying to write Tuple<,,,,,,,,><\/em>, Visual Studio tells that there is no such tuple. Turns out that in the example above, there is an extra check for the variable correspondence with the Tuple<,,,,,,,><\/em> type, and not the missing comma, as it seemed initially.<\/p>\n
But the next analyzer warning that caught my eye, has already raised a couple of questions.<\/p>\n
V3003<\/a> The use of ‘if (A) {…} else if (A) {…}’ pattern was detected. There is a probability of logical error presence. Check lines: 256, 273. SqlPredicate.cs 256<\/p>\n
public ISqlPredicate Reduce(EvaluationContext context) {   ....   if (Operator == Operator.Equal)   {     ....   }   else   if (Operator == Operator.NotEqual)   {     search.Conditions.Add(       new SqlCondition(false, predicate, true));     search.Conditions.Add(       new SqlCondition(false, new IsNull(Expr1, false), false));     search.Conditions.Add(       new SqlCondition(false, new IsNull(Expr2, true), true));     search.Conditions.Add(       new SqlCondition(false, new IsNull(Expr1, true), false));     search.Conditions.Add(       new SqlCondition(false, new IsNull(Expr2, false), false));   }   else   if (Operator == Operator.LessOrEqual ||        Operator == Operator.GreaterOrEqual)   {     ....   }   else if (Operator == Operator.NotEqual)   {     search.Conditions.Add(       new SqlCondition(false, predicate, true));     search.Conditions.Add(       new SqlCondition(false, new IsNull(Expr1, false), false));     search.Conditions.Add(       new SqlCondition(false, new IsNull(Expr2, false), false));   }   else   {     ....   }   .... } <\/code><\/pre>\n
According to the analyzer, there are two branches with the same conditions in the fragment. That’s why the second condition is always false. By the way, this is also indirectly indicated by another analyzer message: V3022<\/a> Expression ‘Operator == Operator.NotEqual’ is always false. SqlPredicate.cs 273.<\/p>\n
In the example, we see the repetition of the Operator == Operator.NotEqual<\/em> condition. These two condition branches perform slightly different operations. So, the question is — which of the branches do the developers really need? After a little analysis of the Reduce<\/em> function I assume that most likely the developers need exactly the first branch. The one that has comparison with Operator.NotEqual<\/em>. Its functionality is more similar to the Equal<\/em> and LessOrEqual<\/em>. Unlike its twin, the second branch with NotEqual<\/em> has absolutely identical functionality with the else<\/em> branch. Here is a link<\/a> to the original file for comparison, pay attention to 245-284 lines.<\/p>\n
V3008<\/a> The ‘newElement’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1320, 1315. ConvertVisitor.cs 1320<\/p>\n
internal IQueryElement? ConvertInternal(IQueryElement? element) {   ....   switch (element.ElementType)   {     ....     case QueryElementType.WithClause:     {       var with = (SqlWithClause)element;        var clauses = ConvertSafe(with.Clauses);        if (clauses != null && !ReferenceEquals(with.Clauses, clauses))       {         newElement = new SqlWithClause()         {           Clauses = clauses         };          newElement = new SqlWithClause() { Clauses = clauses };       }       break;     }     ....   }   .... } <\/code><\/pre>\n
In this code fragment, the author, apparently, could not decide on the style. They couldn’t choose the one and left both options. That’s exactly what the analyzer detected. I would recommend picking one and remove the unnecessary assignment. The analyzer issued the same message one more time:<\/p>\n
V3008<\/a> The ‘Stop’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 25, 24. TransformInfo.cs 25<\/p>\n
public TransformInfo(Expression expression, bool stop, bool @continue) {   Expression = expression;   Stop       = false;   Stop       = stop;   Continue   = @continue; } <\/code><\/pre>\n
Now it’s a different story. Here the *Stop *variable is first assigned with the *false *value and immediately after in the next line — with the stop<\/em> value of parameter. Logically, in this case it is necessary to remove the first assignment since it is not used and is instantly overwritten by the argument value.<\/p>\n
Where did the variable go?<\/h3>\n
V3010<\/a> The return value of function ‘ToDictionary’ is required to be utilized. ReflectionExtensions.cs 34<\/p>\n
public static MemberInfo[] GetPublicInstanceValueMembers(this Type type) {   if (type.IsAnonymous())   {     type.GetConstructors().Single()                                    .GetParameters()                                    .Select((p, i) => new { p.Name, i })                                    .ToDictionary(_ => _.Name, _ => _.i);   }   .... } <\/code><\/pre>\n
What was the developer’s intent with this fragment? It seems that there’s a variable missing, to which you need to assign the result of this expression execution. Otherwise, the logic of action is unclear. During further execution of the GetPublicInstanceValueMembers<\/em> function, there is no call of such expression. The developer’s intent is unknown. Maybe this code fragment is in progress, so we need to wait for its further development.<\/p>\n
V3025<\/a> Incorrect format. A different number of format items is expected while calling ‘AppendFormat’ function. Arguments not used: 1st. ExpressionTestGenerator.cs 663<\/p>\n
void BuildType(Type type, MappingSchema mappingSchema) {   ....   _typeBuilder.AppendFormat(     type.IsGenericType ? @\" {8} {6}{7}{1} {2}<{3}>{5}   {{{4}{9}   }} \" : @\" {8} {6}{7}{1} {2}{5}   {{{4}{9}   }} \",     MangleName(isUserName, type.Namespace, \"T\"),     type.IsInterface ? \"interface\"                       : type.IsClass ? \"class\"                                      : \"struct\",     name,     type.IsGenericType ? GetTypeNames(type.GetGenericArguments(), \",\")                         : null,     string.Join(\"\\r\\n\", ctors),     baseClasses.Length == 0 ? \"\"                              : \" : \" + GetTypeNames(baseClasses),     type.IsPublic ? \"public \"                    : \"\",     type.IsAbstract && !type.IsInterface ? \"abstract \"                                           : \"\",     attr,     members.Length > 0 ? (ctors.Count != 0 ? \"\\r\\n\" : \"\") +                           string.Join(\"\\r\\n\", members)                         : string.Empty); } <\/code><\/pre>\n
In this fragment we see the string formatting. The question is, where did the first argument call go? In the first formatted line, a developer used indexes from 1 to 9. But either the developer didn’t need an argument with the index 0, or they forgot about it.<\/p>\n
V3137<\/a> The ‘version’ variable is assigned but is not used by the end of the function. Query.cs 408<\/p>\n
public void TryAdd(IDataContext dataContext, Query query, QueryFlags flags) {   QueryCacheEntry[] cache;   int version;   lock (_syncCache)   {     cache   = _cache;     version = _version;   }   ....   lock(_syncCashe)   {     ....     var versionsDiff = _version - version;     ....     _cache   = newCache;     _indexes = newPriorities;     version  = _version;   }  } <\/code><\/pre>\n
We’re in a tricky situation here. According to the diagnostic message, a value is assigned to the local version<\/em> variable without ever using this value by end of the function. Well, one thing at a time. <\/p>\n
At the very beginning, the value from _version<\/em> is assigned to the version<\/em> variable. During code execution, the version<\/em> value does not change. It’s only called once to calculate the difference with _version<\/em>.<\/p>\n<\/div>\n<\/div>\n<\/div>\n<\/div>\n","protected":false},"author":1,"featured_media":0,"comment_status":"open","ping_status":"open","sticky":false,"template":"","format":"standard","meta":{"footnotes":""},"categories":[],"tags":[],"class_list":["post-381659","post","type-post","status-publish","format-standard","hentry"],"_links":{"self":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/posts\/381659","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/users\/1"}],"replies":[{"embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Fcomments&post=381659"}],"version-history":[{"count":0,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/posts\/381659\/revisions"}],"wp:attachment":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Fmedia&parent=381659"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Fcategories&post=381659"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Ftags&post=381659"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}