Roslyn: Programmatic suppression of warnings

[jbevain]

Serialization frameworks and tools embedding .NET Core or Mono often set the value of fields or call methods outside of user code.

This leads the IDE to show warning about unused code while the user of the framework knows the fields and methods are going to be used. One example of this scenario is Unity. A typical game object will look like this:

using UnityEngine;

class BulletMovement : MonoBehaviour
{
    [SerializeField] private float speed;

    private void Update()
    {
         this.transform.position *= speed;
    }
}

In that case:

• the value of the field speed is set by Unity based on the value set in the Unity Editor.
• the method Update is going to be invoked by Unity at each and every frame of the game.

For the Tools for Unity, we'd like a way to suppress the warning CS0649 for the field.

If down the line there is a warning about an unused private method such as Update, we'll want to suppress it as well.

@tmat suggested offline:

we could allow analyzers to register for callbacks on specified compiler warnings, pass the callback the context and let it decide whether the warning should be reported or not.

[tmat]

@mavasani We should discuss this feature request at analyzer design meeting.

[Neme12]

Since field is modified from outside, why not make it public to make that clear that that is possible to happen, even though it's not necessary? It would make the code more readable IMO. This way if I look at the code, I have no idea how the field could ever be modified (and IDE features such as FAR won't help me either) and I'm beginning to think that I'm crazy if I saw it modified.

[Neme12]

Yes, this may be an established pattern in the respected areas so users that are familiar will know how this works. But still, from a readability standpoint, I think making these public would be more appropriate.

[tmat]

We have a similar problem in ASP.NET web apps. Application_Start is marked as unused:

[mavasani]

@tmat Can you file a separate IDE issue for it? IDE0051 should handle this case specially to avoid reporting diagnostics here (and probably also xaml cases).

[tmat]

https://github.com/dotnet/roslyn/issues/30377

[Baste-RainGames]

@Neme12

Yes, this may be an established pattern in the respected areas so users that are familiar will know how this works. But still, from a readability standpoint, I think making these public would be more appropriate.

The fields are marked private because they should not be edited by _code_ from outside the class, and because they shouldn't be modified from outside the class _at runtime_. Instead, they're modified through tools that work directly with the serialized version of the object (a yaml file), _before_ the program is run.

The field's initial value is edited by game designers to define the behavior of the object - like the speed in @jbevain's example. The programmers mark the fields as "[SerializeField] private" to indicate that the value can be set to any arbitrary value _before_ the program starts running. After the program has started (and all the objects are loaded), the value is only modified by the same class. So the encapsulation of the private modifier is very much relevant, and the field should not be made public.

This pattern is generally accepted as the "correct" way to expose data to designers in the Unity community. Setting fields to public in order to have them serialized is generally considered a code smell within the same community.
This means that when the Unity engine went from Mono (that didn't show this warning due to a bug) to Roslyn, a lot of users ended up with hundreds of compiler warnings from their code base for code that's essentially dogmatic. We're left with some pretty poor choices:

• turn the warning off altogether for our code base, which means that we don't get it from places where we should get them (ie. when private fields that are not marked as [SerializeField] is never written to)
• surround every single instance of these fields with #pragma warning disable/restore blocks, which bloats the code and reduces readability
• Keep the warnings, which causes the warning list to essentially be too large to read. In practice, this is the same as turning off all compiler warnings for the project.

A solution where Analyzers could be used to remove warnings instead of just introducing them would fix this issue, and it would probably be generally useful. I imagine there's a lot of other situations in the wild where frameworks subtly changes the semantics of the language, and compiler warnings are generated where they're not useful.

[sharwell]

we could allow analyzers to register for callbacks on specified compiler warnings, pass the callback the context and let it decide whether the warning should be reported or not.

This is a duplicate of #20242

[mavasani]

I agree with @sharwell. In fact, I think Sam has precisely stated the correct approach to implement such a feature in that issue:

_Such a "Diagnostic Filter" feature would likely run after analysis is complete, and be provided a list of reported diagnostics with the ability to suppress specific diagnostics. It would be helpful if the diagnostics were treated as suppressed in the Error List window, with the suppression source set to the name of the filter responsible for suppressing it._

[StephenHodgson]

Seems like a simple fix to just initialize the field with some value (which imho is the right thing to do)

```C#
using UnityEngine;

class BulletMovement : MonoBehaviour
{
[SerializeField]
private float speed = 5f;

private void Update()
{
     this.transform.position *= speed;
}

}
```

That being said, I think the warning is by design.

[somethingSTRANGE]

That's not a fix. It's a workaround to avoid the unnecessary and arguably incorrect warning.

Fields marked with "[SerializeField]" are accessed and assigned implicitly by Unity. Assigning unnecessary and potentially unused values to those fields shouldn't be required. Furthermore, assigning default values will likely cause code analysis to mark those assignments as redundant, because class fields are initialized with default values automatically.

Clearly, the warning is useful for unassigned private fields that do not have the "[SerializeField]" attribute, but when the attribute is applied, the warning should be automatically suppressed, as it's usually incorrect.

I believe a more proper workaround is to wrap the fields with pragma directives, but those also clutter code and shouldn't be necessary. They're also a pain to maintain when refactoring.

using UnityEngine;

class BulletMovement : MonoBehaviour
{
#pragma warning disable 649   // Backing fields are assigned through the Inspector

    [SerializeField]
    private float speed;

#pragma warning restore 649

    private void Update()
    {
        this.transform.position *= speed;
    }
}

[StephenHodgson]

Clearly, the warning is useful for unassigned private fields that do not have the [SerializeField] attribute.

Very true, and while the values are implicitly set via the editor, it's probaby a good idea for developers to be setting a default value as a good habit anyway. (I have a lot of components that I know will utilize the default value 90% of the time, but there's always that edge case).

