JetBrains / resharper-unity

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

Preprocessor defines #125

Open citizenmatt opened 7 years ago

citizenmatt commented 7 years ago

ReSharper/Rider will parse source code based on the current "context", which is usually the current project configuration - e.g. Debug or Release. As part of that, various preprocessor symbols can be defined - DEBUG and TRACE are common. Unity will also add symbols such as UNITY_5_3_OR_NEWER, ENABLE_SPRITES and UNITY_EDITOR.

These symbols can be used to switch compilation, for example:

   var a = 23;
#if UNITY_EDITOR
   a = a * 3;
#else
   a = a * 6;
#endif

ReSharper/Rider will only parse the branch of the #if statement that is active for the current context. It doesn't parse the inactive branch. This is for several reasons. For example, there is no guarantee that the inactive branch(es) are correct - people use preprocessor symbols as a means of commenting out.

But it's also because the amount of work required to parse for all combinations is unfeasible in an interactive application - one sample Unity project I have here has 66 symbols, the combination required to get full coverage here is huge.

And indeed, even if we did parse all combinations, running inspections and refactoring would now be greatly complicated - values can be initialised more than once, but not at all on another branch. There might be a chance of a null reference on one branch, but the variable might be unused on another. Understanding all of these combinations, merging the results and presenting useful information to the user is a significant problem.

So ReSharper/Rider will only run analysis, inspections, refactorings and so on, based on the current context. Switching context (such as changing from Debug to Release) will change the active pre-processor symbol set and re-run analysis. Essentially, ReSharper/Rider provide help for the current compile target only.

