How can a static analyzer help Discord.NET developers?

Discord.NET is a library written in C#. This library is used to interface with the Discord API. How can PVS-Studio help? You will find out in the article below.

Introduction

Discord.NET can be useful for creating any applications, that use Discord API. Most often Discord.NET is used for developing Discord bots.

While browsing GitHub, we discovered the repository of the project and decided: "Why not check the code quality with the static analyzer?" Maybe PVS-Studio can find some hidden issues? Well, let's find out!

For this article we took the project's source code from this commit and checked it with PVS-Studio.

Wrong shift

Issue 1

public enum GuildFeature : long
{
  None = 0,
  AnimatedBanner = 1 << 0,
  AnimatedIcon = 1 << 1,
  Banner = 1 << 2,
  ....
  TextInVoiceEnabled = 1 << 32,
  ThreadsEnabled = 1 << 33,
  ThreadsEnabledTesting = 1 << 34,
  ....
  VIPRegions = 1 << 40,
  WelcomeScreenEnabled = 1 << 41,
}

PVS-Studio Warning: V3134 Shift by 32 bits is greater than the size of 'Int32' type of expression '1'. GuildFeature.cs 147

Here, long *is the base type of enumeration. Therefore, each of the *GuildFeature elements will have a value of this type. The values will be obtained by shifting 1 for a different number of bits.

In this fragment, the shift is performed to numbers ranging from 0 to 41. For the int value, a 32-bit shift is equivalent to its absence, and a 33-bit shift is the same as a shift by 1, and so on. Starting with TextInVoiceEnabled, the values of the enumeration elements are repeating. However, the names of elements with matching values are not semantically connected.

Most likely, elements of this enumeration should not have duplicate values. Thus, an actual shift error has occurred. The L suffix helps implement it correctly.

The developers could have made the mistake for two reasons. They either didn't know that numeric literals are of the int type by default, or they expected the shift to return a value of the long type.

If several enumeration elements actually should share the same value, the following would be far more clear:

public enum MyEnum
{
  Elem1 = ....,
  Elem2 = Elem1
}

Purposeless 'Concat' call

Issue 2

public static async Task<RestGuildUser> AddGuildUserAsync(....)
{
  ....
  if (args.Roles.IsSpecified)
  {
    var ids = args.Roles.Value.Select(r => r.Id);

    if (args.RoleIds.IsSpecified)
      args.RoleIds.Value.Concat(ids);                  // <=
    else
      args.RoleIds = Optional.Create(ids);
  }
  ....
}

PVS-Studio Warning: V3010 The return value of function 'Concat' is required to be utilized. GuildHelper.cs 431

The analyzer reports that the return value from a method is not used, so the call is pointless. Is it so?

In this case, Concat is an extension method from System.Linq. It allows us to get an enumeration that contains elements of two collections. The developer might have expected that the result of executing Concat would change the state of RoleIds.Value, but it did not. Concat only returns the result of merging collections without modifying them. We often see such errors while checking projects – if interested, see the link.

A mess of arguments

Issue 3

async Task<IUserMessage> IDiscordInteraction
                         .FollowupWithFileAsync(string filePath,
                                                string text,
                                                string fileName,
                                                ....)
  => await FollowupWithFileAsync(filePath,
                                 text,                     // <=
                                 filename,                 // <=
                                 ....).ConfigureAwait(false);

PVS-Studio Warning: V3066 Possible incorrect order of arguments passed to 'FollowupWithFileAsync' method: 'text' and 'fileName'. RestInteraction.cs 434

To inspect this warning, let's take a look at the definition of the FollowupWithFileAsync method overloading:

/// <summary>
///     Sends a followup message for this interaction.
/// </summary>
/// <param name="text">The text of the message to be sent.</param>
/// <param name="filePath">The file to upload.</param>
/// <param name="fileName">The file name of the attachment.</param>
....
public abstract Task<RestFollowupMessage>
                    FollowupWithFileAsync(string filePath,
                                          string fileName = null, // <=
                                          string text = null,     // <=
                                          ....);

From the description of this method, we know that the text parameter contains the text of the message being sent and the fileName is the name of the attachment file. If we look at the call site, we will notice that the sequence of passed arguments does not match the expected one. It's hard to imagine a case where we need to pass a filename instead of a text of some message and vice versa. Moreover, there are a number of overloads for this method, where the second argument is text. Probably this factor caused confusion when the developer passed arguments.

