Русский
Русский
English
Статистика
Реклама

Pvs

C программист, испытай себя найди ошибку

01.02.2021 16:08:09 | Автор: admin

Анализатор PVS-Studio регулярно пополняется новыми диагностическими правилами. Что интересно, часто диагностики обнаруживают подозрительные фрагменты кода еще до окончания всех работ. Например, в процессе тестирования на open-source проектах. Одной из подобных интересных 'находок' и хотелось бы поделиться сегодня с вами.

Как говорилось выше, один из этапов тестирования диагностического правила - проверка его работы на реальной кодовой базе. Для этого используется набор отобранных open-source проектов, для которых проводится анализ. Очевидное преимущество такого подхода - возможность посмотреть, как ведёт себя диагностическое правило в "реальных условиях". Менее очевидное - иногда находится что-то настолько интересное, что про это и заметку не грех написать.

Итак, предлагаю посмотреть на код из проекта Bouncy Castle C# и найти в нём ошибку:

public static string ToString(object[] a){  StringBuilder sb = new StringBuilder('[');  if (a.Length > 0)  {    sb.Append(a[0]);    for (int index = 1; index < a.Length; ++index)    {      sb.Append(", ").Append(a[index]);    }  }  sb.Append(']');  return sb.ToString();}

Для тех, кто любит подглядывать, я добавил картинку, чтобы сохранить интригу.

Уверен, многие не смогли найти ошибку без открытия IDE или документации к классу StringBuilder. Ошибка допущена при вызове конструктора:

StringBuilder sb = new StringBuilder('[');

Собственно, об этом и предупреждает статический анализатор PVS-Studio: V3165 Character literal '[' is passed as an argument of the 'Int32' type whereas similar overload with the string parameter exists. Perhaps, a string literal should be used instead. Arrays.cs 193.

Программист хотел создать экземпляр типа StringBuilder, строка в котором будет начинаться с символа '['. Однако из-за опечатки будет получен объект ёмкостью под 91 элемент, не содержащий символов.

Это произошло из-за того, что вместо двойных кавычек использовались одинарные, что привело к вызову не той перегрузки конструктора:

....public StringBuilder(int capacity);public StringBuilder(string? value);....

При вызове конструктора символьный литерал '[' будет неявно приведен к соответствующему значению типа int (91 в Unicode). Это приведет к вызову конструктора с параметром типа int, задающим начальную вместимость. Программист же хотел вызвать конструктор, задающий начало строки.

Для исправления ошибки следует заменить символьный литерал на строковый (т.е. использовать "[", а не '['), что позволит вызвать правильную перегрузку конструктора.

Мы решили не ограничиваться этим паттерном и расширили случаи, которые рассматривает диагностика. В результате, кроме символьных литералов, сейчас рассматриваются и другие выражения типа char, а также, аналогично конструкторам, проверяются методы.

Диагностика, срабатывание которой было описано выше, была добавлена в релизе PVS-Studio 7.11. Вы сами можете загрузить последнюю версию анализатора и посмотреть на что способна не только диагностика V3165, но и другие диагностики для языков C, C++, C# и Java.

Кстати, идеи диагностик нам часто предлагают сами пользователи. Так получилось и в этот раз - спасибо пользователю Krypt с ресурса Habr. Если у вас также есть идеи диагностических правил - не стесняйтесь предлагать их!

P.S. В текущей кодовой базе проекта данная ошибка уже исправлена. Однако это не отменяет того факта, что какое-то время она существовала в коде, и что статический анализ позволяет выявлять подобные проблемы и исправлять их на самых ранних этапах.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Valery Komarov. C# Programmer, It's Time to Test Yourself and Find Error.

Подробнее..

Шпион под прикрытием проверяем исходный код ILSpy с помощью PVS-Studio

04.02.2021 18:19:53 | Автор: admin

В блоге компании PVS-Studio можно найти далеко не одну статью с результатами проверок исходного кода различных компиляторов. С другой стороны, немного обделённым вниманием PVS-Studio кажется другой класс программ - класс декомпиляторов. Дабы сделать мир более гармоничным, был проверен исходный C# код программы ILSpy, как раз относящейся к лагерю декомпиляторов. Давайте же посмотрим, что интересного смог найти PVS-Studio.

Введение

Наверное, каждому программисту хоть раз в жизни приходилось использовать декомпилятор. Цели у каждого из нас могли быть разными: узнать имплементацию какого-то метода, подтвердить или опровергнуть свои подозрения насчёт ошибки в используемой библиотеке, или просто под влиянием любопытства посмотреть близкий к исходному код интересующей нас программы. В мире .NET'а при упоминании слова декомпилятор обычно в голову приходит или dotPeek или ILSpy. Про .NET Reflector сейчас вспоминают меньше. Припоминаю, как, когда в первый раз узнал про подобный класс утилит и декомпилировал чужую библиотеку, в голове пробежала мысль о шпионаже. И такие мысли были не только у меня одного - не спроста же декомпилятор ILSpy получил своё название. Итак, в данной статье я описал интересные и подозрительные с моей точки зрения места, обнаруженные с помощью PVS-Studio в исходном коде проекта ILSpy. Захотелось посмотреть, так сказать, что скрывает наш шпион и, в случае необходимости, "прикрыть" его статическим анализатором.

Откровенно говоря, статья про ILSpy получилась несколько случайно. Среди пользователей нашего анализатора достаточно много студий, занимающихся игровой разработкой. Это одна из причин того, почему мы в компании стараемся сделать наш инструмент как можно более полезным и удобным для разработчиков игр, в том числе использующих движки Unity и Unreal Engine.

Я знаком с клиентами, которые используют PVS-Studio совместно с Unreal Engine, а вот про Unity разработчиков, практикующих использование нашего анализатора, слышу значительно реже. В связи с этим хочется популяризировать анализатор среди Unity сообщества. Одним из способов популяризации могла бы стать статья о проверке open-source игры, разработанной при помощи данного движка. Вот только здесь у меня возникла проблема - я не смог найти такую игру (может вы, читатели, сможете любезно предоставить идеи для таких проверок?). Обстоятельства при поиске игры с открытым исходным кодом складывались немного странно, и на одном сайте в списке самых популярных проектов для Unity оказался ILSpy (для меня остаётся загадкой, как и почему он попал в этот список). К слову, ILSpy входит в пул проектов, на которых мы регулярно тестируем наш C# анализатор внутри компании. Странно, что статьи про этот проект у нас до сих пор не было. Ну что ж, раз не удалось найти Unity проект для анализа, так давайте проверим попавшийся на глаза ILSpy.

Вот что написано в описание проекта на GitHub'е: ILSpy is the open-source .NET assembly browser and decompiler.

Я не нашёл информацию о том, что при разработке проекта используется какой-нибудь статический анализатор. Наверное, так даже интереснее - PVS-Studio будет первым. Перейдём непосредственно к результатам анализа.

Замена без замены