But this means that various refactorings and inspections can miss code in inactive branches (e.g. renaming a variable used in an inactive branch, find usages of a method that's also used in an inactive branch).

This issue is NOT tracking a fix to this problem.

This is a fundamental constraint on the way ReSharper can and does work, and is a non-trivial problem to solve. Instead this issue has two purposes - firstly to serve as an explanation why ReSharper/Rider behave like this. Secondly, to better understand how Unity uses preprocessor symbols, in case there is a more targeted approach ReSharper/Rider could take given a specific use case.

So the question is: how do Unity developers use preprocessor symbols? Are all of the 66 symbols in my test project used independently, or is there actually a more constrained set of contexts? Say, targeting multiple platforms (how many?) that each have a set of capabilities, which require, e.g. ENABLE_AUDIO on one, but not on another? Are there other uses of #if?

SirIntruder commented 7 years ago

Ah, what a annoying problem. This especially:

We learned to just be careful about this, but it still can happen sometimes. I guess it would be nice to get a warning about using UnityEditor in build-included scripts.

if UNITY_EDITOR and platform specifics are by far the most often used.

Another annoyance is that there are two ways of defining directives: PlayerSettings (per platform) and smcs/gmcs/mcs files (project wide). Using the later is unavoidable if you want to use the -unsafe keyword (as far as I know). Visual studio apparently only knows about the player-settings ones, which I believe is problem with VS integration for Unity (I use VS2013, maybe it is version related)

ahokinson commented 7 years ago

Similar to @SirIntruder, I ran into the original issue when I refactored a namespace using ReSharper suggestions and committed my code. The automated builds for Android and iOS had different results which was unexpected. After looking at the error, I saw that because I had my project in the Android platform, the namespace for the Android directive had successfully updated, but not for the iOS directive.

While I can only speak for myself, I believe that these are the most commonly used preprocessors for Unity: https://docs.unity3d.com/Manual/PlatformDependentCompilation.html Again, as mentioned, they are mostly for platform specific code.

For now, I have also adapted to this fact and have been more careful when making automated changes. I think that after reading your post here, I definitely appreciate the complexity of the problem. Hopefully more discussion will yield some best practices at the very least.

Thanks @citizenmatt.

adamgit commented 7 years ago

For me, this is a deal-breaker on using an IDE. If it cannot refactor while respecting preprocessors, then it cannot refactor. Life is too short to chase down bugs created by an IDE rewriting working code into non-working code.

My naive expectation was:

  1. Jetbrains IDE's are high quality: refactoring looks into preprocessor statements automatically always, because any other default is definitely wrong.
  2. If there were any uncertainty, a separate popup (distinct from other ones that come up during refactoring) would hilight the fact the IDE has found a situation that's seriously messed up: refactoring the #if'd out section could be the worst possible thing, or NOT doing it could be the worst possible thing: user should be forced to explicit choose on a case-by-case basis (like running through merge conflicts in a merge tool) whether to refactor each secton.

When it comes to the #if UNITY_EDITOR, I'm surprised that refactoring adds a "using" without automatically putting it in #if UNITY_EDITOR. To me this is a violation of principle of least surprise - nothing else in the file uses that "using", so there's no reason for the using to be main scope - but there's obvious reason for it to be in #if'd scope. Without knowing anything about Unity, Rider should keep the using in #if up until the moment it sees the same classes being used outside #if scope - at which point, by definition, it's correct to unhide the using. No?

adamgit commented 7 years ago

On the subject of "which directives", I think the common list is longer.

Asset authors are generally forced to use the #if UNITY_5_0_5, #if UNITY_5_2_OR_NEWER, etc defines, because Unity makes frequent backwards-compatibility-breaking changes in point releases.

So, for example, Unity 5.3 replaced a lot of APIs. Unity 5.2, 5.3, 5.4, and 5.6 broke a lot of APIs. My Asset-store code has plenty of sections: #if // 5.2 ... #elsif // 5.3 ... #elsif // 5.5+ ... #elsif // 5.0-5.2 ... #else // 4.x

Game projects don't have the same problem - but they're often using that code because they've embedded assets. So if the asset author does a refactor, publishes, and doesn't notice that one of the many codepaths didn't refactor, then some of their users are going to get a broken copy of the asset.

Ethan-VisualVocal commented 7 years ago

I almost exclusively use them for editor-only or debug-only code, and branching platform code, e.g. #if UNITY_EDITOR ... #endif, #if DEBUG ... #endif and

#if UNITY_IOS
...
#elif UNITY_ANDROID
...
#else 
...
#endif
van800 commented 6 years ago

https://twitter.com/Worthless_Bums/status/997506377496039425

silverua commented 6 years ago

@SirIntruder Got exact same issue with Rider including unnecessary using statements that break the build. Ended up contacting Rider support and got this response (which worked for me, btw):

Thanks for contacting us. Rider doesn't have UI for this setting yet (see RIDER-14342), but it honors "Show import items in basic completion" setting if configured with ReSharper. With this setting disabled, Rider won't suggest non-imported symbols in the completion. As a workaround, you can configure Rider settings file directly:

  1. Close Rider
  2. Open %HomePath%.Rider2018.1\config\resharper-host\GlobalSettingsStorage.DotSettings file
  3. Add this line to 'ResourceDictionary' node False</s:Boolean>
  4. Save the file and open Rider. Now completion should not suggest non-imported symbols.

HomePath being in C:\Users\ somewhere. After this setting is applied, Rider no longer tries to add using statements itself, you have to add everything manually. On the other hand, chances of it breaking the build are highly reduced.

SugoiDev commented 6 years ago

Was there any progress to alleviate some of the issues with conditional compilation?

For safety (and to respect the principle of least surprise) Rider/ReSharper should never move anything that is wrapped in a ifdef


A very common issue we face frequently is with imports. They'll be moved out of their conditional blocks and will fail (often silently) at runtime. The most common of those is

#if UNITY_EDITOR
using UnityEditor;
#endif

I do want my usings to be sorted properly, but it should have special treatment for usings that are wrapped into a conditional block. They should remain where they are.


The second most common is using a editor type and having the UnityEditor assembly imported naked, without being wrapped into those symbols, when in a runtime type.

In this case, using the solution suggested to @silverua is not super productive. I do want suggestions for non imported types. I just want that types that are in the UnityEditor* assembly (and types that are in custom assemblies that are set as Editor assemblies) be wrapped in a conditional compile block, to avoid unnecessary build errors.

citizenmatt commented 6 years ago

Code shouldn't be moved out of a #ifdef block - the ReSharper engine (mostly) treats them as a black box and doesn't really do anything with them. If we are moving code out of these blocks, then that's a bug and we'd appreciate repro steps.

89 should address the second issue.

SugoiDev commented 6 years ago

If we are moving code out of these blocks, then that's a bug and we'd appreciate repro steps.

Have a class like below. It should have

#if UNITY_EDITOR
    using UnityEditor;
#endif

namespace WrongNamespace {

    public class TestClass {
        private static void PreventNamespaceRemoval() {
            var x = EditorGUIUtility.fieldWidth;
        }
    }
}

Issue 1

Notice that the using directive was moved out of the ifdef region.

rider moving ifdefed using directive gif

What I would suggest as a fix is either a warning message for each "using" directive Rider decided not to move because it was wrapped into a ifdef region. or move the entire ifdef region (not sure if deterministically possible.


Have the class like this now

namespace TestFolder {
#if UNITY_EDITOR
    using UnityEditor;
#endif

    public class TestClass
#if UNITY_EDITOR
        : IInterfaceFromAnotherNamespace
#endif
    {
        //empty
    }
}

Note:

Second and third situations are unrelated to this issue, let me know if I should a new issue for each

Issue 2

rider import usings in wrong place

Issue 3

rider moves closing curly brackets in special case with ifdef

ctide commented 6 years ago

The issue #3 reported in the previous comment:

notice that rider moved the { and the code is now invalid and no longer compiles.

Is a deal breaker for us, too. We were really hoping to standardize on a set of ReSharper rules and just let code cleanup manage all of our code guidelines for us, but it unfortunately breaks our code at the moment. Is there a separate issue to track that bug?

citizenmatt commented 6 years ago

Thanks @SugoiDev and @ctide. These aren't bugs we can fix as part of the Unity plugin, but need to be fixed in Rider itself. I've raised three issues:

Please vote for them. Hopefully the dev team can get them sorted for Rider 2018.3

nanodeath commented 5 years ago

To save others some clicks:

hiradyazdan commented 5 years ago

I almost exclusively use them for editor-only or debug-only code, and branching platform code, e.g. #if UNITY_EDITOR ... #endif, #if DEBUG ... #endif and

#if UNITY_IOS
...
#elif UNITY_ANDROID
...
#else 
...
#endif

I am not sure if this is related, but how about Release profile? multi-platform directives are not defined when switching to Release mode in Rider.

van800 commented 5 years ago

Afaik, there is no way to switch to Release profile directly from Rider. We are hiding the corresponding piece of UI for Unity generated solutions. What do you mean? Screenshot may help. Thank you!

somethingSTRANGE commented 5 years ago

My experience mirrors what most people have reported earlier, however I'd like to add that when working with consoles, like Nintendo Switch, I often have code blocks that specifically target the hardware, and they tend to get wrapped in #if UNITY_SWITCH && !UNITY_EDITOR or something like that. For example, file access code that uses the standard .NET System.IO API when running in the editor, but the Nintendo API IO calls when running on hardware.

Since Visual Studio / Rider is linked to the Unity Editor, the UNITY_EDITOR is always true. To compile, refactor, or get syntax highlighting on code in the !UNITY_EDITOR block, I have to manually add an #undef UNITY_EDITOR to the top of the file ... sometimes in multiple files to get things to compile correctly in Visual Studio / Rider. Unfortunately, if the files were to be actually saved with that addition, the code would fail to run in the editor.

It would be wonderful if there were an easy way to temporarily apply the #undef UNITY_EDITOR to the entire project or solution, so that I can more easily work with code targeting device hardware. Something that would allow Visual Studio / Rider to work with !UNITY_EDITOR code, but have that code compile and run correctly in the Unity Editor where UNITY_EDITOR is defined.

I know I can remove the UNITY_EDITOR definition from the Project Properties panel (under "Define constants"), but I'd have to do that for multiple projects in the solution, such as Assembly-CSharp, Assembly-CSharp-Editor, and possibly others. Those edits are also overwritten when new C# project files are created by Unity.

vertxxyz commented 4 years ago

I'm unsure whether this thread constitutes a request to have Rider understand if runtime assemblies are referencing UnityEditor code, and suggest an preprocessor define. It's mentioned very early here, and the discussion is still open.

If not, I can make a new issue!

citizenmatt commented 4 years ago

Yeah, this is a rather open ended discussion on trying to figure out how to handle the combinatorial explosion of preprocessor defines and #if statements. #89 is a more concrete action for warning you about using editor APIs in runtime code.

noio commented 4 years ago

Thanks for writing up this explanation from the perspective of the ReSharper/Rider tech. Useful to know, and also definitely understandable why it doesn't "just work" in regards to the combinatorics of preprocessor defines.

However, there might be a smaller set of fixes that would alleviate 80% of the issues around this subject. For one: the issues arising when using UnityEditor is inserted outside of #if UNITY_EDITOR. Just having that single using statement be added inside its a conditional would go a long way towards preventing compile errors that pop up later.

The second would be — as @somethingSTRANGE suggested — to allow a way to quickly toggle between different sets of defines, even just to test if the syntax is correct. Undefining UNITY_EDITOR or having a toggle for quickly enabling UNITY_IOS would go a long way towards making sure certain code paths are workable. It's not a waterproof solution, but it would make writing code for different platforms feel a bit less like working in the dark.

h3902340 commented 4 years ago

Please tell us how to make Rider update the define when changing the defines from "Scripting Define Symbols" in the Player Settings. Rider doesn't recognize the changes.

van800 commented 4 years ago

@h3902340 Rider package 2.0.7 had first approach on that problem, however it turned out not to work in all cases, so I reworked it in 2.0.8 (see https://github.com/van800/com.unity.ide.rider/commit/fa45ac6192095d10a0b5a1fb53e73b8cdf82af59#diff-60331cbcc30463cbdfc2a851d8fdbfc0). In the meantime you may put package to your Packages folder, it would override registry package. Please use https://github.com/van800/com.unity.ide.rider/commits/2.0.8/Packages/com.unity.ide.rider Let us know how it works for you.

h3902340 commented 4 years ago

Hi, thanks for the reply. I haven't tried the newest package yet, but I found a workaround to this issue. In Unity, go to Preference, and then External Tools. Toggle "Editor Attaching" will trigger Rider to reload the project. Just toggle this option and then toggle it back.

HowlingMadZ commented 3 years ago

Hi, thanks for the reply. I haven't tried the newest package yet, but I found a workaround to this issue. In Unity, go to Preference, and then External Tools. Toggle "Editor Attaching" will trigger Rider to reload the project. Just toggle this option and then toggle it back.

Thanks, I needed this just now.

m-ronchi commented 3 years ago

Hi, I'll add my use case to the discussion: I have to maintain a legacy project filled with #if on UNITY_EDITOR, platform (android & ios), and custom defines, abused in all possible ways (e.g. define the same method multiple times with slightly different signature in different #if blocks, or nested chains of #if...#elif...#else...#end) it also has an unfair amount of dead code

it would be great if Rider parsed at least the #define combinations relevant for the project (i.e. android build, ios build, and editor set to either platform) for analysis and refactoring purposes

the list of possible combinations could be either manually specified, or can be inferred from a list of target platforms and the contents of unity build settings for those platforms