Issue 4

public async Task<InviteMetadata>
            CreateChannelInviteAsync(ulong channelId,
                                     CreateChannelInviteParams args,
                                     RequestOptions options = null)
{
  ....
  if (args.TargetType.Value == TargetUserType.Stream)
    Preconditions.GreaterThan(args.TargetUserId, 0,
                              nameof(args.TargetUserId));      // <=

  if (args.TargetType.Value == TargetUserType.EmbeddedApplication)
    Preconditions.GreaterThan(args.TargetApplicationId, 0,
                              nameof(args.TargetUserId));      // <=
  ....
}

PVS-Studio warning: V3127 Two similar code fragments were found. Perhaps, this is a typo and 'TargetApplicationId' variable should be used instead of 'TargetUserId' DiscordRestApiClient.cs 1759

The analyzer has detected a section of code that contains a typo. Now look at the GreaterThan calls. The first call passes args.TargetUserId as the first argument, and nameof(args.TargetUserId) as the third. The second call has args.TargetApplicationId as its first argument, and the third argument is again nameof(args.TargetUserId). Seems odd enough that the third argument is the same in both calls.

The third parameter is the name of the checked variable, as we can observe from the method signature. Interestingly, it is the same for different objects.

public static void GreaterThan(Optional<ulong> obj,
                               ulong value,
                               string name,
                               string msg = null)

The corrected condition will be as follows:

if (args.TargetType.Value == TargetUserType.EmbeddedApplication)
  Preconditions.GreaterThan(args.TargetApplicationId, 0,
                            nameof(args.TargetApplicationId));

A tricky constructor

Issue 5, 6

public class ThreadUpdateAuditLogData : IAuditLogData
{
  private ThreadUpdateAuditLogData(IThreadChannel thread,
                                   ThreadType type,
                                   ThreadInfo before,
                                   ThreadInfo after)
  {
    Thread = thread;
    ThreadType = type;
    Before = before;
    After = After;
  }
  ....
}

Now PVS-Studio issues two warnings at once:

• V3117 Constructor parameter 'after' is not used. ThreadUpdateAuditLogData.cs 13
• V3005 The 'After' variable is assigned to itself. ThreadUpdateAuditLogData.cs 18

Both analyzer warnings indicate the same problem. Obviously, the developer made a mistake in assigning the After value. The property is assigned its own value instead of one of the constructor parameters. This operation makes no sense.

Null errors

Issue 7, 8

internal SocketResolvableData(DiscordSocketClient discord,
                              ulong? guildId,
                              T model)
{
  var guild = guildId.HasValue ? discord.GetGuild(guildId.Value) : null;
  ....
  if (resolved.Members.IsSpecified && guild != null)         // <=
  {
    ....
    var user = guild.AddOrUpdateUser(member.Value);
    ....
  }

  if (resolved.Roles.IsSpecified)
  {
    foreach (var role in resolved.Roles.Value)
    {
      var socketRole = guild.AddOrUpdateRole(role.Value);    // <=
      ....
    }
  }

  if (resolved.Messages.IsSpecified)
  {
    foreach (var msg in resolved.Messages.Value)
    {
      ....
      if (guild != null)                                     // <=
      {
        if (msg.Value.WebhookId.IsSpecified)
          ....
        else
          author = guild.GetUser(msg.Value.Author.Value.Id);
      }
      else
        ....
    }
  }
  ....
}

Again, a couple of warnings for one piece of code:

• V3125 The 'guild' object was used after it was verified against null. Check lines: 76, 62. SocketResolvableData.cs 76
• V3095 The 'guild' object was used before it was verified against null. Check lines: 76, 88. SocketResolvableData.cs 76

A closer look at the guild variable declaration shows that guild can be null. That's why the developer checks it before calling methods. Well, except for one case. So, if the variable does contain null, an exception of the NullReferenceException type will be thrown.

Issue 9

internal class NullableComponentConverter<T> : ComponentTypeConverter<T>
{
  ....

