JetBrains / resharper-unity

Unity support for both ReSharper and Rider
Apache License 2.0
1.21k stars 134 forks source link

Mark enums that are exposed to the Unity editor as implicitly used #139

Closed harryr0se closed 7 years ago

harryr0se commented 7 years ago

In the following example rider will suggest removing the enum values:

using UnityEngine;

public class MyGameObject : MonoBehaviour
{
    public enum ObjectType
    {
        Amazing,
        Awesome,
        Brilliant,
    }

    public ObjectType objectType;
}

Even if they're used in the follow way later on:

animator.SetInteger("objectType", (int)currentObject.objectType);

As removing them would potentially cause unwanted effects, should they be flagged as implicitly used?

citizenmatt commented 7 years ago

I'm not sure. Given this example, a value is flagged as unused not only because it's being set implicitly but also because you're not checking for the value anywhere, in which case it's a valid warning - you might set the value to Brilliant, but you're not acting on it, or it would be marked as used.

Does this sound right?

SirIntruder commented 7 years ago

Agree with citizenmatt, hint is very useful when you use enums directly in code. I also see that hint is not needed when you use your enum values as strings/ints, but it would be unfair to flat out disable it everywhere.

SugoiDev commented 7 years ago

@citizenmatt you can act on the set value from outside of code. For example, in mecanim state machines. You can also use the value on UI bindings that are set purely in the editor. There's also the case for visual programming (like the popular playmaker).

In short, if something is exposed to the editor, we can't assume it to be unused in a safe way because of soft/weak bindings like that.

CAD97 commented 7 years ago

To leave my opinion,

The current behavior is desirable. Even when exposing enums in the editor, the common use case is something like

public enum MyComponentType { Smart, Dumb, Mediocre }

public class MyComponent : MonoBehaviour
{
  [SerializeField] MyComponentType type;

  void Update() {
    switch (type) {
      case Smart: break;
      case Dumb: break;
      case Mediocre: break;
      default: AssertFail();
    }
  }
}

e.g., the enum is used in code. (Typed by memory; may not be perfectly accurate)

In the provided example (animator interop), the enum is being used as a wrapper around magic ints which the animator provides meaning to. You never touch the values of the enum directly, so they're marked as unused. Yes, removing them would have unwanted side effects. But that's why [UsedImplicitly] exists: to tell the checker that the consumer of your code is using the values implicitly.

The implicit use is in the editor. It is not incorrect to say that doing a [SerializeField] (or public) causes the implicit use of that enum's names, but it's not common to say that it does, either. If you use any serialization schema (which is what is using the names here), the members of a serialized enum are not marked as in use.

You could argue that the use is in the (int)myEnumValue. Again, this has never been marked as using the individual names in an enum, and is generally bad practice: it's always better to use the descriptive names if you have a choice. In this case, the animator is taking an int and not a YourDescriptiveEnum, and so the translation is just a wrapper for a magic value, and my MagicConstants.cs file has "Suppress Warnings: Unused". 🙃

That got a bit long, but TL;DR: cases where the names aren't [UsedImplicitly] abound and this specific case is the one [UsedImplicitly]///SuppressWarnings:Unused are made for. I agree with Matt.

SugoiDev commented 7 years ago

I agree with the current behavior.

Moreover, if we begin treating enums in a special way, it would be inconsistent with public fields in a MonoBehaviour: they would also have to be assumed to be in use, and this is certainly undesirable.

My previous remark remains: Anything exposed to the editor can't be safely assumed unused. This is a sad tradeoff we have to live with in exchange for the Unity editor. This is the main reason I don't recommend relying too much on the editor at all, but this is a distinct subject.

citizenmatt commented 7 years ago

@SugoiDev do you have an example of both using and consuming an enum external to the codebase? Something where it would make sense to mark the enum values as being used?

SugoiDev commented 7 years ago

@citizenmatt

In my opinion, users should use [UsedImplicitly] for the example I'm going to give below. My previous comments on this are mostly because this seems trivial, but it is actually pretty hairy, especially with Unity (and even more so when working in teams).


My current game uses a flag system that is enum based. The designer adds the flags in a manager in the editor, and the system generates some code with the enums and some poor man's (but high performance) notify property changed for the flag.

We have hundreds of such flags, each being an enum that defines the possible states of the flag (and a property to contain the current state and trigger events).

Except in very rare cases, I never actually touch the flags in code. It is all done with soft-bindings at edit-time by the designers in a very simple visual scripting language. In all except a very selected few, the actual values of the enum are never directly used in the codebase.


A concrete example:

We have a PlayerGot100OrbsQuest flag. This is a quest flag, and it has quite a few possible states (for example, None, Underway, Complete)

This flag is checked by several NPCs in order to change certain lines to encourage the player to continue the quest. It is also set by an NPC. All this done by designers using the Unity editor. I never deal with this flag directly in the codebase.

At most, I will use an event that is bound to changes to the flag. Usually not even this, as the designer will bind the event to another flag in the editor as well. Pretty hellish for us programmers, but designers are very productive in the system.

I don't like this system and don't think I'll use this again in the future. I like my bindings like I like my languages: hard and compile-time checked.

screenshot 2017 05 07-19 05 28

citizenmatt commented 7 years ago

Great explanation, thanks!

I think I'm happy to close this issue as won't fix, at least for now. ReSharper/Rider's analysis can only do so much, and I suspect the normal usage of setting the value implicitly in the editor, but then checking the value in code is going to be the major usage, and then ReSharper's behaviour will be correct. When you're heading outside of this pattern, then, as commented, that's when [UsedImplicitly] comes in.

Of course, I'm more than happy to revisit this, so if anyone has a compelling argument why enum values should be marked as used - or a way for ReSharper to see that they're actually being used, then please reopen/comment.