More Unity Inspections

[CAD97]

UnityEngineAnalyzer is a Roslyn-based tool for Unity-specific code inspections. Below are some of the inspections from UEA, which would make good further inspections for this plugin.

This issue exists to serve as a task-list grouping of inspections "borrowed" from UEA, and individual issues will likely be extracted (and linked) to be worked on.

Where possible I've included sources for these recommendations. Some phrasing is slightly changed to fit my understanding of the hints.

• OnGUI should not be used in MonoBehaviours for production UI Warning
• SendMessage, SendMessageUpwards, and BroadcastMessage can lead to hard to maintain code, and are slower than delegates, events, and direct method calls. Note that it's unlikely we can add static typing to these (Add method reference for BroadcastMessage #84). Suggestion/Hint
• Empty Unity "magic methods" (Start, Update, etc.) cause unnecessary calling overhead. Warning (Mark empty event functions as dead code #137)
• Use CompareTag instead of gameobject.tag == Warning (Inspection for CompareTag #82)
• Find<> and GetComponent<> style methods should not be used within Update or Rendering functions. Cache the result in Awake or Start instead. Warning
• override/virtual functions which are not overridden should be sealed to benefit from devirtualization. Suggestion/Hint

[WeslomPo]

gameObject.Invoke as and SendMessage can lead to hard to maintain code...

[CAD97]

Note: SendMessage cost does become more affordable when multiple Components receive it. Someone should do testing on how other.SendMessage("Foo") or other.GetComponent<Bar>().Foo() compare.

@WeslomPo, I can't tell whether you're agreeing or disagreeing, but keep in mind that stringly typed code is inherently riskier than strongly typed, and that all inspections are opinionated at best. And just because something can lead to code that is hard to follow doesn't mean it always does, or that it's not useful. I just brought over the tips from the other repository as suggestions.

[breineng]

You can prevent that in the base class methods are already available Update, Awake, etc. ?

[CAD97]

@Temka193 The blog post 10000 Update() calls has some more information, but the key point is: Update/Awake/etc aren't virtual methods inherited and overriden from GameObject; they're magic methods that the Unity engine calls when they exist. That's why they work when private and don't need the sealed, virtual, or override .

Unity's native code keeps a list of MonoBehaviours which are subscribing to lifecycle events and then trampolines into the managed code to call the magic methods when appropriate. As that native->managed transition has cost, it's best to not include Unity event methods when they aren't needed.

(Oops, did not mean to hit the Close and comment button....)

[CAD97]

Possible automatic refactoring tied to above warnings:

• OnGUI() -> [System.Diagnostics.ConditionalAttribute("DEVELOPMENT_BUILD")] OnGUI()

This should limit the method to only being present "in a player which was built with the “Development Build” option enabled", but currently for me it's showing up in every build. That could just be user error, however.

• FindComponent<>, GetComponent<>, etc. in an every-frame magic function -> private $type$ $name$; in class and $name$ = $expensivecall$ in Start().

When finding something on another object, it's best practice to wait until Start() such that all objects in the scene have had a chance to initialize themselves in their Awake() callback. Thus, it's simplest and less likely to change behavior to move find-like calls into Start() than Awake(). If the user feels confident, they can move it into Awake() themselves.

• Delete empty magic functions

• override -> sealed when not overridden further

[nkAlex]

Unfortunately, OnGUI() is sometimes required as a workaround for some peculiar Unity's behavior (for example, it seems it's not possible to listen to the KeyCode.SysReq key press anywhere else).

GetComponent<>() usually makes sense in Awake() for components on the same GameObject or in its hierarchy tree, and in Start() — for any "external" objects so that we are sure all of their Awake()s have been called by that time. Or at least it makes sense to me :)

[CAD97]

Why would you ever want to listen for SysReq @nkAlex? In any case, I think OnGUI as the workaround is the exception to the rule. If you're digging deep enough to be doing something that requires that, I suspect you can suppress the warning without guilt.

I agree on the use of GetComponent<>(). I just think it'll be easier to, in the general case, move calls into Start(). If someone wants to codify when which calls should go in Awake() versus Start(), be my guest.

• GetComponent family
• Find family

[nkAlex]

SysReq is the PrintScreen button ;)
I once got really frustrated trying to debug why on Earth the game wouldn't save screenshots. It turned out that SysReq is somewhat a special case button. I think there are one or two more like it, can't remember now.

Well, I'm mostly calling GetComponent<>() in Awake(), that way I'm sure I can call any of the cached components in Start() from other objects without the risk of having a NullReferenceException ;)

Find() also makes more sense in Awake() in my opinion. I kinda tend to put most of the heaviest stuff in there, I suppose. Also, it's the closest we get to a constructor initialization for a MonoBehaviour :)

[breineng]

@CAD97 You did not understand correctly.
I will try to give an example:

public class MyClass : BaseMyClass
{
	//It should be highlighted the error - "Method awake is already defined in the base class, make protected virtual method in the base class and protected override in the current class."
	void Awake()
	{
		//Here code for additional initialization MyClass
	}

	void Update()
	{
		//other code
	}
}

public class BaseMyClass : MonoBehaviour
{
	void Awake()
	{
		//Code for initialization BaseMyClass
	}
}

[CAD97]

@Temka193
If you define an abstract base class and provide a base definition for a magic method, then you're just dealing with C#: it has to follow C#'s rules for virtualization.

For best practice Unity development, however,

You should not unnecessarily implement Unity's magic methods. Doing so causes unnecessary performance overhead, especially if said magic method is called every frame.