V3038 The '"'"' argument was passed to 'Replace' method several times. It is possible that other argument should be passed instead. ICSharpCode.Decompiler ReflectionDisassembler.cs 772

private static void WriteSimpleValue(ITextOutput output,                                     object value, string typeName){  switch (typeName)  {    case "string":      output.Write(  "'"                   + DisassemblerHelpers                      .EscapeString(value.ToString())                      .Replace("'", "\'")                   // <=                   + "'");      break;    case "type":    ....  }  ....}

Кажется, что автор хотел осуществить замену всех вхождений символа одинарной кавычки на строку, состоящую из двух символов: символа обратной косой черты и символа одинарной кавычки. Из-за невнимательности вышла осечка - символ "'" меняется на самого себя, что является бессмысленной операцией. Между присвоением строке значения "'" и "\'" разницы нет - в любом случае строка будет проинициализирована символом одинарной кавычки. Если мы хотим, чтобы в строку попало значение "\'", то backslash нужно экранировать или воспользоваться символом '@': "\\'" или @"\'". То есть вызов метода Replace следует переписать следующим образом:

Replace("'", @"\'")

Правда и только правда

Предупреждение 1

V3022 Expression 'negatedOp == BinaryOperatorType.Any' is always true. ICSharpCode.Decompiler CSharpUtil.cs

static Expression InvertConditionInternal(Expression condition){  var bOp = (BinaryOperatorExpression)condition;  if (   (bOp.Operator == BinaryOperatorType.ConditionalAnd)      || (bOp.Operator == BinaryOperatorType.ConditionalOr))  {    ....  }  else if (   (bOp.Operator == BinaryOperatorType.Equality)           || (bOp.Operator == BinaryOperatorType.InEquality)            || (bOp.Operator == BinaryOperatorType.GreaterThan)           || (bOp.Operator == BinaryOperatorType.GreaterThanOrEqual)           || (bOp.Operator == BinaryOperatorType.LessThan)            || (bOp.Operator == BinaryOperatorType.LessThanOrEqual))  {    ....  }  else  {    var negatedOp = NegateRelationalOperator(bOp.Operator);    if (negatedOp == BinaryOperatorType.Any)                  // <=      return new UnaryOperatorExpression(....);    bOp = (BinaryOperatorExpression)bOp.Clone();    bOp.Operator = negatedOp;    return bOp;  }}

Анализатор предупреждает, что значение переменной negatedOp всегда равно значению Any из перечисления BinaryOperatorType. Чтобы убедиться в этом, давайте посмотрим на код метода NegateRelationalOperator, из которого данная переменная и получает своё значение.

public static BinaryOperatorType NegateRelationalOperator(BinaryOperatorType op){  switch (op)  {    case BinaryOperatorType.GreaterThan:      return BinaryOperatorType.LessThanOrEqual;    case BinaryOperatorType.GreaterThanOrEqual:      return BinaryOperatorType.LessThan;    case BinaryOperatorType.Equality:      return BinaryOperatorType.InEquality;    case BinaryOperatorType.InEquality:      return BinaryOperatorType.Equality;    case BinaryOperatorType.LessThan:      return BinaryOperatorType.GreaterThanOrEqual;    case BinaryOperatorType.LessThanOrEqual:      return BinaryOperatorType.GreaterThan;    case BinaryOperatorType.ConditionalOr:      return BinaryOperatorType.ConditionalAnd;    case BinaryOperatorType.ConditionalAnd:      return BinaryOperatorType.ConditionalOr;  }  return BinaryOperatorType.Any;}

Если к моменту вызова метода NegateRelationalOperator переменная bOp.Operator имела значение, несоответствующее ни одной метке case, то из метода вернётся значение BinaryOperatorType.Any. Видно, что вызов метода NegateRelationalOperator происходит только в том случае, если условия в вышестоящем операторе if и операторе if else были вычислены как false. А если быть совсем внимательным, то становится заметно, что условия в операторах if и if else покрывают все метки case из метода NegateRelationalOperator. Следовательно, к моменту вызова метода NegateRelationalOperator переменная bOp.Operator не подходит ни под одну метку case, и этот метод в данном случае всегда вернёт значение BinaryOperatorType.Any. Вот и получается, что negatedOp == BinaryOperatorType.Any всегда оценивается как true, и на следующей строке происходит возврат значения из метода. Вдобавок получаем недостижимый код:

bOp = (BinaryOperatorExpression)bOp.Clone();bOp.Operator = negatedOp;return bOp;

К слову, анализатор любезно выдал предупреждение и на это: V3142 Unreachable code detected. It is possible that an error is present. ICSharpCode.Decompiler CSharpUtil.cs 81

Предупреждение 2

V3022 Expression 'pt != null' is always true. ICSharpCode.Decompiler FunctionPointerType.cs 168

public override IType VisitChildren(TypeVisitor visitor){  ....  IType[] pt = (r != ReturnType) ? new IType[ParameterTypes.Length] : null;  ....  if (pt == null)    return this;  else    return new FunctionPointerType(      module, CallingConvention, CustomCallingConventions,      r, ReturnIsRefReadOnly,      pt != null ? pt.ToImmutableArray() : ParameterTypes,    // <=      ParameterReferenceKinds);}

Здесь всё очевидно - ветка else выполняется только в том случае, если переменная pt не равна null. Зачем тогда нужно писать тернарный оператор с проверкой переменной pt на неравенство null, мне непонятно. Возможно, в прошлом не было ветвления if else и первого оператора return. Тогда подобная проверка имела бы смысл, ну а сейчас всё же стоит убрать лишний тернарный оператор:

public override IType VisitChildren(TypeVisitor visitor){  ....  IType[] pt = (r != ReturnType) ? new IType[ParameterTypes.Length] : null;  ....  if (pt == null)    return this;  else    return new FunctionPointerType(      module, CallingConvention, CustomCallingConventions,      r, ReturnIsRefReadOnly,      pt.ToImmutableArray(), ParameterReferenceKinds);}

Предупреждение 3

V3022 Expression 'settings.LoadInMemory' is always true. ICSharpCode.Decompiler CSharpDecompiler.cs 394

static PEFile LoadPEFile(string fileName, DecompilerSettings settings){  settings.LoadInMemory = true;  return new PEFile(    fileName,    new FileStream(fileName, FileMode.Open, FileAccess.Read),    streamOptions: settings.LoadInMemory ?                           // <=      PEStreamOptions.PrefetchEntireImage : PEStreamOptions.Default,    metadataOptions: settings.ApplyWindowsRuntimeProjections ?         MetadataReaderOptions.ApplyWindowsRuntimeProjections :        MetadataReaderOptions.None  );}

Аналогично предыдущему срабатыванию анализатора получаем совершенно ненужный тернарный оператор. Свойству settings.LoadInMemory, проверяемому в тернарном оператора, выше присваивается значение true, которое не меняется вплоть до самого тернарного оператора. Для полноты картины приведу код геттера и сеттера самого свойства:

public bool LoadInMemory {  get { return loadInMemory; }  set {      if (loadInMemory != value)      {        loadInMemory = value;        OnPropertyChanged();      }  }}

Думаю, переписанный метод без тернарного оператора приводить не стоит - тут всё достаточно бесхитростно.

Предупреждение 4

V3022 Expression 'ta' is always not null. The operator '??' is excessive. ICSharpCode.Decompiler ParameterizedType.cs 354

public IType VisitChildren(TypeVisitor visitor){  ....  if (ta == null)      return this;  else      return new ParameterizedType(g, ta ?? typeArguments);     // <=}

Тут уже натыкаемся на ненужный null coalescing оператор. При попадании в ветку else переменная ta всегда имеет значение неравное null. Как следствие, использование оператора ?? тут лишнее.

Всего было найдено 31 предупреждение с номером V3022.

Ты здесь лишний

Предупреждение 1

V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: End. ICSharpCode.Decompiler Interval.cs 269

public override string ToString(){  if (End == long.MinValue)  {    if (Start == long.MinValue)      return string.Format("[long.MinValue..long.MaxValue]", End); // <=    else      return string.Format("[{0}..long.MaxValue]", Start);  }  else if (Start == long.MinValue)  {    return string.Format("[long.MinValue..{0})", End);  }  else  {    return string.Format("[{0}..{1})", Start, End);  }}

При вызове самого первого метода string.Format строка форматирования не соответствует передаваемым в метод фактическим аргументам. Значение переменной End, передаваемое в качестве аргумента, не будет подставлено в строку форматирования, так как в ней отсутствует элемент форматирования {0}. Исходя из логики данного метода это всё же не ошибка, первый оператор return вернёт именно ту строку, которую и задумывали авторы кода. Это, разумеется, не отменяет того факта, что присутствует бесполезный вызов метода string.Format с неиспользуемым аргументом. Это хорошо бы исправить, дабы не вводить в заблуждения человека, который будет читать этот метод.

Предупреждение 2

V3025 Incorrect format. A different number of format items is expected while calling 'AppendFormat' function. Arguments not used: angle. ILSpy.BamlDecompiler XamlPathDeserializer.cs 177

public static string Deserialize(BinaryReader reader){  ....  var sb = new StringBuilder();  ....  sb.AppendFormat(CultureInfo.InvariantCulture,                  "A{0} {2:R} {2} {3} {4}",                  size, angle, largeArc ? '1' : '0',                  sweepDirection ? '1' : '0', pt1);  ....}

В данном случае за бортом оказалась переменная angle. Несмотря на то, что её передали в метод AppendFormat, из-за того, что в строке форматирования отсутствует элемент форматирования {1} и дважды использован {2}, это переменная остаётся неиспользованной. Скорее всего, авторы хотели написать строку формата следующим образом: "A{0} {1:R} {2} {3} {4}".

Двойные стандарты

Предупреждение 1

V3095 The 'roslynProject' object was used before it was verified against null. Check lines: 96, 97. ILSpy.AddIn OpenILSpyCommand.cs 96

protected Dictionary<string, detectedreference=""> GetReferences(....){  ....  var roslynProject =  owner.Workspace                            .CurrentSolution                            .GetProject(projectReference.ProjectId);  var project = FindProject(owner.DTE.Solution                                 .Projects.OfType<envdte.project>(),                            roslynProject.FilePath);              // <=  if (roslynProject != null && project != null)           // <=  ....}

Сначала мы обращаемся к свойству FilePath объекта roslynProject без какого-либо опасения, что в переменной roslynProject может быть записан null, а буквально строкой ниже мы выполняем проверку равенства этой переменной на null. Такой код выглядит небезопасно и чреват возникновением исключения типа NullReferenceException. Для исправления подобной ситуации стоит обращаться к свойству FilePath, используя null-условный оператор, а в методе FindProject предусмотреть возможность получения потенциального null в качестве последнего параметра.

Предупреждение 2

V3095 The 'listBox' object was used before it was verified against null. Check lines: 46, 52. ILSpy FlagsFilterControl.xaml.cs 46

public override void OnApplyTemplate(){  base.OnApplyTemplate();  listBox = Template.FindName("ListBox", this) as ListBox;  listBox.ItemsSource = FlagGroup.GetFlags(....);         // <=  var filter = Filter;  if (filter == null || filter.Mask == -1)  {    listBox?.SelectAll();                                 // <=  }}

Ситуация аналогична предыдущему примеру. Сначала обращаемся к свойству ItemsSource без какой-либо проверки, что в переменной listBox может быть записан null, а несколькими строками ниже уже видим использование null-условного оператора с переменной listBox. Причём переменная listBox между двумя этими обращениями к полю и методу не получала нового значения.

Всего анализатор нашёл 10 предупреждений с номером V3095. Привожу список данных предупреждений:

  • V3095 The 'pV' object was used before it was verified against null. Check lines: 761, 765. ICSharpCode.Decompiler TypeInference.cs 761

  • V3095 The 'pU' object was used before it was verified against null. Check lines: 882, 886. ICSharpCode.Decompiler TypeInference.cs 882

  • V3095 The 'finalStore' object was used before it was verified against null. Check lines: 261, 262. ICSharpCode.Decompiler TransformArrayInitializers.cs 261

  • V3095 The 'definitionDeclaringType' object was used before it was verified against null. Check lines: 93, 104. ICSharpCode.Decompiler SpecializedMember.cs 93

  • V3095 The 'TypeNamespace' object was used before it was verified against null. Check lines: 84, 88. ILSpy.BamlDecompiler XamlType.cs 84

  • V3095 The 'property.Getter' object was used before it was verified against null. Check lines: 1676, 1684. ICSharpCode.Decompiler CSharpDecompiler.cs 1676

  • V3095 The 'ev.AddAccessor' object was used before it was verified against null. Check lines: 1709, 1717. ICSharpCode.Decompiler CSharpDecompiler.cs 1709

  • V3095 The 'targetType' object was used before it was verified against null. Check lines: 1614, 1657. ICSharpCode.Decompiler CallBuilder.cs 1614

Кстати, если вы хотите проверить свой собственный проект с помощью PVS-Studio или перепроверить тот же ILSpy, чтобы более детально изучить срабатывания, то можете собственноручно попробовать анализатор. Перейдя на сайт PVS-Studio, вы сможете как скачать сам анализатор, так и получить триальную лицензию.

Всё идёт к одному

Предупреждение 1

V3139 Two or more case-branches perform the same actions. ILSpy Images.cs 251

protected override ImageSource GetBaseImage(MemberIcon icon){  ImageSource baseImage;  switch (icon)  {    case MemberIcon.Field:      baseImage = Images.Field;      break;    case MemberIcon.FieldReadOnly:      baseImage = Images.FieldReadOnly;      break;    case MemberIcon.Literal:      baseImage = Images.Literal;             // <=      break;    case MemberIcon.EnumValue:      baseImage = Images.Literal;             // <=      break;    case MemberIcon.Property:      baseImage = Images.Property;      break;    case MemberIcon.Indexer:      baseImage = Images.Indexer;      break;    case MemberIcon.Method:      baseImage = Images.Method;      break;    case MemberIcon.Constructor:      baseImage = Images.Constructor;      break;    case MemberIcon.VirtualMethod:      baseImage = Images.VirtualMethod;      break;    case MemberIcon.Operator:      baseImage = Images.Operator;      break;    case MemberIcon.ExtensionMethod:      baseImage = Images.ExtensionMethod;      break;    case MemberIcon.PInvokeMethod:      baseImage = Images.PInvokeMethod;      break;    case MemberIcon.Event:      baseImage = Images.Event;      break;    default:      throw new ArgumentOutOfRangeException(nameof(icon),                  $"MemberIcon.{icon} is not supported!");  }  return baseImage;}

На мой взгляд, это явная ошибка. В случае, если переменная icon равна MemberIcon.EnumValue, то переменная baseImage в ветке case должна получать значение Images.EnumValue. Это хороший пример ошибки, который легко замечает статический анализатор, и легко пропускает человек при беглом обзоре кода.

Предупреждение 2

V3139 Two or more case-branches perform the same actions. ICSharpCode.Decompiler CSharpConversions.cs 829

bool ImplicitConstantExpressionConversion(ResolveResult rr, IType toType){  ....  switch (toTypeCode)  {    case TypeCode.SByte:      return val >= SByte.MinValue && val <= SByte.MaxValue;    case TypeCode.Byte:      return val >= Byte.MinValue && val <= Byte.MaxValue;    case TypeCode.Int16:      return val >= Int16.MinValue && val <= Int16.MaxValue;    case TypeCode.UInt16:      return val >= UInt16.MinValue && val <= UInt16.MaxValue;    case TypeCode.UInt32:      return val >= 0;                 // <=    case TypeCode.UInt64:      return val >= 0;                 // <=  }  ....}

Утверждать, что анализатор нашёл здесь явную ошибку я не берусь, но предупреждение однозначно имеет смысл. Если метки case для значения TypeCode.UInt32 и TypeCode.UInt64 выполняют один и тот же набор действий, почему бы не написать код более компактно:

bool ImplicitConstantExpressionConversion(ResolveResult rr, IType toType){  switch (toTypeCode)  {      ....      case TypeCode.UInt32:      case TypeCode.UInt64:        return val >= 0;  }  ....}

Анализатор выдал ещё 2 предупреждения с номером V3139:

  • V3139 Two or more case-branches perform the same actions. ICSharpCode.Decompiler EscapeInvalidIdentifiers.cs 85

  • V3139 Two or more case-branches perform the same actions. ICSharpCode.Decompiler TransformExpressionTrees.cs 370

Безопасность превыше всего

V3083 Unsafe invocation of event, NullReferenceException is possible. Consider assigning event to a local variable before invoking it. ILSpy MainWindow.xaml.cs 787class ResXResourceWriter : IDisposable

void assemblyList_Assemblies_CollectionChanged(....){  ....  if (CurrentAssemblyListChanged != null)    CurrentAssemblyListChanged(this, e);      // <=}

Подобный способ вызова событий является достаточно распространённым, но то, что мы встречаем данный паттерн во многих проектах, не является оправданием к его применению. Разумеется, это не критичная ошибка, но, как и говорит текст предупреждения анализатора, - этот вызов не является безопасным, и не исключено возникновение исключения типа NullReferenceException. Если между проверкой CurrentAssemblyListChanged на null и вызовом самого события от него отписались все обработчики (например, в другом потоке исполнения), то произойдёт выброс исключения NullReferenceException. Лучше сразу писать безопасный код, например, следующим образом:

void assemblyList_Assemblies_CollectionChanged(....){  ....  CurrentAssemblyListChanged?.Invoke(this, e);}

PVS-Studio обнаружил ещё 8 подобных случаев, и все их можно исправить аналогично представленному выше способу.

Уверенная неуверенность

V3146 Possible null dereference. The 'FirstOrDefault' can return default null value. ILSpy.BamlDecompiler BamlResourceEntryNode.cs 76

bool LoadBaml(AvalonEditTextOutput output, CancellationToken cancellationToken){  var asm = this.Ancestors().OfType<assemblytreenode>()                            .FirstOrDefault().LoadedAssembly;       // <=  ....  return true;}

Из коллекции, возвращаемой методом OfType, посредством вызова метода FirstOrDefault хотят получить первый встретившийся элемент типа AssemblyTreeNode. Все мы знаем, что, если в коллекции не будет встречено такого элемента, который бы удовлетворял предикату поиска, или, если сама коллекция оказалось пустой, то метод FirstOrDefault вернёт значение по умолчанию - null в нашем случае. Дальнейшее обращение к свойству LoadedAssembly по нулевой ссылке приведёт к возникновению исключения типа NullReferenceException. Соответственно, чтобы избежать подобной ситуации следует использовать null-условный оператор:

bool LoadBaml(AvalonEditTextOutput output, CancellationToken cancellationToken){  var asm = this.Ancestors().OfType<assemblytreenode>()                            .FirstOrDefault()?.LoadedAssembly;     // <=  ....  return true;}

Можно предположить, что разработчик уверен в том, что в данном конкретном месте метод FirstOrDefault никогда не вернёт null. Если подобная ситуация имеет место быть, то тогда лучше воспользоваться методом First вместо FirstOrDefault, так как он подчёркивает нашу уверенность в том, что мы всегда достанем нужный элемент из коллекции. Тем более, если элемент не будет найден в коллекции, то мы получим исключение типа InvalidOperationException (а не NullReferenceException как в случае с использованием FirstOrDefault с последующим обращением к свойству по нулевой ссылке) с понятным сообщением: "Sequence contains no elements".

Небезопасное сканирование

V3105 The 'm' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. ILSpy MethodVirtualUsedByAnalyzer.cs 137

static bool ScanMethodBody(IMethod analyzedMethod,                            IMethod method, MethodBodyBlock methodBody){  ....  var mainModule = (MetadataModule)method.ParentModule;  ....  switch (member.Kind)  {    case HandleKind.MethodDefinition:    case HandleKind.MethodSpecification:    case HandleKind.MemberReference:      var m = (mainModule.ResolveEntity(member, genericContext) as IMember)              ?.MemberDefinition;      if (   m.MetadataToken == analyzedMethod.MetadataToken               // <=          && m.ParentModule.PEFile == analyzedMethod.ParentModule.PEFile)  // &lt;=      {        return true;      }      break;  }  ....}

При инициализации переменной m использовался null-условный оператор, следовательно, предполагается, что m потенциально может иметь значение null. Интересно, что сразу на следующей строке мы уже без использования null-условного оператора обращаемся к свойствам переменной m, что чревато возникновением исключения типа NullReferenceException. Как и в некоторых других примерах из данной статьи исправляем ситуацию с помощью использования null-условного оператора:

static bool ScanMethodBody(IMethod analyzedMethod,                            IMethod method, MethodBodyBlock methodBody){  ....  var mainModule = (MetadataModule)method.ParentModule;  ....  switch (member.Kind)  {    case HandleKind.MethodDefinition:    case HandleKind.MethodSpecification:    case HandleKind.MemberReference:      var m = (mainModule.ResolveEntity(member, genericContext) as IMember)              ?.MemberDefinition;      if (   m?.MetadataToken == analyzedMethod.MetadataToken          && m?.ParentModule.PEFile == analyzedMethod.ParentModule.PEFile)      {        return true;      }      break;  }  ....}

Старые знакомые

V3070 Uninitialized variable 'schema' is used when initializing the 'ResourceSchema' variable. ICSharpCode.Decompiler ResXResourceWriter.cs 63

class ResXResourceWriter : IDisposable{  ....  public static readonly string ResourceSchema = schema;  ....  static string schema = ....;  ....}

Изначально я не планировал выписывать это предупреждения. Дело в том, что абсолютно идентичная ошибка уже была найдена пять лет назад в результате проверки проекта Mono, но после небольшого обсуждения с коллегой мы пришли к выводу, что упомянуть её в статье всё-таки стоит. Как и описано в статье про проверку Mono, на момент инициализации статического поля ResourceSchema другим статическим полем schema, поле schema ещё само не инициализировано и имеет значение по умолчанию - null. Файл ResXResourceWriter.cs, в котором была найдена ошибка, был любезно позаимствован с сохранением авторских прав из проекта Mono. Файл был расширен некоторым уникальным для проекта ILSpy функционалом. Вот так баги из одно проекта расползаются по сети и кочуют из одного проекта в другой. Кстати, в первоисточнике ошибку до сих пор не поправили.

Заключение

Подытоживая разбор результатов анализа, можно сказать, что были найдены не только фрагменты исходного кода, которые просто желательно переписать в целях рефакторинга, но и реальные ошибки, где авторы кода явно ожидают другого результата (например, тот же вызов метода Replace с одинаковыми аргументами). Регулярное использование статического анализа позволяет быстрее находить и исправлять подобные подозрительные места. Всегда проще и дешевле править баг на стадии написания/тестирования кода, нежели чем в продакшене, когда о баге вам уже сообщает пользователь, а не статический анализатор. Спасибо за прочтение.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Ilya Gainulin. A Spy Undercover: PVS-Studio to Check ILSpy Source Code.

Подробнее..

Подводные камни в бассейне строк, или ещё один повод подумать перед интернированием экземпляров класса String в C

06.04.2021 16:12:32 | Автор: admin

Будучи разработчиками программного обеспечения, мы всегда хотим, чтобы написанное нами ПО работало быстро. Использование оптимального алгоритма, распараллеливание, применение различных техник оптимизации мы будем прибегать ко всем известным нам средствам, дабы улучшить производительность софта. К одной из таких техник оптимизации можно отнести и так называемое интернирование строк. Оно позволяет уменьшить объём потребляемой процессом памяти, а также значительно сокращает время, затрачиваемое на сравнение строк. Однако, как и везде в жизни, необходимо соблюдать меру не стоит использовать интернирование на каждом шагу. Далее в этой статье будет показано, как можно обжечься и создать своему приложению неочевидный bottleneck в виде метода String.Intern.

Для тех, кто мог забыть, напомню, что в C# строки являются ссылочным типом. Следовательно, сама строковая переменная это просто ссылка, которая лежит на стеке и хранит в себе адрес, указывающий на экземпляр класса String, расположенный на куче.

Есть несколько версий формулы для того, чтобы посчитать, сколько байт занимает строковый объект на куче: вариант от Джона Скита и вариант, показанный в статье Тимура Гуева. На картинке выше я использовал второй вариант. Даже если эта формула верна не на 100 %, мы всё равно можем приблизительно прикинуть размер строковых объектов. К примеру, для того чтобы занять 1 Гб оперативной памяти, достаточно будет, чтобы в памяти процесса было около 4,7 миллионов строк (каждая длиной в 100 символов). Если мы полагаем, что среди строк, с которыми работает наша программа, есть большое количество дубликатов, как раз и стоит воспользоваться встроенным во фреймворк функционалом интернирования. Давайте сейчас быстро вспомним, что вообще такое интернирование строк.

Интернирование строк

Идея интернирования строк состоит в том, чтобы хранить в памяти только один экземпляр типа String для идентичных строк. При старте нашего приложения виртуальная машина создаёт внутреннюю хэш-таблицу, которая называется таблицей интернирования (иногда можно встретить название String Pool). Эта таблица хранит ссылки на каждый уникальный строковый литерал, объявленный в программе. Кроме того, используя два метода, описанных ниже, мы сами можем получать и добавлять ссылки на строковые объекты в эту таблицу. Если наше приложение работает с большим количеством строк, среди которых часто встречаются одинаковые, то нет смысла каждый раз создавать новый экземпляр класса String. Вместо этого можно просто ссылаться на уже созданный на куче экземпляр типа String, получая ссылку на него путём обращения к таблице интернирования. Виртуальная машина сама интернирует все строковые литералы, встреченные в коде (более подробно про многие хитрости интернирования можно прочитать в этой статье). А нам для работы предоставляются два метода: String.Intern и String.IsInterned.

Первый на вход принимает строку и, если идентичная строка уже имеется в таблице интернирования, возвращает ссылку на уже имеющийся на куче объект типа String. Если же такой строки ещё нет в таблице, то ссылка на этот строковый объект добавляется в таблицу интернирования и затем возвращается из метода. Метод IsInterned также на вход принимает строку и возвращает ссылку из таблицы интернирования на уже имеющийся объект. Если такого объекта нет, то возвращается null (про неинтуитивно возвращаемое значение этого метода не упомянул только ленивый).

Используя интернирование, мы сокращаем количество новых строковых объектов путём работы с уже имеющимися через ссылки, полученные посредством метода Intern. Помимо того, что мы не создаём большого количества новых объектов и экономим память, мы ещё и увеличиваем производительность программы. Ведь если мы работаем с большим количеством строковых объектов, ссылки на которые быстро пропадают со стека, то это может провоцировать частую сборку мусора, что негативно скажется на общем быстродействии программы. Интернированные строки будут существовать до конца жизни процесса, даже если ссылок на эти объекты в программе уже нет. Это первый нюанс, к которому стоит отнестись с осторожностью, ведь изначальное стремление к использованию интернирования в целях уменьшения расхода памяти может привести к совершенно противоположному результату.

Интернирование строк может дать прирост производительности при сравнении этих самых строк. Взглянем на реализацию метода String.Equals:

public bool Equals(String value){  if (this == null)    throw new NullReferenceException();   if (value == null)    return false;   if (Object.ReferenceEquals(this, value))    return true;    if (this.Length != value.Length)    return false;   return EqualsHelper(this, value);}

До вызова метода EqualsHelper, где производится посимвольное сравнение строк, метод Object.ReferenceEquals выполняет проверку на равенство ссылок. Если строки интернированы, то уже на этапе проверки ссылок на равенство метод Object.ReferenceEquals вернёт значение true при условии равенства строк (без необходимости сравнения самих строк посимвольно). Конечно, если ссылки не равны, то в итоге произойдёт вызов метода EqualsHelper и последующее посимвольное сравнение. Ведь методу Equals неизвестно, что мы работаем с интернированными строками, и возвращаемое значение false методом ReferenceEquals уже свидетельствует о различии сравниваемых строк.

Если мы точно уверены, что в конкретном месте в программе поступающие на вход строки будут гарантированно интернированы, то их сравнение можно осуществлять через метод Object.ReferenceEquals. Однако я бы не рекомендовал этот подход. Так как всё-таки всегда есть вероятность, что код в будущем изменится или будет переиспользован в другой части программы и в него смогут попасть неинтернированные строки. В этой ситуации при сравнении двух одинаковых неинтернированных строк через метод ReferenceEquals мы посчитаем, что они неидентичны.

Интернирование строк для последующего их сравнения выглядит оправданным только в том случае, если вы планируете проводить сравнение интернированных строк довольно часто. Помните, что интернирование целого набора строк тоже занимает некоторое время. Поэтому не стоит выполнять его ради разового сравнения нескольких экземпляров строк.

После того, как мы немного освежили память в вопросе интернирования строк, давайте же перейдём непосредственно к описанию той проблемы, с которой я столкнулся.

Краткая история того, как всё начиналось

В корпоративном bug tracker'е уже какое-то время значилась задача, в которой требовалось провести исследование: как распараллеливание процесса анализа С++ кода может сократить продолжительность анализа. Хотелось, чтобы анализатор PVS-Studio параллельно работал на нескольких машинах при анализе одного проекта. В качестве программного обеспечения, позволяющего проводить подобное распараллеливание, был выбран IncrediBuild. IncrediBuild позволяет параллельно запускать на машинах, находящихся в одной сети, различные процессы. Например, такой процесс, как компиляция исходных файлов, можно распараллелить на разные машины, доступные в компании (или в облаке), и таким образом добиться существенного сокращения времени сборки проекта. Данное программное обеспечение является довольно популярным решением среди разработчиков игр.

Моя задача заключалась в том, чтобы подобрать проект для анализа, проанализировать его с помощью PVS-Studio локально на своей машине, а затем запустить анализ уже с использованием IncrediBuild, который будет заниматься распараллеливанием процессов анализатора на машинах, доступных в расположении компании. По результатам подвести некий итог целесообразности такого распараллеливания. Если результат окажется положительным, предлагать нашим клиентам такое решения для ускорения анализа.

Был выбран open source проект Unreal Tournament. Удалось успешно уговорить программистов посодействовать в решении задачи и установить IncrediBuild на их машины. Полученный объединённый кластер включал около 145 ядер.

Так как я анализировал проект Unreal Tournament с помощью применения системы мониторинга компиляции в PVS-Studio, то мой сценарий работы был следующим. Я запускал программу CLMonitor.exe в режиме монитора, выполнял полную сборку проекта Unreal Tournament в Visual Studio. Затем, после прохождения сборки, опять запускал CLMonitor.exe, но уже в режиме запуска анализа. В зависимости от указанного в настройках PVS-Studio значения для параметра ThreadCount, CLMonitor.exe параллельно одновременно запускает соответствующее количество дочерних процессов PVS-Studio.exe, которые и занимаются анализом каждого отдельного исходного С++ файла. Один дочерний процесс PVS-Studio.exe анализирует один исходный файл, а после завершения анализа передаёт полученные результаты обратно в CLMonitor.exe.

Всё просто: выставил параметр ThreadCount в настройках PVS-Studio, равный количеству доступных ядер (145), запустил анализ и сел в ожидании того, что сейчас увижу, как 145 процессов PVS-Studio.exe будут исполняться параллельно на удалённых машинах. У приложения IncrediBuild есть удобная система мониторинга распараллеливания Build Monitor, через которую можно наблюдать, как процессы запускаются на удалённых машинах. Что-то подобное я и наблюдал в процессе анализа:

Казалось, что нет ничего проще: сиди и смотри, как проходит анализ, а после просто зафиксируй время его проведения с использованием IncrediBuild и без. Реальность оказалась несколько интересней...

Обнаружение проблемы, её локализация и устранение

Пока проходил анализ, можно было переключиться на выполнение других задач или просто медитативно поглядывать на бегущие полосы процессов PVS-Studio.exe в окне Build Monitor. После завершения анализа с использованием IncrediBuild я сравнил временные показатели продолжительности анализа с результатами замеров без применения IncrediBuild. Разница ощущалась, но общий результат, как мне показалось, мог бы быть и лучше: 182 минуты на одной машине с 8 потоками и 50 минут с использованием IncrediBuild и 145 потоками. Получалось, что количество потоков возросло в 18 раз, но при этом время анализа уменьшилось всего в 3,5 раза. Напоследок я опять решил взглянуть уже на итоговый результат окна Build Monitor. Прокручивая нижний ползунок, я заметил следующие аномалии на графике:

Было видно, что процессы PVS-Studio.exe запускались и успешно завершались, а затем по какой-то причине происходила некая пауза между запуском следующей партии процессов PVS-Studio.exe. И так повторялось несколько раз. Пауза за паузой. И эти простои между запусками складывались в ощутимую задержку и вносили свою лепту в общее время анализа. Первое, что пришло в голову, это вина IncrediBuild. Наверняка он выполняет какую-то свою внутреннюю синхронизацию между процессами и тормозит их запуск.

Я показал эти аномалии на графиках своему старшему коллеге. Он, в отличие от меня, не был так поспешен в выводах. Он предложил посмотреть на то, что происходит внутри нашего приложения CLMonitor.exe в момент того, как на графике начинают появляться видимые простои. Запустив анализ повторно и дождавшись первого очевидного "провала" на графике, я подключился к процессу CLMonitor.exe с помощью отладчика Visual Studio и поставил его на паузу. Открыв вкладку Threads, мы с коллегой увидели около 145 приостановленных потоков. Перемещаясь по местам в коде, где остановили своё исполнение данные потоки, мы увидели строки кода подобного содержания:

....return String.Intern(settings == null ? path                                 : settings                                 .TransformToRelative(path.Replace("/", "\\"),                                                      solutionDirectory));....analyzedSourceFiles.Add( String.Intern(settings                        .TransformPathToRelative(analyzedSourceFilePath,                                                  solutionDirectory))                       );....

Что общего мы видим в этих строках кода? В каждой из них используется метод String.Intern. Причём его применение кажется оправданным. Дело в том, что это те места, где CLMonitor.exe обрабатывает данные, приходящие от процессов PVS-Studio.exe. Обработка происходит путём записи данных в объекты типа ErrorInfo, инкапсулирующего в себе информацию о найденной анализатором потенциальной ошибке в проверяемом коде. Интернируем мы тоже вполне разумные вещи, а именно пути до исходных файлов. Один исходный файл может содержать множество ошибок, поэтому нет смысла, чтобы объекты типа ErrorInfo содержали в себе разные строковые объекты с одинаковым содержимым. Логично просто ссылаться на один объект из кучи.

После недолгих раздумий стало понятно, что интернирование здесь применено в очень неподходящий момент. Итак, вот какую ситуацию мы наблюдали в отладчике. Пока по какой-то причине 145 потоков висели на выполнении метода String.Intern, кастомный планировщик задач LimitedConcurrencyLevelTaskScheduler внутри CLMonitor.exe не мог запустить новый поток, который в дальнейшем бы породил новый процесс PVS-Studio.exe, а IncrediBuild уже запустил бы этот процесс на удалённой машине. Ведь, с точки зрения планировщика, поток ещё не завершил своё исполнение он выполняет преобразование полученных данных от PVS-Studio.exe в ErrorInfo с последующим интернированием. Ему всё равно, что сам процесс PVS-Studio.exe уже давно завершился и удалённые машины попросту простаивают без дела. Поток ещё активен, и установленный нами лимит в 145 потоков не даёт планировщику породить новый.

Выставление большего значения для параметра ThreadCount не решило бы проблему, так как это только увеличило бы очередь из потоков, висящих на исполнении метода String.Intern.

Убирать интернирование совсем нам не хотелось, так как это привело бы к увеличению объёма потребляемой оперативной памяти процессом CLMonitor.exe. В конечном счёте было найдено довольно простое и элегантное решение: перенести интернирование из потока, выполняющего запуск процесса PVS-Studio.exe в чуть более позднее место исполнения кода (в поток, который занимается непосредственно формированием отчёта об ошибках).

Как сказал мой коллега, обошлись хирургической правкой буквально двух строк и решили проблему с простаиванием удалённых машин. При повторных запусках анализа окно Build Monitor уже не показывало каких-либо значительных промежутков времени между запусками процессов PVS-Studio.exe. А время проведения анализа снизилось с 50 минут до 26, то есть почти в два раза. Теперь если смотреть на общий результат, который мы получили при использовании IncrediBuild и 145 доступных ядер, то общее время анализа уменьшилось в 7 раз. Это впечатляло несколько больше, нежели цифра в 3,5 раза.

String.Intern почему так медленно, или изучаем код CoreCLR

Стоит отметить, что как только мы увидели зависания потоков на местах вызова метода String.Intern, то почти сразу подумали, что под капотом у этого метода присутствует критическая секция с каким-нибудь lock'ом. Раз каждый поток может писать в таблицу интернирования, то внутри метода String.Intern должен быть какой-нибудь механизм синхронизации, чтобы сразу несколько потоков, выполняющих запись в таблицу, не перезаписали данные друг друга. Хотелось подтвердить свои предположения, и мы решили посмотреть имплементацию метода String.Intern на ресурсе reference source. Там мы увидели, что внутри нашего метода интернирования есть вызов другого метода *Thread.GetDomain().GetOrInternString(str)*. Что ж, давайте посмотрим его реализацию:

internal extern String GetOrInternString(String str);

Уже интересней. Этот метод импортируется из какой-то другой сборки. Из какой? Так как интернированием строк всё-таки занимается сама виртуальная машина CLR, то мой коллега направил меня прямиком в репозиторий среды исполнения .NET. Выкачав репозиторий, мы отправились к солюшену с названием CoreCLR. Открыв его и выполнив поиск по всему решению, мы нашли метод GetOrInternString с подходящей сигнатурой:

STRINGREF *BaseDomain::GetOrInternString(STRINGREF *pString)

В нём увидели вызов метода GetInternedString. Перейдя в тело этого метода, заметили код следующего вида:

....if (m_StringToEntryHashTable->GetValue(&StringData, &Data, dwHash)){  STRINGREF *pStrObj = NULL;  pStrObj = ((StringLiteralEntry*)Data)->GetStringObject();  _ASSERTE(!bAddIfNotFound || pStrObj);  return pStrObj;}else{  CrstHolder gch(&(SystemDomain::GetGlobalStringLiteralMap()                                   ->m_HashTableCrstGlobal));  ....  // Make sure some other thread has not already added it.  if (!m_StringToEntryHashTable->GetValue(&StringData, &Data))  {    // Insert the handle to the string into the hash table.    m_StringToEntryHashTable->InsertValue(&StringData, (LPVOID)pEntry, FALSE);  }  ....}....

Поток исполнения попадает в ветку else только в том случае, если метод, занимающийся поиском ссылки на объект типа *String *(метод GetValue) в таблице интернирования, вернёт false. Перейдём к рассмотрению кода, который представлен в ветке else. Интерес тут вызывает строка создания объекта типа CrstHolder с именем gch. Переходим в конструктор CrstHolder и видим код следующего вида:

inline CrstHolder(CrstBase * pCrst)    : m_pCrst(pCrst){    WRAPPER_NO_CONTRACT;    AcquireLock(pCrst);}

Видим вызов метода AcquireLock. Уже хорошо. Код метода AcquireLock:

DEBUG_NOINLINE static void AcquireLock(CrstBase *c){  WRAPPER_NO_CONTRACT;  ANNOTATION_SPECIAL_HOLDER_CALLER_NEEDS_DYNAMIC_CONTRACT;  c->Enter();}

Вот, собственно, мы и видим точку входа в критическую секцию вызов метода Enter. Оставшиеся сомнения в том, что это именно тот метод, который занимается блокировкой, у меня пропали после того, когда я прочитал оставленный к этому методу комментарий: "Acquire the lock". В дальнейшем погружении в код CoreCLR я не видел особого смысла. Получается, версия с тем, что при занесении новой записи в таблицу интернирования поток заходит в критическую секцию, вынуждая все другие потоки ожидать, когда блокировка спадёт, подтвердилась. Создание объекта типа CrstHolder, а следовательно, и заход в критическую секцию происходят сразу перед вызовом метода m_StringToEntryHashTable->InsertValue.

Блокировка пропадает сразу после того, как мы выходим из ветки else. Так как в этом случае для объекта gch вызывается его деструктор, который и вызывает метод ReleaseLock:

inline ~CrstHolder(){  WRAPPER_NO_CONTRACT;  ReleaseLock(m_pCrst);}

Когда потоков немного, то и простой в работе может быть небольшим. Но когда их количество возрастает, например до 145 (как в случае использования IncrediBuild), то каждый поток, пытающийся добавить новую запись в таблицу интернирования, временно блокирует остальные 144 потока, также пытающихся внести в неё новую запись. Результаты этих блокировок мы и наблюдали в окне Build Monitor.

Заключение

Надеюсь, описанный в этот статье кейс поможет применять интернирование строк более осторожно и вдумчиво, особенно в многопоточном коде. Ведь там блокировки при добавлении новых записей в таблицу интернирования могут стать "бутылочным горлышком", как в нашем случае. Хорошо, что мы смогли докопаться до истины и решить обнаруженную проблему, сделав работу анализатора более быстрой.

Благодарю за прочтение.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Ilya Gainulin. Pitfalls in String Pool, or Another Reason to Think Twice Before Interning Instances of String Class in C#.

Подробнее..

Roslyn API, или из-за чего PVS-Studio очень долго проект анализировал

22.04.2021 16:18:43 | Автор: admin

Многие ли из вас использовали сторонние библиотеки при написании кода? Вопрос риторический, ведь без применения сторонних библиотек разработка некоторых продуктов затягивалась бы на очень-очень большое время, потому что для решения каждой проблемы приходилось бы "изобретать велосипед". Однако в использовании сторонних библиотек кроме плюсов имеются и минусы. Один из этих минусов недавно коснулся и анализатора PVS-Studio для C#. Анализатор долгое время не мог закончить анализ большого проекта из-за использования метода SymbolFinder.FindReferencesAsync из Roslyn API в диагностике V3083.

Жизнь в PVS-Studio, как обычно, шла своим чередом. Разрабатывались новые диагностики, улучшался статический анализатор, писались новые статьи. Как вдруг! У одного из пользователей нашего анализатора на его большом проекте в течение дня шёл анализ и никак не мог закончиться. Alarm! Alarm! Свистать всех наверх! И мы свистали, получили дампы от пользователя и начали разбираться в причинах долгого анализа. При подробном изучении проблемы выяснилось, что дольше всех работали 3 C# диагностики. Одной из них оказалась диагностика под номером V3083. К этой диагностике уже и раньше было повышенное внимание, но пора было предпринять конкретные действия. V3083 предупреждает о некорректных вызовах C# событий. Например, в коде:

public class IncorrectEventUse{  public event EventHandler EventOne;    protected void InvokeEventTwice(object o, Eventers args)  {    if (EventOne != null)    {      EventOne(o, args);              EventOne.Invoke(o, args);    }  }}

V3083 укажет на вызовы обработчиков события EventOne в методе InvokeEventTwice. О причинах опасности этого кода вы можете узнать более подробно в документации этой диагностики. Со стороны, логика работы V3083 очень простая:

  • найти вызов события;

  • проверить, корректно ли это событие вызывается;

  • выдать предупреждение, если событие вызывается некорректно.

Из-за этой простоты становится еще интереснее понять причину долгой работы диагностики.

Причина замедления

На самом же деле логика чуть-чуть сложнее. V3083 в каждом файле для каждого типа создает только одно срабатывание анализатора на событие, куда записывает номера всех строк (для навигации в различных плагинах: Visual Studio, Rider, SonarQube), где событие некорректно вызывается. Получается, первым делом необходимо найти все места вызова события. Для подобной задачи в Roslyn API уже имеется метод SymbolFinder.FindReferencesAsync, который и был использован в V3083, чтобы не "изобретать велосипед".

Этот метод советуют использовать во многих руководствах: первое, второе, третье и т. д. Возможно, в каких-то простых случаях скорости работы этого метода и достаточно. Однако, чем больше кодовая база проекта, тем дольше этот метод будет работать. На 100 % мы убедились в этом только после изменения V3083.

Ускорение V3083 после изменения

При изменении кода диагностики или ядра анализатора необходимо проверить, что ничего из того, что раньше работало, не сломалось. Для этого у нас имеются позитивные и негативные тесты на каждую диагностику, юнит тесты для ядра анализатора, а также база open-source проектов (которых уже почти 90 штук). Для чего нам база open-source проектов? На ней мы запускаем наш анализатор для проверки его в "боевых условиях", а также этот прогон служит дополнительной проверкой, что мы ничего не сломали в анализаторе. У нас уже имелся прогон анализатора на этой базе до изменения V3083. Все, что нам осталось сделать, это совершить аналогичный прогон после изменения V3083 и выяснить выигрыш во времени. Результаты нас приятно удивили. Без использования SymbolFinder.FindReferencesAsync в V3083 мы получили ускорение на тестах на 9 %. Если кому-то эти цифры показались незначительными, то вот вам характеристики компьютера, на котором производились замеры:

Думаю, после этого даже самые упертые скептики убедились в масштабах проблемы, которая тихо-мирно жила в диагностике V3083.

Заключение

Пусть всем, кто использует Roslyn API, эта заметка будет предостережением! И вы не допустите наших ошибок. Причем, это касается не только метода SymbolFinder.FindReferencesAsync, но и всех других методов класса Microsoft.CodeAnalysis.FindSymbols.SymbolFinder, которые используют один и тот же механизм.

Также советую всем разработчикам тщательней и внимательней изучать библиотеки, которые вы используете. Я говорю это не просто так! Чтобы понять, почему это так важно, советую изучить две других наших заметки: первая, вторая. В них этот вопрос рассмотрен более подробно.

Кроме диагностик мы занялись другими оптимизациями анализатора PVS-Studio, о чем расскажем в следующих статьях и заметках.

Изменение диагностики V3083 пока что не попало в релиз, поэтому версия анализатора 7.12 работает с использованием SymbolFinder.FindReferencesAsync.

Как упоминалось ранее, замедление анализатора было найдено еще в двух C# диагностиках, кроме V3083. Ради интереса, предлагаю читателям оставлять свои предположения о том, какие это диагностики в комментариях. Когда предположений станет больше 50, я приоткрою завесу тайны и назову номера этих диагностик.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Valery Komarov. Roslyn API: Why PVS-Studio Was Analyzing the Project So Long.

Подробнее..

Категории

Последние комментарии

  • Имя: Макс
    24.08.2022 | 11:28
    Я разраб в IT компании, работаю на арбитражную команду. Мы работаем с приламы и сайтами, при работе замечаются постоянные баны и лаги. Пацаны посоветовали сервис по анализу исходного кода,https://app Подробнее..
  • Имя: 9055410337
    20.08.2022 | 17:41
    поможем пишите в телеграм Подробнее..
  • Имя: sabbat
    17.08.2022 | 20:42
    Охренеть.. это просто шикарная статья, феноменально круто. Большое спасибо за разбор! Надеюсь как-нибудь с тобой связаться для обсуждений чего-либо) Подробнее..
  • Имя: Мария
    09.08.2022 | 14:44
    Добрый день. Если обладаете такой информацией, то подскажите, пожалуйста, где можно найти много-много материала по Yggdrasil и его уязвимостях для написания диплома? Благодарю. Подробнее..
© 2006-2024, personeltest.ru