Фондацията .NET е независима организация, създадена от Microsoft, за да поддържа проекти с отворен код около платформата DotNet. В момента организацията събра под крилото си много библиотеки. Вече сме тествали някои от тези библиотеки с помощта на PVS-Studio. Следващият проект за проверка с анализатора — LINQ to DB.
Въведение
LINQ to DB е рамка за достъп до база данни, базирана на LINQ. LINQ to DB събра най-доброто от своите предшественици. Тя ви позволява да работите с различни СУБД, докато LINQ to SQL навремето ви позволяваше да работите само с MS SQL. Не е толкова тежък и сложен като LINQ към SQL или Entity Framework. LINQ to DB предоставя повече контрол и бърз достъп до данни. Рамката не е толкова голяма: написана е на C# и съдържа повече от 40 000 реда код.
LINQ to DB също е един от проектите на .NET Foundation. Преди това сме проверили проектите на тази организация: Windows Forms, Xamarin.Forms, Teleric UI for UWP и др.
Малко по-малко разговори, малко повече действие! Нека проверим кода на LINQ към DB, взет от официалното хранилище на GitHub. С помощта на нашия статичен анализатор PVS-Studio ще видим дали всичко е наред с наследника на LINQ.
Дежа вю
Позволете ми да започна, вероятно, с най-често срещаните случаи, с които всеки разработчик се сблъсква поне веднъж: дублиран код.
V3001 Има идентични подизрази ‘genericDefinition == typeof(Tuple‹,,,,,,,›)’ отляво и отдясно на оператора ‘||’. TypeExtensions.cs 230
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;
}
....
}Първото съобщение на анализатора привлече вниманието ми. Тези, които рядко използват кортежи, може да си помислят, че това е често срещано следствие от копиране и поставяне. Без колебание можем да предположим, че разработчик е пропуснал запетая в последния ред на условието Tuple‹,,,,,,,›. Но дори функционалността на Visual Studio ми показа, че греша.
Кортежите в C# са разделени на 8 вида според броя на елементите. 7 от тях се различават само в различен брой елементи, съответно от 1 до 7. В този случай те съответстват на първите седем реда в условието. И последният, Tuple‹,,,,,,,›, включва 8 или повече елемента.
В резултат на това, когато се опитвате да напишете Tuple‹,,,,,,,,›, Visual Studio казва, че няма такъв кортеж. Оказва се, че в примера по-горе има допълнителна проверка за съответствие на променливата с типа Tuple‹,,,,,,,›, а не липсващата запетая, както изглеждаше първоначално.
Но следващото предупреждение на анализатора, което хвана окото ми, вече повдигна няколко въпроса.
V3003 Открито е използването на модела „ако (A) {…} else if (A) {…}“. Има вероятност от наличие на логическа грешка. Проверете редове: 256, 273. SqlPredicate.cs 256
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
{
....
}
....
}Според анализатора във фрагмента има два клона с еднакви условия. Ето защо второто условие винаги е невярно. Между другото, това също е косвено посочено от друго съобщение на анализатора: V3022 Изразът ‘Operator == Operator.NotEqual’ винаги е false. SqlPredicate.cs 273.
В примера виждаме повторение на условието Operator == Operator.NotEqual. Тези два клона на условие изпълняват малко по-различни операции. И така, въпросът е - от кой от клоновете наистина се нуждаят разработчиците? След малък анализ на функцията Reduce предполагам, че най-вероятно разработчиците се нуждаят точно от първия клон. Този, който има сравнение с Operator.NotEqual. Функционалността му е по-подобна на Equal и LessOrEqual. За разлика от своя близнак, вторият клон с NotEqual има абсолютно идентична функционалност с клона else. Ето „връзка“ към оригиналния файл за сравнение, обърнете внимание на 245–284 реда.
V3008 На променливата ‘newElement’ се присвояват стойности два пъти последователно. Може би това е грешка. Проверете редове: 1320, 1315. ConvertVisitor.cs 1320
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; } .... } .... }
В този кодов фрагмент авторът очевидно не е могъл да вземе решение за стила. Те не можаха да изберат единия и оставиха и двата варианта. Точно това откри анализаторът. Бих препоръчал да изберете един и да премахнете ненужното присвояване. Анализаторът издаде същото съобщение още веднъж:
V3008 На променливата ‘Stop’ се присвояват стойности два пъти последователно. Може би това е грешка. Проверете редове: 25, 24. TransformInfo.cs 25
public TransformInfo(Expression expression, bool stop, bool @continue)
{
Expression = expression;
Stop = false;
Stop = stop;
Continue = @continue;
}Сега е друга история. Тук променливата Stopпърво се присвоява със стойността false и веднага след това на следващия ред — със стойността stop на параметъра. Логично, в този случай е необходимо да се премахне първото присвояване, тъй като то не се използва и незабавно се презаписва от стойността на аргумента.
Къде отиде променливата?
V3010 Изисква се да се използва върнатата стойност на функцията „ToDictionary“. ReflectionExtensions.cs 34
public static MemberInfo[] GetPublicInstanceValueMembers(this Type type)
{
if (type.IsAnonymous())
{
type.GetConstructors().Single()
.GetParameters()
.Select((p, i) => new { p.Name, i })
.ToDictionary(_ => _.Name, _ => _.i);
}
....
}Какво беше намерението на разработчика с този фрагмент? Изглежда, че липсва променлива, на която трябва да присвоите резултата от изпълнението на този израз. В противен случай логиката на действие е неясна. По време на по-нататъшното изпълнение на функцията GetPublicInstanceValueMembers няма извикване на такъв израз. Намерението на разработчика е неизвестно. Може би този кодов фрагмент е в процес, така че трябва да изчакаме по-нататъшното му развитие.
V3025 Неправилен формат. При извикване на функцията „AppendFormat“ се очаква различен брой форматни елементи. Неизползвани аргументи: 1-ви. ExpressionTestGenerator.cs 663
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);
}В този фрагмент виждаме форматирането на низа. Въпросът е къде отиде първото извикване на аргумент? В първия форматиран ред разработчикът използва индекси от 1 до 9. Но или разработчикът не се нуждае от аргумент с индекс 0, или е забравил за него.
V3137 Променливата ‘version’ е присвоена, но не се използва до края на функцията. Query.cs 408
public void TryAdd(IDataContext dataContext, Query<T> 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;
}
}Тук сме в трудна ситуация. Според диагностичното съобщение се присвоява стойност на локалната променлива version, без изобщо да се използва тази стойност до края на функцията. Е, едно по едно нещо.
В самото начало стойността от _version се присвоява на променливата version. По време на изпълнение на кода стойността на версията не се променя. Извиква се само веднъж, за да се изчисли разликата с _version. И накрая, _version отново се присвоява на version. Наличието на оператори lock предполага, че по време на изпълнението на кодов фрагмент, извън блока с променливата _version, могат да настъпят промени паралелно извън функцията.
В този случай е логично да се предположи, че накрая е било необходимо версия да се размени с _version. Все пак изглежда странно да се присвои глобална стойност на локална променлива в края на функция. Анализаторът издаде подобно съобщение още веднъж: V3137 Променливата „leftcontext“ е присвоена, но не се използва до края на функцията. ExpressionBuilder.SqlBuilder.cs 1989
Една итерация на цикъл.
V3020 Безусловно „връщане“ в рамките на цикъл. QueryRunner.cs 751
static T ExecuteElement<T>( Query query, IDataContext dataContext, Mapper<T> 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<T>.Empty.First(); } }
Естествено е да използвате конструкцията while (reader.Read()), ако трябва да изберете няколко реда от база данни. Но тук в цикъла виждаме return без никакви условия, което означава, че е необходим само един ред. Тогава въпросът е - защо да използвате цикъл? В нашия случай няма нужда от цикъла while. Ако имате нужда само от първия елемент от базата данни, можете да използвате прост if.
Повтарящите се действия правят съвършенството
Все още са налице случаите с повторни проверки.
V3022 Изразът ‘версия › 15’ винаги е верен. SqlServerTools.cs 250
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); } } } .... }
Видяхте фрагмент от код. Забелязахте ли грешка? Анализаторът казва, че в този пример условието версия › 15винаги е вярно, поради което върнатият низ GetDataProvider(SqlServerVersion.v2008, доставчик) е недостижим код. Но нека разгледаме по-подробно функцията ProviderDetector.
Първо, предлагам да обърнете внимание на условието версия ‹= 8. Това означава, че допълнителен код не може да бъде изпълнен, ако версията на SQLServer е 8 или по-стара. Но ако погледнем надолу, ще видим клона case 8 в оператора switch. Този клон изпълнява идентичен код. Фрагментът е недостъпен код, тъй като 8-та версия вече не може да се използва поради условието по-горе. И тъй като все още изпълнява същия код, тогава можете безопасно да премахнете този клон от switch.
Второ, нека поговорим за съобщението на анализатора. Както вече казахме, всички версии, по-ранни или равни на 8-ма, няма да надхвърлят първото условие. Версии от 9-та до 15-та се улавят в клоновете switch. В този случай влизаме в клона по подразбиране, когато е изпълнено условието версия › 15. Това прави проверката на същото условие в клона default безсмислена.
Но остава въпросът: какво трябва да напишем в GetDataProvider — v2017 или v2008? Ако разгледаме останалите клонове на switch, можем да предположим следното: колкото по-стара е версията, годината на издаване на SQLServer също е по-висока. В този случай нека използваме SQLServerVersion.V2017. Правилната версия на този код трябва да изглежда така:
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); } } } .... }
Сега нека да разгледаме по-прост пример за задействане на диагностиката V3022 в този проект.
V3022 Изразът ‘table == null’ винаги е верен. LoadWithBuilder.cs 113
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) { .... } .... }
Какво имаме тук? Променливата table се сравнява с null два пъти. Първият път условието проверява променливата за неравенство с null. Когато условието е изпълнено, се осъществява изход от функция. Това означава, че кодът под разклонението на условието се изпълнява само когато table = null. Не се извършват действия върху променливата до следващата проверка. В резултат на това, когато кодът достигне условието table == null, тази проверка винаги връща true.
Диагностиката на V3022 издаде още няколко полезни предупреждения. Няма да ги разглеждаме всички в статията, но препоръчваме на авторите сами да проверят проекта и да видят всички предупреждения на анализатора на PVS-Studio.
V3063 Част от условен израз винаги е вярна, ако се оценява: field.Field.CreateFormat != null. BasicSqlBuilder.cs 1255
protected virtual void BuildCreateTableStatement(....)
{
....
if (field.Field.CreateFormat != null)
{
if (field.Field.CreateFormat != null && field.Identity.Length == 0)
{
....
}
}
....
}В кодовия фрагмент по-горе можете да видите, че field.Field.CreateFormat се проверява два пъти за null. Но в този случай втората проверка се извършва директно в клона на първата проверка. Тъй като първата проверка е успешна, така че когато проверената стойност не се е променила, не е необходимо стойността field.Field.CreateFormat да се сравнява с null за втори път .
нула като нещо, за което да умреш
V3022 Изразът „редове“ винаги не е нула. Операторът „?.“ е прекомерен. SQLiteSqlBuilder.cs 214
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; }
Според анализатора в реда на този кодов фрагмент проверката if (rows?.Count › 0)за null е ненужна, тъй като rows не може да бъде null в този момент. Нека да разберем защо. Резултатът от функцията BuildRows се присвоява на променливата rows. Ето кодовия фрагмент на функцията:
internal IReadOnlyList<ISqlExpression[]> BuildRows(EvaluationContext context) { if (Rows != null) return Rows; .... var rows = new List<ISqlExpression[]>(); if (ValueBuilders != null) { foreach (var record in source) { ....var row = new ISqlExpression[ValueBuilders!.Count]; var idx = 0; rows.Add(row);.... } } return rows; }
Тъй като BuildRows не може да върне null, тогава, според анализатора, проверката за null е излишна. Но ако BuildRows беше върнал null — какво се разбира под условие rows rows?.Count › 0 — тогава в момента на rows.Count == 0 проверка на условието, NullReferenceException щеше да бъде хвърлено. При такова състояние ще трябва да направите и нулева проверка, за да избегнете грешка. Дотогава текущият код изглежда подозрителен и проверката за null е излишна.
Стигнахме до съобщението, което ме накара да се замисля и да направя няколко проверки.
V3042 Възможно изключение за NullReference. Операторите „?.“ и „.“ се използват за достъп до членове на обекта „_update“ SqlUpdateStatement.cs 60
public override ISqlTableSource? GetTableSource(ISqlTableSource table)
{
....
if (table == _update?.Table)
return _update.Table;
....
}Малък фрагмент, условие и изход от функцията.
И така, анализаторът е открил, че update се осъществява по два начина — с нулевия условен оператор и без него. Може да мислите, че условието е изпълнено само ако _update не е равно на null и двете части на равенството са еднакви. Но. Голямо дебело НО.
В случай, когато table и _update са равни на null, тогава _update?.Table връща null. Това отговаря на условието. След това, когато се опитате да извикате _update.Table, ще получите NullReferenceException. Ако можем да върнем null, както ISqlTableSource? ни казва в декларацията на функцията, тогава трябва да напишем return _update?.Table, за да избегнем грешка .
Заключение
Проектът LINQ to DB е голям и сложен, което прави проверката му по-вълнуваща. Проектът има много голяма общност и имахме късмета да получим някои интересни предупреждения.
Ако искате да знаете дали вашата кодова база има подобни грешки, можете да опитате PVS-Studio на вашия проект.