The key point is, you control your code. You control the methods which are defined in your base class. Due to the way Unity handles magic methods (see the blog post 10000 Update calls), you should not provide a definition when you aren't running code in it.

I would argue that by the principle of Composition over Inheritance and Unity's Component-based architecture, you shouldn't need to use a custom base class. But that's less a universal design rule in Unity and just my opinion.

[vad710]

Hello all! I am the current maintainer of the linked Roslyn analyzer in the OP.

In general I agree with @CAD97 regarding creating a base class in Unity. Furthermore, it's also generally considered good practice to "pool" update logic into one component when the number of update calls of similar methods gets large - though this is not something that can be analyzed via static code analysis.

It's also worth noting that the devirtualization analyzer is mostly useful when targeting IL2CPP.

We've recently added an analyzer that confirms that the function mentioned in the Invoke and InvokeRepeating methods actually exist. This should be easy to do from within Resharper as well.

[CAD97]

Invoke(Repeating) already is implemented as contributing a method reference (see #41, #66, #85). However, doing so for SendMessage(Upward) hasn't been accomplished yet (#84), and I doubt it's possible to do staticly, as it requires run-time information (attached components, GameObject hierarchy).

[hymerman]

I imagine a warning about GetComponent and Find in Update would get extremely annoying. They shouldn't be done every frame, sure, but what about all the stuff that happens in Update that's conditional and doesn't happen every frame? e.g. Timer expires -> find an object to tell. I just think such a warning would generate far too much noise for anything but the most trivial programs.

[CAD97]

@hymerman, I will admit I haven't worked on any large projects, but I can't think of any occasion where you should need to write rare running code in Update.

Unity is callback-based. For e.g. the Timer example, the Unity way of doing so would be to Invoke("Method", delay), which means that Method is no longer being called from Update. I find that a Find<>/Get<> in Update has never been necessary, but if you have an example of when it really is, I'm all ears!

[hymerman]

Take this with a pinch of salt: I'm just an old-school C++ developer and I care more about performance than ease of use! Unity's event-based approach is great for toy examples and smaller games, but for large games the tiny performance impacts (memory allocation in that case, CPU in others) of things like coroutines really start to add up. Death by a thousand paper cuts and all that. My approach is like that in the 10,000 updates article, but pushed to the extreme; just one bootstrapper game object with an Update method, which calls out to the whole of the rest of the game tree to allow it to update. I have my own implementation of e.g. timer delays which does what the Unity version does, but without the allocation overhead. As such, there's plenty of conditional logic in there :)

I know my approach would be 'special case', but the same happens to lesser degrees in other codebases. I feel like a warning about this would be noisy for anything other than tiny projects.

The other warnings look good though :)

[CAD97]

I'd be happy to take a stab at implementing these inspections @citizenmatt (even if only so I can have them myself), but I don't know where I should start; I've never worked on a R# plugin before. Do you have any suggestions or links for what I could look at to get a quick idea of how this project is structured and how I'd go about adding an inspection? I'd love to contribute, I just lack the knowledge needed to start.

Also, I've gone ahead and categorized the suggested inspections into Warnings or Suggestion/Hints: warnings for things suggested against by Unity Technologies, and suggestions/hints for the subtler and more opinion-based on this list.

As a compromise for Find called in Update, perhaps downgrading it to a suggestion from a warning when it's indirectly called would be beneficial. In any case, if we can determine that it is not guaranteed to run every frame (it's in a conditional block of some sort), it should probably be downgraded to a suggestion, as the warning is against calling it every frame.

[citizenmatt]

Hey. I've somehow managed to miss most of the conversation on this thread. Some great ideas here, thanks. I'm going to pull some of them out into separate issues as they're being worked (mostly because I like closing issues with commit messages. Gives me a lovely sense of progress 😄).

As for working on this yourselves with pull requests and whatnot, that'd be brilliant. I have to confess that the SDK isn't as easy or as complete as we'd like it (ReSharper's APIs and capabilities are kind of huge, and time, resources and priorities are limited, sadly). But there is still a lot of good stuff in the SDK's devguide. And of course, the current code is also a good place to start, to see what's going on.

Inspections are best handled with a class that derives from EventProblemAnalyzer. You can take a look at the current ones here: Daemon/Stages/Analysis/. These will look at the syntax tree and semantic model, looking for a particular pattern and then add a highlight, which are defined and registered in Daemon/Stages/Highlightings/.

Quick Fixes are Alt+Enter actions that are attached automatically to highlights - warnings and errors. And then there's Code Completion, and another useful tool is external annotations.

The work to add code completion, find usages, rename, etc. to string literals, such as Invoke("MyMethod", Delay) is all handled with reference providers (which are documented in the devguide). These are more complex, but you can see them in action in Psi/Resolve/.

If you do clone and try and build, I'd recommend unloading any Rider projects right now. Rider's headless version of ReSharper is more or less the head of master at the time of the EAP, which means it needs its own set of SDK NuGet packages, and they're not publicly available right now. And loading the plugin in Rider right now is undocumented and subject to change. We'll have more of an SDK closer to release, hopefully.

[van800]

There is no more need to borrow ideas from UnityEngineAnalyzer, you can relatively easy add it to your solution. Rider will display roslyn warning directly in Errors in Solution view.
vad710/UnityEngineAnalyzer#27 (comment)
Is it fine to close this issue?

[CAD97]

Unless there's any plans to add any of these still, I'm fine to see this closed!