How can a static analyzer help Discord.NET developers?
Tags: csharp, dotnet, opensource, codequality
Published: 2022-08-25 09:04:43

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\.

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/fq8foqhhh5m4y667tigs.png)

## 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](https://github.com/), we discovered the [repository](https://github.com/discord-net/Discord.Net) 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](https://github.com/discord-net/Discord.Net/tree/2c428600b00e4b3f966158203c564313e5fbfb0a) and checked it with PVS\-Studio\.

## Wrong shift

**Issue 1**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3134/?utm_source=dev_to&utm_medium=articles&utm_content=Discord_NET) 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: 

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

## Purposeless 'Concat' call

**Issue 2**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3010/?utm_source=dev_to&utm_medium=articles&utm_content=Discord_NET) 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](https://pvs-studio.com/en/blog/examples/v3010/?utm_source=dev_to&utm_medium=articles&utm_content=Discord_NET)\.

## A mess of arguments

**Issue 3**

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

PVS\-Studio Warning: [V3066](https://pvs-studio.com/en/docs/warnings/v3066/?utm_source=dev_to&utm_medium=articles&utm_content=Discord_NET) 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:

```cpp
/// <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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3127/?utm_source=dev_to&utm_medium=articles&utm_content=Discord_NET) 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\.

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

The corrected condition will be as follows:

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

## A tricky constructor

**Issue 5, 6**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3117/?utm_source=dev_to&utm_medium=articles&utm_content=Discord_NET) Constructor parameter 'after' is not used\. ThreadUpdateAuditLogData\.cs 13
* [V3005](https://pvs-studio.com/en/docs/warnings/v3005/?utm_source=dev_to&utm_medium=articles&utm_content=Discord_NET) 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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3125/?utm_source=dev_to&utm_medium=articles&utm_content=Discord_NET) The 'guild' object was used after it was verified against null\. Check lines: 76, 62\. SocketResolvableData\.cs 76
* [V3095](https://pvs-studio.com/en/docs/warnings/v3095/?utm_source=dev_to&utm_medium=articles&utm_content=Discord_NET) 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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3080/?utm_source=dev_to&utm_medium=articles&utm_content=Discord_NET) 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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3146/?utm_source=dev_to&utm_medium=articles&utm_content=Discord_NET) 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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3095/?utm_source=dev_to&utm_medium=articles&utm_content=Discord_NET) 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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3022/?utm_source=dev_to&utm_medium=articles&utm_content=Discord_NET) 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](https://pvs-studio.com/en/blog/examples/v3022/?utm_source=dev_to&utm_medium=articles&utm_content=Discord_NET) 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](https://pvs-studio.com/pvs-studio/try-free/?utm_source=dev_to&utm_medium=articles&utm_content=Discord_NET)\. Has the analyzer found any oddities in your code? Drop a comment\!