Problem statement

Context:

Unity is using two distinct representations for a RGBA Color:
Color: Each color component is a floating point value with a range from 0 to 1. (this format is used inside all graphics cards and shaders).
Color32: Each color component is a byte value with a range from 0 to 255. (32bit RGBA).

Color32 is much faster and uses 4X less memory. Color and Color32 can be implicitly converted to each other.

Note: sometimes though 32b RGBA is not enough (like for HDR or Dolby Vision or advanced color manipulation).

Proposed solution

Report a diagnostic for all SetPixels overloads (and propose to use SetPixels32 instead):

• UnityEngine.Texture2D
• UnityEngine.Texture3D
• UnityEngine.Cubemap
• UnityEngine.Texture2DArray
• UnityEngine.Texture3DArray
• UnityEngine.CubemapArray

Users targeting HDR/Dolby Vision scenarios can still disable this diagnostic.

So far we only test analyzers. We need to test suppressors as well in our CI.

The new-analyzer project has not been updated after we updated the testing infrastructure.

Bug description

This is more of a query as to the scope of the rules, rather than a bug report.

• Version of analyzers assembly: Unofficial.Microsoft.Unity.Analyzers 1.0.0
• Analyzer rules: UNT0007 and UNT0008

These rules appear to be flagging anything that is derived from UnityEngine.Object, however it appears the issues tackled by UNT0007 and UNT0008 are primarily related to stuff derived from MonoBehaviour - as far as I can tell.

For example, in the game Cities: Skylines:

prop.m_finalProp.m_mesh.name

The prop and m_finalProp are both derived from MonoBehaviour - as expected, anything null-related is problematic with them. The analyzer correctly flags that issue.

However, the m_mesh is derived from Unity.Object but not MonoBehaviour. In my testing I've not encountered any problems using ?., ?? or == null - they have always behaved as expected.

Also, in the case of == null checking instances derived from MonoBehaviour:

// problematic - fails to spot null ref
if (someThing == null) ...

// reliable
if (someThing) ...

The analyzer does not catch that issue.

