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

![0974_DiscordDotNet/image1.png](https://import.viva64.com/docx/blog/0974_DiscordDotNet/image1.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/) 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/) 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/)\.

## 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/) 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/) 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/) Constructor parameter 'after' is not used\. ThreadUpdateAuditLogData\.cs 13
* [V3005](https://pvs-studio.com/en/docs/warnings/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**

```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/) 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/) 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/) 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/) 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/) 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/) 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/) 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/en/pvs-studio/try-free/)\. Has the analyzer found any oddities in your code? Drop a comment\!