I've got many open and closed source projects where it's part of our coding guidelines to set a default value.

The pragma "workaround" seems rather combersome than just setting a default value like the warning asks, not to mention a heck of a lot more work to keep up with if something changes.

Furthermore, assigning default values will likely cause code analysis to mark those assignments as redundant, because class fields are initialized with default values automatically.

I've never seen this personally, but I do utilize a few Unity specific tools like a resharper extension that handles correcting some of that code analysis, but I could see this potentially being an issue.

TL;DR if it's not broken don't fix it.

[soraphis]

I've never seen this personally, but I do utilize a few Unity specific tools like a resharper extension that handles correcting some of that code analysis, but I could see this potentially being an issue.

JetBrains Rider 2018.3:

[78Star]

Okay so I have been dealing with this for a while. The warning should still be there, and this is why. If a private field has a property, or gets assigned to from a public method, this warning will go away. If a private serialized field doesn't have one of those two things then it is bad. Clearly that variable needs to be assigned to by hand since you are showing it to the editor, so there should be a way to assign to it from code side as well. I would suggest if you don't want to use prop, then use a Setup() method that has all serialized fields assigned to, which will make your code more suitable for unit testing in the future. I sincerely hope they do not get rid of this warning for [SerializedFeilds] as that would make wrong coding practices accepted.

[Trisibo]

@78Star:

Okay so I have been dealing with this for a while. The warning should still be there, and this is why. If a private field has a property, or gets assigned to from a public method, this warning will go away. If a private serialized field doesn't have one of those two things then it is bad. Clearly that variable needs to be assigned to by hand since you are showing it to the editor, so there should be a way to assign to it from code side as well. I would suggest if you don't want to use prop, then use a Setup() method that has all serialized fields assigned to, which will make your code more suitable for unit testing in the future. I sincerely hope they do not get rid of this warning for [SerializedFeilds] as that would make wrong coding practices accepted.

In some environments you don't need a way to assign the field a value from code. For example, in the case of Unity the fields are assigned values from the editor via Unity's serialization, the editor is not only for visualizing the data. Having a property or a method that assigns the values is unnecessary and a waste of time and resources, since they won't be used.

In general, it may be accepted that not initializing a field is a bad code practice in C#, but in some environments that may not be the case, it's all about the context. In Unity, if you see a serialized private field in code with a default value (explicitly assigned or otherwise), you don't assume that the field will have that value when the instance is first used, because in most occassions it will not; in other environments it will be different.

I agree that removing the warning would be a bad idea, but note that what is being requested here is a way to let the environment/tools/xxx disable the warning when it makes sense according to the accepted practices for each one.

Can't we just explicitly initialize our fields to get rid of the warnings? Well, yes, we can indeed (not without some effort on existing projects, of course), but then we have many plugins in source code form that most probably aren't doing that (because it was the accepted practice), so we still have a problem.

[soraphis]

I would suggest if you don't want to use prop, then use a Setup() method that has all serialized fields assigned to, which will make your code more suitable for unit testing in the future.