Note: Cities: Skylines runs on Mono "2.0" (in Dr. Evil air quotes, b/c #define VERSION 2.0 and b/c game dev applied some extra tweaks so it's sort of v2.ish).

Note 2: I've not done much testing with GameObject so not sure if that's problematic like MonoBehaviour.

Bug description

Even though project is rebuilt, cleaned, etc, the green squiggles in Solution Explorer do not disappear, except when you disable Disable the full build of projects. As soon as you do that the problem is instantly fixed.

Visual Studio Version 16.5.4
VSTU 4.6.0.0 Experimental
also installed Microsoft Code Analysis 2019 3.0.0.1925803

To Reproduce

Steps or code to reproduce the behavior:

1. edit your code until you trigger some of these analyzers
2. do some work that rebuilds the project
3. see that errors are non-existent
4. now disable Disable the full build of projects.
5. rebuild your project
6. analyzer errors are now gone

Expected behavior

Disable the full build of projects should not influence the analysis.

(in Tools/Options/Tools for Unity)

Screenshots

Also, here all the extensions I have installed:

Problem statement

Like in #19 and #58, IDE0051 is still raised for MenuItem

        [MenuItem("Tools/Project Reference Generator")]
        private static void Init()
        {
            var window = GetWindow<ProjectReferenceGenerator>(true, "Project Reference Generator", true);
            window.minSize = window.maxSize = new Vector2(300.0f, 100.0f);
        }

VS 16.6.2
VSTU 4.6.0.0

Bug description

USP0001 and USP0002 are not working when using a condition in parentheses

To Reproduce

When using:

    var v = o == null ? null : o.ToString();

or

    return a != null ? a : b;

IDE0029 and IDE0031 are correctly suppressed.

but when using

    var v = ((o == null)) ? null : o.ToString();

or

    return (a != null) ? a : b;

IDE0029 and IDE0031 are displayed.

Edit:

It is not working as well when used as an argument:

    transform.SetParent( _area != null ? _area.transform : null );

See the image in:

https://twitter.com/JfranMora/status/1224742294483619842?s=20

The method ToggleInstance is greyed out as it's statically unused. We should also cover that case in our suppressor.

Problem statement

Consider this LateUpdate() on a MonoBehaviour:

        [UsedImplicitly]
        protected void LateUpdate()
        {
            DoSomethingNotInstanceRelated();
        }

CA1822 suggets to mark the member as static for performance reasons.

However, Unity messages will not be processed if they are marked static; it would be a breaking change.

Proposed solution

CA1822 should be suppressed for MonoBehaviour messages.

If there is more than one Analyzer ID in a file (like CreateInstance), it does not correctly increment the ID for a new analyzer created using new-analyzer

It's generally recommended to cache the result of expensive calls like Camera.main and GetComponent inside the Start message and avoid calling them in Update/FixedUpdate.

Proposed solution

We should detect those calls, offer to cache the result and replace the invocations by the cached field.

Bug description

• Version of analyzers assembly: master
• Analyzer rule: USP0008

To Reproduce

using UnityEngine;

public class Rotator : MonoBehaviour {

	void Update () 
	{
		Invoke("InvokeMe", 0f);
	}

	// this one is (and should be suppressed)
	void InvokeMe()
	{

	}
}
public class Unrelated : MonoBehaviour
{
	// we should have IDE0051 for this, but we suppress it
	void InvokeMe()
	{

	}
}

Expected behavior

In this case, we should not suppress IDE0051 for Unrelated.InvokeMe.
We do that because of https://github.com/microsoft/Microsoft.Unity.Analyzers/blob/main/src/Microsoft.Unity.Analyzers/UnusedMethodSuppressor.cs#L54 which is completely wrong.

Bug description

SerializeFieldSuppressor fails to suppress CS0649 in supported scenarios.

• Version of analyzers assembly: Latest master
• Analyzer rule: USP0007
• Error: D:\Unity Projects\SerializeFieldReferenceTest\Assets\Scripts\SerializeReferenceTestScript.cs(8,20): warning CS0649: Field 'Foo.bar' is never assigned to, and will always have its default value null

To Reproduce

Steps or code to reproduce the behavior:

1. Create empty GameObject, and attach a new script Foo to it.
2. Insert the following code into Foo.cs:

using UnityEngine;

public class Foo : MonoBehaviour
{
    [SerializeField]
    private string bar;

    public void F()
    {
        Debug.Log(bar);
    }
}

3. Build
4. CS0649 appears in both the build output and in Visual Studio

Expected behavior

CS0649 is suppressed

Screenshots

Bug description

I'm getting some warnings in the Visual Studio Error List from analyser exceptions which say they are happening in my UltEvents plugin (it is showing 17 of these warnings, but it looks like there are only two different ones being triggered in multiple places).

• Version of analyzers assembly: 4.3.3.0
• Analyzer rule: EmptyUnityMessageAnalyser and MessageSignatureAnalyser

First warning

Severity	Code	Description	Project	File	Line	Suppression State	Detail Description
Warning	AD0001	Analyzer 'Microsoft.Unity.Analyzers.EmptyUnityMessageAnalyzer' threw an exception of type 'System.NullReferenceException' with message 'Object reference not set to an instance of an object.'.	UltEvents		1	Active	Analyzer 'Microsoft.Unity.Analyzers.EmptyUnityMessageAnalyzer' threw the following exception:
'Exception occurred with following context:
Compilation: UltEvents
SyntaxTree: C:\Users\Kailas\Source\Repos\Games\Mage Wars\Assets\Plugins\UltEvents\Events\UltEventBase.cs
SyntaxNode: PersistentCall AddPersistentCall ... [MethodDeclarationSyntax]@[699..749) (20,8)-(20,58)

System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.Unity.Analyzers.EmptyUnityMessageAnalyzer.AnalyzeMethodDeclaration(SyntaxNodeAnalysisContext context)
   at Microsoft.CodeAnalysis.Diagnostics.AnalyzerExecutor.<>c__53`1.<ExecuteSyntaxNodeAction>b__53_0(ValueTuple`2 data)
   at Microsoft.CodeAnalysis.Diagnostics.AnalyzerExecutor.ExecuteAndCatchIfThrows_NoLock[TArg](DiagnosticAnalyzer analyzer, Action`1 analyze, TArg argument, Nullable`1 info)

Second Warning