  public NullableComponentConverter(InteractionService interactionService,
                                    IServiceProvider services)
  {
    var type = Nullable.GetUnderlyingType(typeof(T));

    if (type is null)
      throw new ArgumentException($"No type {nameof(TypeConverter)}" +
                                  $"is defined for this {type.FullName}",  // <=
                                  "type");

    _typeConverter = interactionService
                       .GetComponentTypeConverter(type, services);
  }
  ....
}

PVS-Studio Warning: V3080 Possible null dereference. Consider inspecting 'type'. NullableComponentConverter.cs 15

The analyzer reports a possible null reference dereference. In the condition, the type variable is checked for null, and then the FullName property of this variable is accessed in the then branch. Obviously, such an accessing will result in NullReferenceException.

To fix the error, replace type.FullName with typeof(T).FullName.

Issue 10

public sealed class BuildOverrides
{
  private static Assembly
                 _overrideDomain_Resolving(AssemblyLoadContext arg1,
                                           AssemblyName arg2)
  {
    var v = _loadedOverrides
      .FirstOrDefault(x => 
        x.Value.Any(x =>
           x.Assembly.FullName == arg1.Assemblies
                                      .FirstOrDefault().FullName)); // <=

     return GetDependencyAsync(v.Key.Id, $"{arg2}").GetAwaiter()
                                                   .GetResult();
  }
}

PVS-Studio Warning: V3146 Possible null dereference. The 'FirstOrDefault' can return default null value. BuildOverrides.cs 254

FirstOrDefault will return the first Assemblies element or the default value if there are no elements. This collection stores objects of reference type, therefore, the default value will be null. Since the developer expected the Assemblies to have no elements, then it is obscure why there is no check for null before accessing FullName. If the collection is certainly not empty, perhaps, it's better to use First, not FirstOrDefault. Then the code will not raise too many questions.

Issue 11

internal void Update(ClientState state, Model model)
{
  var roles = 
       new ConcurrentDictionary<ulong, SocketRole>
           (ConcurrentHashSet.DefaultConcurrencyLevel,
           (int)(model.Roles.Length * 1.05));         // <=
  if (model.Roles != null)                            // <=
  {
    for (int i = 0; i < model.Roles.Length; i++)
    {
      var role = SocketRole.Create(this, state, model.Roles[i]);
      roles.TryAdd(role.Id, role);
    }
  }
}

PVS-Studio warning: V3095 The 'model.Roles' object was used before it was verified against null. Check lines: 534, 535. SocketGuild.cs 534

Another curious warning related to the potential null dereference occurs. Firstly, the model.Roles.Length property is accessed, and then model.Roles is checked for null. The developers were likely to assume that model.Roles could have a null value, that is why they wrote the check. So, it seems weird that this property is only checked in the second case.

The expression is always false

Issue 12

public IEnumerable<CommandMatch> GetCommands(....)
{
  ....
  int nextSegment = NextSegment(text, index, service._separatorChar);
  ....
  if (visitChildren)
  {
    ....
    if (nextSegment != -1)
    {
      name = text.Substring(index, nextSegment - index);
      if (_nodes.TryGetValue(name, out nextNode))
      {
        foreach (var cmd in
                   nextNode.GetCommands(service,
                                        nextSegment == -1 ? "" : text, // <=
                                        nextSegment + 1,
                                        false))
          yield return cmd;
      }
    }
  }
}

PVS-Studio Warning: V3022 Expression 'nextSegment == -1' is always false. CommandMapNode.cs 109

Take a look at the second* if* in this code snippet, and the nextSegment == -1 ? "" : text expression. The condition result will always be false. This example has no error, just redundant code, to be avoided as well.

In fact, the code containing this type of errors is not always so harmless. If you don't believe me, you can see it yourself — there's a list of errors detected by this diagnostic.

Conclusion

PVS-Studio found some suspicious code fragments in Discord.NET. The majority of them is related to the possible null reference dereference. It would be great if the developers inspected this. And also, the other warnings described in this article.

The static analyzer allows us to save time and money because the errors will be found at the stage of writing code, and not at later stages of development. Well, it's clear that static analysis is not perfect and will not be able to find all the flaws in the project. Anyway, such tools can expedite the project and make the code better.

Can the analyzer help you? Let's see. Has the analyzer found any oddities in your code? Drop a comment!