In the case of unity, you would not need the setup method, because you can just use prefabs for testing if you need a specific setup.

I sincerely hope they do not get rid of this warning for [SerializedFeilds] as that would make wrong coding practices accepted.

so you fear that people would throw the [SerializedFeilds] attribute on private members just to avoid initialzation warnings, even if they have no serialization system?

[zsoi]

Wouldn't it be possible to use a hinting attribute that marks fields/properties etc. covered by a custom attribute as implicitly read/written to? The ReSharper analysis engine does exactly this with their MeansImplicitUseAttribute.

[Gotcab]

Probably, but this would be similar to adding pragma warning, which is a pain...
We just wan't warnings to be context aware. In the case of Unity, not initializing a [SerializeField] private is normal and shouldn't be flagged as bad pratice.

[zsoi]

We just wan't warnings to be context aware.

Exactly. Context can be provided by different means, my suggestion just tries to be a more generic approach to providing this context to the compiler. Attributes are already used all over the place to provide context for the compiler (struct layouts etc.), so it would be a natural way for providing context.

Probably, but this would be similar to adding pragma warning, which is a pain...

I don't understand why this is similar to adding a pragma. You'd only add the attribute once to the class definition of SerializeFieldAttribute and you're done. The pragma, from my understanding, would have to be added wherever the SerializeFieldAttribute is applied, which indeed is totally impractical.

[Trisibo]

@zsoi The problem I see is that, though it would work for Unity and its SerializeFieldAttribute, for other environments and/or purposes other attributes could be used which could come from sources over which the environment where they are to be used has no direct control (for example, let's say, System.Runtime.Serialization.DataContractAttribute). Passing the context information to the environment's analyzer would solve that, and be more flexible (it could check other information apart from the attributes).

[Chexxxor]

How would we deal with Component Reference fields?

Any [SerializeField] referencing unity component types should never instantiate these through constructors directly (by design), meaning any non-null initializer would be worse practice than anything else mentioned so far. I'm running VSCode pretty lightweight so I don't get the warnings of different tools in VS, and Unity seems to be fine with this, but won't all the code analysis tools in VS complain about trying to access null-references if one implements anything like the example below?

[SerializeField] private MeshRenderer _renderer = null;

private void Start()
{
    _renderer.enabled = false;
}

[PathogenDavid]

@Chexxxor

won't all the code analysis tools in VS complain about trying to access null-references if one implements anything like the example below?

No, because the field _renderer isn't marked as nullable, so the access to _renderer.enabled is considered safe. (Assigning null to a non-nullable field creates a warning, but it does not change the nullability of that field.)

You will, however, get a warning on the declaration of _renderer. (As seen here on SharpLab.)

Right now the only ways to suppress those warnings are:

• Make the field nullable: private MeshRenderer? _renderer = null; (This will cause warnings on all unprotected accesses to _renderer as you described.)
• Add a null-forgiving operator to the assignment: private MeshRenderer _renderer = null!;

Programmatic suppression would give us a new tool, writing a plugin for the compiler that suppresses the CS8625 warnings on the declaration in cases where it doesn't make sense. (Like with Unity, where fields are generally initialized via deserialization and constructors aren't ever written.)

[Trisibo]

@PathogenDavid

First of all, for those a bit confused about David's comment, that's C# 8's Nullable reference types (C# 8 still isn't released, and anyway is not supported by Unity yet, and this feature may require enabling an option).

Add a null-forgiving operator to the assignment: private MeshRenderer _renderer = null!;

The problem I see with that is that if at some point you do if (_renderer == null), some analyzers may generate warnings, since _renderer is suppossed to never be null; you'd have to do if (_renderer? == null), but I have no idea if that's even possible (and in any case it's rather ugly). Though considering Unity overloads == for UnityEngine.Object, maybe some analyzers could take that into account.

Anyway, @Chexxxor, as far as I know, at least some analyzers already take into account the quirks of Unity (see sorafis comment above about JetBrains Rider), so it wouldn't be an issue for _all_ of them.

But maybe we are going a bit off-topic.

[mavasani]

Fixed with https://github.com/dotnet/roslyn/pull/36067 in VS2019 16.3

[renattou]

Created a feature request for Unity to implement a new DiagnosticSuppressor: https://forum.unity.com/threads/feature-request-use-new-diagnosticsuppressor-api-to-suppress-cs0649-on-serializefield.697514/