microsoft / Microsoft.Unity.Analyzers

Roslyn analyzers for Unity game developers
MIT License
689 stars 74 forks source link

static keyword in MonoBehaviour messages #282

Closed bdovaz closed 1 year ago

bdovaz commented 1 year ago

Problem statement

I have several analyzers and some of them do not take into account the particularities of the Unity APIs.

In this case I mean when you have for example

class Test : MonoBehaviour
{
  void OnGUI()
  {
    Debug.Log("Test");
  }
}

There are certain analyzers I have that try to convert the OnGUI method to static, but at least in the case of OnGUI I know it causes it not to be called and possibly occur with other Unity messages.

Proposed solution

Suggest that if someone has a special Unity method implemented, if they have static set, an error should be raised.

sailro commented 1 year ago

Could you share the analyzer id trying to convert your method to static ?

bdovaz commented 1 year ago

I can't tell you exactly but I can tell you at least the analyzers we have in the project in case it helps:

https://www.nuget.org/packages/Meziantou.Analyzer/

https://www.nuget.org/packages/Microsoft.CodeAnalysis.NetAnalyzers

https://www.nuget.org/packages/Microsoft.Unity.Analyzers/

https://www.nuget.org/packages/SonarAnalyzer.CSharp/

I suppose that creating a new Unity project with this manifest.json file and running a dotnet format on the solution with a script like the one you posted should reproduce it.

We run with Azure Pipelines the command like this:

dotnet format --severity warn --verbosity diagnostic $projectFilePath

{
    "dependencies": {
        "org.nuget.meziantou.analyzer": "2.0.56",
        "org.nuget.microsoft.codeanalysis.netanalyzers": "7.0.1",
        "org.nuget.microsoft.unity.analyzers": "1.16.1",
        "org.nuget.sonaranalyzer.csharp": "9.1.0-70676",
        "com.unity.modules.ai": "1.0.0",
        "com.unity.modules.androidjni": "1.0.0",
        "com.unity.modules.animation": "1.0.0",
        "com.unity.modules.assetbundle": "1.0.0",
        "com.unity.modules.audio": "1.0.0",
        "com.unity.modules.cloth": "1.0.0",
        "com.unity.modules.director": "1.0.0",
        "com.unity.modules.imageconversion": "1.0.0",
        "com.unity.modules.imgui": "1.0.0",
        "com.unity.modules.jsonserialize": "1.0.0",
        "com.unity.modules.particlesystem": "1.0.0",
        "com.unity.modules.physics": "1.0.0",
        "com.unity.modules.physics2d": "1.0.0",
        "com.unity.modules.screencapture": "1.0.0",
        "com.unity.modules.terrain": "1.0.0",
        "com.unity.modules.terrainphysics": "1.0.0",
        "com.unity.modules.tilemap": "1.0.0",
        "com.unity.modules.ui": "1.0.0",
        "com.unity.modules.uielements": "1.0.0",
        "com.unity.modules.umbra": "1.0.0",
        "com.unity.modules.unityanalytics": "1.0.0",
        "com.unity.modules.unitywebrequest": "1.0.0",
        "com.unity.modules.unitywebrequestassetbundle": "1.0.0",
        "com.unity.modules.unitywebrequestaudio": "1.0.0",
        "com.unity.modules.unitywebrequesttexture": "1.0.0",
        "com.unity.modules.unitywebrequestwww": "1.0.0",
        "com.unity.modules.vehicles": "1.0.0",
        "com.unity.modules.video": "1.0.0",
        "com.unity.modules.vr": "1.0.0",
        "com.unity.modules.wind": "1.0.0",
        "com.unity.modules.xr": "1.0.0"
    },
    "scopedRegistries": [
        {
            "name": "Unity NuGet",
            "url": "https://unitynuget-registry.azurewebsites.net",
            "scopes": [
                "org.nuget"
            ]
        }
    ]
}
sailro commented 1 year ago

I do not repro reusing the exact same manifest, in both VS, Unity and using the dotnet command line. Perhaps something is missing.

bdovaz commented 1 year ago

It may be caused by this rule but I am not sure either, just in case I have disabled it (is deprecated but enabled by default):

https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0038.md

I close the issue and if it comes up again, I will comment.

Thanks.

sailro commented 1 year ago

And the good news is that we already support suppressing CA1822.