Severity	Code	Description	Project	File	Line	Suppression State	Detail Description
Warning	AD0001	Analyzer 'Microsoft.Unity.Analyzers.MessageSignatureAnalyzer' threw an exception of type 'System.ArgumentException' with message 'An item with the same key has already been added.'.	UltEvents		1	Active	Analyzer 'Microsoft.Unity.Analyzers.MessageSignatureAnalyzer' threw the following exception:
'Exception occurred with following context:
Compilation: UltEvents
SyntaxTree: C:\Users\Kailas\Source\Repos\Games\Mage Wars\Assets\Plugins\UltEvents\Inspector\ObjectPicker.cs
SyntaxNode: internal sealed class ObjectPickerWindow ... [ClassDeclarationSyntax]@[11004..29499) (218,4)-(717,5)

System.ArgumentException: An item with the same key has already been added.
   at System.ThrowHelper.ThrowArgumentException(ExceptionResource resource)
   at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
   at System.Linq.Enumerable.ToDictionary[TSource,TKey,TElement](IEnumerable`1 source, Func`2 keySelector, Func`2 elementSelector, IEqualityComparer`1 comparer)
   at Microsoft.Unity.Analyzers.MessageSignatureAnalyzer.AnalyzeClassDeclaration(SyntaxNodeAnalysisContext context)
   at Microsoft.CodeAnalysis.Diagnostics.AnalyzerExecutor.<>c__53`1.<ExecuteSyntaxNodeAction>b__53_0(ValueTuple`2 data)
   at Microsoft.CodeAnalysis.Diagnostics.AnalyzerExecutor.ExecuteAndCatchIfThrows_NoLock[TArg](DiagnosticAnalyzer analyzer, Action`1 analyze, TArg argument, Nullable`1 info)

To Reproduce

Steps or code to reproduce the behavior:

1. Create a Unity project (I'm using 2019.3.2f1) and import UltEvents.
2. Open Assets\Plugins\UltEvents\Events\UltEventBase.cs and Assets\Plugins\UltEvents\Inspector\ObjectPicker.cs.
3. Wait for the analysers to run I guess?
4. Check the Error List window.

Problem statement

C# 8 introduces the ??= operator, which is not going to work with UnityObject.

Proposed solution

Just like ??, we should make sure Visual Studio doesn't offer to use that operator.

Problem statement

Unity (2019.3.2f1) doesn't support switch expressions.

Proposed solution

Suppress the code style suggestion IDE0066 - 'Convert switch statement to switch expression'.

Problem statement

VS 16.6.2
VSTU 4.6.0.0

The following warnings are shown for the code sample below:

IDE0060	Remove unused parameter 'path'
IDE0051	Private member 'XXX' is unused

using System;
using System.IO;
using System.Linq;
using System.Text;
using System.Xml.Linq;
using UnityEditor;

namespace Editor
{
    internal sealed class EnsureCSharp8Postprocessor : AssetPostprocessor
    {
        private static string OnGeneratedCSProject(string path, string content)
        {
            var document = XDocument.Parse(content);

            var root = document.Root;
            if (root is null)
                throw new ArgumentNullException(nameof(root));

            var ns = root.GetDefaultNamespace();

            var element = root.Descendants(ns + "LangVersion").SingleOrDefault();
            if (element == null)
                throw new ArgumentNullException(nameof(element));

            element.Value = "8.0";

            // ReSharper disable once ConvertToUsingDeclaration
            using (var writer = new Utf8StringWriter())
            {
                document.Save(writer);

                content = writer.ToString();
            }

            return content;
        }

        private sealed class Utf8StringWriter : StringWriter
        {
            public override Encoding Encoding => Encoding.UTF8;
        }
    }
}

Proposed solution

Both of these warnings should not be shown.

Bug description

conductor instead of constructor

Problem statement

Coroutines that are called directly are flagged by Visual Studio as having a return value that is unused. Calling a coroutine in this way will result in the coroutine's code not being executed.

Proposed solution

The call to the coroutine should be wrapped in StartCoroutine().

Problem statement

Because the SerializeField attribute does not have an AttributeUsage attribute limiting its usage to fields, people sometimes mistakenly use the SerializeField on properties. We should have an analyzer warning about it.

Also, we've seen people using SerializeField on a public field, which is not necessary either.

Proposed solution

In both cases, we can safely offer to remove the attribute.

The following method should not trigger the UNT0014 message:

void MyMethod<T>() where T : MonoBehaviour
{
    GetComponent<T>();
}

UNT0006 Incorrect message signature shows up on the following methods (and probably a bunch of others):

void OnTriggerEnter() and Stay and Exit
void OnCollisionEnter() and Stay and Exit
void OnAnimatorIK()

But those methods work perfectly fine without the parameters.

Adding the expected parameters causes IDE0060 (Remove unused parameter) if they aren't used.

We need to add detailed documentation for suppressors (like we have for analyzers):

https://github.com/microsoft/Microsoft.Unity.Analyzers/blob/main/doc/index.md

Bug description

CS0649 is triggered for public serialized field of internal types.

To Reproduce

internal class CameraScript : MonoBehaviour
{
    public Transform MyTransform;
}

Expected behavior

We should suppress the warning.

Bug description

UNT0014 triggers in cases with generic type parameters (with Component constraint), which is correct.

Example:

void InvalidCase<T>() where T : Component
{
   GetComponent<T>(); // UNT0014: T is not a Unity Component
}

Here T is a generic type parameter and because of "T: Component" constraint only valid for that case classes can be used as T on call site:

// It's fine
InvalidCase<Transform>();
// Compiler error:
// The type 'int' must be convertible to 'UnityEngine.Component'
// in order to use it as parameter 'T' 
InvalidCase<int>();

Also, that applied to generic class parameters as well.

• Version of analyzers assembly: built on commit - f2b4ab1
• Analyzer rule: UNT0014

To Reproduce

Steps to reproduce:

1. Clone sample repository on that commit - KonH/UnityAnalyzerUsage@49a6381
2. Open project in IDE that supports Roslyn analyzers: SampleProject/SampleProject.csproj
3. Check UNT0014 analyzer warnings at https://github.com/KonH/UnityAnalyzerUsage/blob/master/SampleProject/GenericTest.cs#L22 and https://github.com/KonH/UnityAnalyzerUsage/blob/master/SampleProject/GenericTest.cs#L27
4. Analyzer warnings occur in unexpected context

Expected behavior

UNT0014 should not triggers if T from GetComponent call refers to generic type parameter, which has Component constraint.

Additional context

Related code:

This issue is not obvious in simple codebases but happens in production code while working with some generic wrappers, caching, data management and so on.

Unity 2019.3 introduces the SerializeReference attribute.

This is another annotation for their serialization engine that we need to take into account for suppressors.

Problem statement

A simple problem I've seen from beginners is where they use .transform or .gameObject when it isn't necessary, wasting a bit of performance and making the script longer:

private void Awake()
{
    // Bad:
   transform.name = "Something";
    // Good:
   name = "Something";

    // Bad:
   gameObject.transform.position = Vector3.zero;
    // Good:
   transform.position = Vector3.zero;
}

private void OnTriggerEnter(Collider collider)
{
    // Bad:
    collider.gameObject.GetComponent<Rigidbody>();
    // Good:
    collider.GetComponent<Rigidbody>();
}

Proposed solution

Remove the unnecessary indirection.

Problem statement

The need for magic strings is unfortunately widespread throughout Unity's APIs. For example:

animator.Play("Attack");

This is inefficient because Unity has to call Animator.StringToHash every time, but it's also a bad practice because of the magic string. Are you sure you spelled it correctly? Are you sure it's not "Light Attack" or "Attack 0"? Is that animation state referred to anywhere else in the script or the project? And so on.

The same thing applies to Layers, Materials, Navigation Areas, Resources, Scenes, Shaders, Tags, and probably a few other areas of the engine. But I'll only go over the areas I'm most familiar with.

Proposed solution

I know that Jetbrains Rider is able to validate that tag strings in your code are actually valid tags in the Unity project. I don't know if that would be possible or practical to do with an analyser, but some simple code fixes to use a static field would be a good starting point.

Fix 1 - In current type

public static readonly int Attack = Animator.StringToHash("Attack");

...
animator.Play(Attack);

Fix 2 - In global type

public static class Animations
{
    public static readonly int Attack = Animator.StringToHash("Attack");
}

...
animator.Play(Animations.Attack);

This would need the analyser to create a new script if the Animations class doesn't already exist (like how the warning suppressions can create a GlobalSuppressions.cs file). Probably put it in Assets/Code or Assets/Scripts if they exist, but otherwise just in the root Assets folder. Can it simply ask where you want to save a file?

Animator

Applies to:

• Play, CrossFade, and their ...InFixedTime variants.
• All the parameter methods like GetFloat, SetBool, etc.

Examples as above.

Layers

Applies to:

• LayerMask.NameToLayer
• GameObject.layer

Example:

var mask = (1 << LayerMask.NameToLayer("Default")) | (1 << LayerMask.NameToLayer("Water"));
if (Physics.Raycast(... mask ...))

...
gameObject.layer = 5;

Becomes

public static readonly int DefaultWater = (1 << LayerMask.NameToLayer("Default")) | (1 << LayerMask.NameToLayer("Water"));
public static readonly int Layer5 = 5;

...

if (Physics.Raycast(... DefaultWater ...))

...
gameObject.layer = Layer5;

The separate class would be called Layers.

Ideally the analyser could call LayerMask.LayerToName to determine a proper name for the Layer5 field and initialise it with LayerMask.NameToLayer. Otherwise it could just add a comment recommending the use of LayerMask.NameToLayer.

Resources

Applies to:

• Resources.Load
• AssetDatabase.LoadAsset

Example:

var prefab = Resources.Load<GameObject>("GUI/Text/Damage Text");
var instance = Instantiate(prefab, ...);

Becomes:

private static GameObject _DamageText;

public static GameObject DamageText
{
    get
    {
        if (_DamageText == null)
            _DamageText = Resources.Load<GameObject>("GUI/Text/Damage Text");
        return _DamageText;
    }
}

...
var instance = Instantiate(DamageText, ...);

The separate class for this would be called Assets since Resources is obviously already taken.

This one should only be a suggestion (not a warning) since the loaded asset might only be used once anyway and it adds a lot of extra verbosity.

A fix to move the string out to a constant could also be reasonable.

Problem statement

A method marked with either [InitializeOnLoadMethod] or [RuntimeInitializeOnLoadMethod] will still emit IDE0051:Remove unused private members.

Proposed solution

The following snippet or equivalent should not be flagged as IDE0051:

#if UNITY_EDITOR
        [InitializeOnLoadMethod]
#endif
        [RuntimeInitializeOnLoadMethod]
        private static void InitializeWhatever()
        {
            // ...
        }

Bug description

Types extending ScriptedImporter expose public or [SerializeField] fields like fields in a MonoBehaviour.

See:

https://github.com/Unity-Technologies/AlembicForUnity/blob/master/com.unity.formats.alembic/Editor/Importer/AlembicImporter.cs

For instance.

Expected behavior

This means we should treat those fields like fields of a MonoBehaviour, suppress the CS0649 warning and prevent refactoring suggestions like making them readonly.

Problem statement

Unity messages like Awake, Update, etc. do not need to be public for Unity to call them and are only rarely meant to be called manually.

Also, if a base class has a private message and a child class implements the same one, only the child one will be called without any notification or any way to call the base method, which is pretty much guaranteed to cause bugs.

Proposed solution

Recommend that all Unity messages be protected. Give a warning if they are private, but only a suggestion otherwise.

Inside sealed classes, either disable that suggestion or suppress CS0628 // New protected member declared in sealed class.

A NuGet package would be helpful for running analyzers as part of CI and command line builds, and would make the analyzers available to VS Code users as well. I don't know exactly what work this would entail, but I imagine it should be possible to ship the analyzers on NuGet in a similar manner to the Roslyn Analyzers.

mesh.vertices returns a copy of all vertices. If you have code like mesh.vertices.Length, this will be quite inefficient. It's much better to use mesh.vertexCount or cache mesh.vertices if you plan to use data from vertices array later.

I have seen this at least one time in real code where there was code similar to this:

for(int i=0; i<mesh.vertices.Length; i++)
{
//some stuff
}

There is also a much worse offender which look a bit similar but is perhaps another issue:

    void Update()
    {
        Mesh mesh = GetComponent<MeshFilter>().mesh;

        for (var i = 0; i < mesh.vertexCount; i++)
        {
            //This will copy a new vertices and normals arrays at each iteration
            mesh.vertices[i] += mesh.normals[i] * Mathf.Sin(Time.time);
        }
    }

while correct code should be something like this:

    void Update()
    {
        Mesh mesh = GetComponent<MeshFilter>().mesh;
        Vector3[] vertices = mesh.vertices;
        Vector3[] normals = mesh.normals;

        for (var i = 0; i < vertices.Length; i++)
        {
            vertices[i] += normals[i] * Mathf.Sin(Time.time);
        }

        mesh.vertices = vertices;
    }

Unity Mesh API reference : https://docs.unity3d.com/ScriptReference/Mesh.html

I don't really know how to create an analyzer for this yet. I want to be sure that this is something useful and in the scope of this project first!

Is there anyway to lower the CPU Usage upon Visual Studio using Unity. I'd like to use VS rather than Rider

After looking at the behavior of the suppressor USP0008: Don't flag private methods used with Invoke/InvokeRepeating or StartCoroutine/StopCoroutine as unused, it seems like it might be beneficial to provide an analyzer that will refactor the code.

A proposed solution would be to take the string literal parameter and look for a matching existing method name (allow for a case insensitive search to potentially correct an error in the string representation of the method name) and replace the string literal with the nameof() operator. It has the further benefit of allowing the method to use a rename refactoring without remembering to update the string literals.

Resulting code would look something like the following:

using UnityEngine;

class Camera : MonoBehaviour
{
	void Start()
	{
		Invoke(nameof(InvokeMe), 10.0f);
	}

	private void InvokeMe()
	{
		// Invoke me
	}
}

Problem statement

MonoBehaviour exposes several methods able to invoke using reflection. When using this mechanism, target methods (when access scope is private) are flagged as unused given there are no explicit invocations.

Proposed solution

Suppress IDE0051 for target methods when we detect an invocation to:

• InvokeMethod
• InvokeRepeating
• StartCoroutine
• StopCoroutine

And when using a string literal as first argument. Like in this snippet:

public class DemoBehaviour : MonoBehaviour
{
    public void Update()
    {
        InvokeMethod("Foo");
    }

    private void Foo()
    {
        // bar
    }
}

But we cannot extend that to *Message methods (they are working on gameObject attached behaviors), see:
https://docs.unity3d.com/ScriptReference/GameObject.SendMessage.html
https://docs.unity3d.com/ScriptReference/GameObject.SendMessageUpwards.html
https://docs.unity3d.com/ScriptReference/GameObject.BroadcastMessage.html
Because we cannot properly analyze that, by only working on the SyntaxTree.

I don't understand how I'm supposed to install the analyzers - I've got the latest version of VS installed, and if I open a script from Unity it opens in VS but I don't see anything from the Unity analyzers

Is there something else I'm supposed to install? Am I supposed to build this repo myself in order to use it? It's not very clear...

Recommended on Twitter as part of #unitytips

https://twitter.com/BinaryImpactG/status/1265191692531044352?s=20

Problem statement

Users want to check if a component exists on a GameObject.

Proposed solution

When using GetComponent and null checking the result, TryGetComponent can be used instead and prevents memory allocation.

Note, API is available in 2019.2+

Problem statement

Accessing the transform should be done as few times as possible for performance reasons. If this is still needed in cases such as:

transform.position = newPosition;
transform.rotation = newRotation;

Proposed solution

Whenever users set the position they should do it using:

transform.SetPositionAndRotation(newPosition, newRotation);

We need a tool, similar to new-analyzer, allowing new diagnostic suppressors scaffolding.

We need #8 to be done before, so we can scaffold tests as well.

Problem statement

I'm new to C# dev and Unity dev. I have no idea which language features are supported by .Net 3.5
and Mono 2."0". I want to use latest language features, but don't want my code causing CTD.

Example from PR review where I added <LangVersion>latest</LangVersion> to a .csproj:

Proposed solution

A mapping between .Net version and language feature compatibility, warn/error whenever an incompatible feature is used for the given .Net version.

Side note: How do I even get Microsoft.Unity.Analyzers in to VS2019 - doesn't seem to be a NuGet pkg?

Bug description

(reported from a user)

Using Visual Studio 2019 Enterprise, Visual Studio Tools for Unity (Microsoft.Unity.Analyzers.dll 4.3.3.0) and a large Unity project with a wide variety of code, I get lots of crashes from MessageSignatureAnalyzer of the following form:

1>CSC : warning AD0001: Analyzer 'Microsoft.Unity.Analyzers.MessageSignatureAnalyzer' threw an exception of type 'System.ArgumentException' with message 'An item with the same key has already been added.'.

To Reproduce

Use the following snippet:

using UnityEditor;

class TestWindow : EditorWindow
{
    private void OnDestroy(int foo)
    {
    }
}

Expected behavior

Signature mismatch is detected, and the following fix is offered:

using UnityEditor;

class TestWindow : EditorWindow
{
    private void OnDestroy()
    {
    }
}

Screenshots

N/A

Additional context

After some in-depth investigation I was able to narrow down the causes into the following:

MessageSignatureAnalyzer's GetMessages().ToDictionary call sees two declarations for OnDestroy
(one in EditorWindow and one in ScriptableObject) which produces a key collision exception.

Problem statement

Using GetComponent doesn't give any indication of what your script actually needs in the Unity Editor. That's one of the reasons I prefer serialized fields, but GetComponent still has its uses.

Proposed solution

If this.GetComponent is used anywhere in a MonoBehaviour script, offer a code "fix" to add a [RequireComponent] attribute to that class.

I wouldn't want a blue underline for it though since the script could simply null check the component and ignore it if not present. Maybe the analyser could detect that it isn't being null checked?

Problem statement

This is disturbingly common in commuinity submitted mods, especially small "that couldn't possibly be the cause of the lag" mods:

public class DecimateFps : MonoBehaviour
{
   protected void Update()
   {
        // Code using `System.Reflection` to get/set private properies
   }
}

Is also disastrous in LateUpdate(), OnGUI(), etc.

Often the System.Reflection code will be contained in a separate method - I'm not sure if the analyzer can track that down? Such methods often have a signature similar to Q ReadPrivate<T, Q>(T o, string fieldName) or WritePrivate<T, Q>(T o, string fieldName, object value).

Proposed solution

No code fix suggestion for this, it just needs flagging to user as a warning.

Performance: Never use System.Reflection features in performance critical messages.

Docs could mention:

If you must use System.Reflection to access a private member, consider caching FieldInfo or MethodInfo in Start(), or elsewhere, and use the cached references in the loop instead.

In many cases alternatives are available which don't require reflection. For example, if a game object has a private field referencing a UI component, a direct reference to the component can be obtained using code such as FindObjectOfType<SomeGO>().GetComponent<T>(); the component can be cached and subsequently directly accessed in the update loop.

Problem statement

// Works:
[RuntimeInitializeOnLoadMethod]
private static void Initialise() { }

// Doesn't work (not static):
[RuntimeInitializeOnLoadMethod]
private void Initialise() { }

// Doesn't work (not parameterless):
[RuntimeInitializeOnLoadMethod]
private static void Initialise(int i) { }

Proposed solution

Give a warning if the attributed method is not static because Unity won't call it.

Give an error if the attributed method has any parameters because Unity will throw a NullReferenceException when trying to call it (tested in 2019.3.2f1).

Return values are fine.

The same probably applies to UnityEditor.InitializeOnLoadMethod.

UNT0005 Time.deltaTime used with FixedUpdate

Accessing Time.deltaTime in FixedUpdate gives the exact same value as Time.fixedDeltaTime so using fixedDeltaTime has no actual benefit, but it causes problems if you later move that code into another method that isn't called by FixedUpdate.

Problem statement

In relation to #28, I just found out that GameObjects actually have a gameObject property which is undocumented but seems to just return this, so you can do gameObject.gameObject.gameObject.gameObject....

And the same for transform.

Proposed solution

Unless I'm missing something, it seems to do absolutely nothing so it should give an error any time you use it.

Right now if you want to develop and debug analyzers on a full project instead of just the unit tests, you need Visual Studio on Window to run the analyzers in a VSIX.

We need the equivalent for Visual Studio for Mac.

Problem statement

Sometimes people use GetComponent with types or interfaces which are not Components, which does not work.

Proposed solution

No obvious code fix.