Search Unity

  1. Megacity Metro Demo now available. Download now.
    Dismiss Notice
  2. Unity support for visionOS is now available. Learn more in our blog post.
    Dismiss Notice

Caution: Debug.Assert calls are omitted from built apps

Discussion in 'Scripting' started by JoeStrout, Sep 20, 2016.

  1. JoeStrout

    JoeStrout

    Joined:
    Jan 14, 2011
    Posts:
    9,859
    I'm not normally one to use exclamation points in my thread titles, but holy cows, this is a serious bug. It appears that the behavior of ref parameters, under at least some circumstances, is different (wrong) in a built app, while it functions correctly within the editor.

    I have some parsing code. It works its way over a string, keeping track of its current position by a ref int parameter, like so:

    Code (CSharp):
    1.     static List<string> ParseRow(string jsonData, ref int pos) {
    2.         List<string> result = new List<string>();
    3.        
    4.         // Check for the open bracket; then parse inner stuff, until we get to a close bracket
    5.         int beforePos = pos;
    6.         Debug.Assert(NextAfterWhitespace(jsonData, ref pos) == '[');
    7.         Debug.Log("Parsing row at position " + beforePos + "; after gobbling [, pos is " + pos);      
    8.         while (jsonData[pos] != ']') {
    9.            // ...details...
    10.         }
    11.         return result;
    12.     }
    You can see it calling a NextAfterWhitespace function, which is just this:

    Code (CSharp):
    1.     static char NextAfterWhitespace(string data, ref int pos) {
    2.         while (pos < data.Length) {
    3.             char c = data[pos++];
    4.             if (c > ' ') return c;
    5.         }
    6.         return (char)0;
    7.     }
    Notice it advancing pos there as it grabs each character, c = data[pos++]. So when this function returns, if pos was < data.Length, pos should be incremented at least one, possibly more. And so the Debug.Log in the calling function should look like this:

    And running within Unity (on Mac), that's exactly what I get. But when I build and test my game (for either Mac or Windows), I get this:

    And of course my parser returns garbage results because it doesn't advance past the square brackets. But, what the frick?!? How can pos++ fail to increment the reference variable?

    Note that I haven't yet pinned down the exact circumstances; it might be only when using ++ inside a string indexer, or it might be when it's a ref of a ref (i.e., have to do with how deeply levels your ref parameter has been passed). The position is still being advanced from line to line, so the reference variable is working in some cases but not others.

    So I'll try to narrow it down more, but I'm a bit freaked out, and thought I should share. It's really disturbing when you can't trust your language to behave as it should on such fundamental stuff. And my google-fu didn't turn up anyone else freaking out about this, suggesting that I may be the first to discover it... but such a subtle language bug could be lurking in any code, causing hidden problems. I feel like there's something under my bed, and it's drooling.

    I'm using Unity 5.3.5p7, with the Scripting Backend set to Mono2x (the only option), x86 architecture.
     
    dithyrambs likes this.
  2. JoeStrout

    JoeStrout

    Joined:
    Jan 14, 2011
    Posts:
    9,859
    Hooray! I'm an idiot.

    It turns out not to be a deep hairy C# bug after all, but a misunderstanding (which I blame on the completely undocumented nature) of a Unity "feature":

    Debug.Assert calls — including any methods called in their arguments — are completely omitted in built apps.

    Debug.Assert is a fairly new-ish addition to the framework, but since I discovered it a couple months ago, I've been using it more and more in place of my own assert methods. The docs don't say anything about these getting magically stripped out of built apps, so I was cheerfully calling things that were important, just to be sure the results were what I expected... as in

    Code (CSharp):
    1.         Debug.Assert(NextAfterWhitespace(jsonData, ref pos) == '[');
    which I read as "Advance to the next character after whitespace, and let's make sure it's the expected left-bracket." But in reality it's "Advance to the next character after whitespace, and make sure it's a left-bracket, unless you're in a built app, in which case don't bother doing any of that, because who would want their app to behave the same when built as when testing?"

    Yeah, I'm a little grumpy about it. :)

    So now I need to quit using Debug.Assert, and go back to using my own assert methods, which will not only avoid such nasty-side effects, but also give me a clue in the player log if an assertion (that should never fail) actually does fail at runtime for some reason.

    And I guess I should also go log a bug against the documentation.

    But anyway, you've stumbled across this thread, so now you know too... don't tear your hair out for an hour like I just did!
     
    dithyrambs, Joe-Censored and ModLunar like this.
  3. NickAtUnity

    NickAtUnity

    Unity Technologies

    Joined:
    Sep 13, 2016
    Posts:
    84
    Yes, the documentation looks like it'd benefit from a more direct disclaimer. The Debug class is modeled in some ways on the System.Diagnostics.Debug class in .NET. You can see there that the methods like Assert have a Conditional attribute which indicates they are stripped from release builds. The same intention applies to the Debug class in Unity. The documentation hints at this with "Class containing methods to ease debugging while developing a game." but you're right it should be more explicit. I'll let the documentation team know.
     
  4. NickAtUnity

    NickAtUnity

    Unity Technologies

    Joined:
    Sep 13, 2016
    Posts:
    84
    I did some more digging and found another documentation page which has this helpful bit:

    There are two things in there for you.
    1. If you do your own script for builds, you can configure the BuildOptions to include all assertions.
    2. If you don't do that, you can still add UNITY_ASSERTIONS to the Scripting Define Symbols inside of Player Settings.
    I tested the second option and it seemed to work as you'd want in a Windows standalone build. You might give that a try and see if that resolves it for you.

    And yes, always file bugs against documentation if things aren't accurate or clear. :)
     
    ModLunar likes this.
  5. JoeStrout

    JoeStrout

    Joined:
    Jan 14, 2011
    Posts:
    9,859
    I appreciate the reply. That doc page is about the Assert class, though — I was using Debug.Assert, not Assert.IsTrue. (Perhaps one calls the other under the hood, but that's an implementation detail we shouldn't need to know.)

    I do see how .NET's System.Diagnostics.Debug is documented to be Conditional, but yeah, that's not a very direct clue to Unity's Debug.Assert behavior.

    I'm not going to go mucking about with build settings because then I'd have code that works in some projects but fails (in weird ways) in others, depending on whether the build settings have been set as intended. Better to just avoid the method altogether. I just went back to using my own Assert method.

    In my view,
    Code (CSharp):
    1. foo = expr;
    2. SomeMethod(foo);
    should be no different than
    Code (CSharp):
    1. SomeMethod(expr);
    except that the latter is often preferable, because it's shorter/cleaner code and doesn't leave a local variable lying around asking for trouble. So any time these two do not result in the same behavior, there should be big warning flags complete with bells, sirens, and people in orange jackets flapping their arms to get your attention.

    Documentation bug filed this morning. :)
     
    dithyrambs likes this.
  6. NickAtUnity

    NickAtUnity

    Unity Technologies

    Joined:
    Sep 13, 2016
    Posts:
    84
    The problem is that there are absolutely times when you don't want this. Generally speaking asserts are used during development to state things that you believe should always be true but would like to validate while developing the game. Then those asserts are removed for release builds so that you don't spend the CPU cycles doing that validation once you're confident in the code. When that happens, the compiler is removing the entire function call including all argument expressions because your expressions might be logic related solely to the assertion (e.g. calling a method to compute some value that you only need for the assertion). Therefore you absolutely want those two code snippets to behave differently.

    I'm not saying you shouldn't have the choice to keep them and I'm not saying the documentation shouldn't have a much better notice about this, but I do believe that more often than not developers expect these types of debug asserts to be stripped from release builds. For runtime checks in C#, the usual pattern is to have an explicit if statement that throws a custom exception. You can obviously wrap that in a method, but those are generally not labeled as assertion methods.

    Very many thanks! Bugs are critical to help the documentation team learn about and improve rough areas in the documentation.
     
    noio likes this.
  7. JoeStrout

    JoeStrout

    Joined:
    Jan 14, 2011
    Posts:
    9,859
    Well, I understand some people think that way... but I'm with this guy, and this other guy, firmly in the "assertions should be left in production code" camp (except of course where profiling shows that unusual measures are justified).

    But I know that game developers are (A) paranoid about performance, and (B) more likely than other engineers to believe their code is bug-free. :) And I do get your point about asserts that might be doing computation only to check something that should never fail. So it's not a surprising choice now that I know about it. But, yeah, a note in the docs will help avoid tripping up other people in the future.
     
    dithyrambs likes this.
  8. Kiwasi

    Kiwasi

    Joined:
    Dec 5, 2013
    Posts:
    16,860
    Nice catch.

    My first thought when I saw the OP was that having a side effect in an assert is bound to lead to trouble. I personally wouldn't recommend the use any side effects in anything called from the Debug class.

    But it would be nice to have this explicitly called out in the docs. Many Unity programmers don't come from a background where this is obvious.
     
    AubryStrawberry and JoeStrout like this.
  9. JoeStrout

    JoeStrout

    Joined:
    Jan 14, 2011
    Posts:
    9,859
    Yeah, and in fact years of relying on Debug.Log (which works even in builds) had led me to believe I could rely on Debug.Assert too.

    Not a problem once you know about it, but as my public display of ignorance above shows, the undocumented feature can certainly lead one to bark up the wrong tree!
     
    ModLunar and Kiwasi like this.
  10. MattVD

    MattVD

    Joined:
    Jan 15, 2017
    Posts:
    1
    Did that bug escape, Nick? I cannot see anything regarding this in the Unity 5.5 or 5.6b documentation

    Debug.Assert is definitely omitted in a non-development build. I always find it a bit strange that other 'Debug' calls, such as Debug.Log are not, and remain in production builds. Maybe if the documentation team gets around to adding that bit to the Debug.Assert documentation, they could also add that for Debug.Log and Debug.LogXXX?
     
    Last edited: Mar 2, 2017
  11. DavidSWu

    DavidSWu

    Joined:
    Jun 20, 2016
    Posts:
    183
    It would be nice if Debug.Log calls were [Conditional] based on flags in the player.
    You can waste a lot of time and garbage building debugs strings, accessing classes to get debug info, etc.

    We wrap most of our Debug Logging calls in Conditional functions, but in order to enable/disable them, you have to use a creative selection of predefined symbols or add your own to the scripting define symbols which is no fun at all.

    Something like a warning level filter would be nice.
     
    ModLunar likes this.
  12. Jerry_Alexander

    Jerry_Alexander

    Joined:
    Jun 14, 2019
    Posts:
    6
    Wow, that is very confusing indeed. Having both a UnityEngine.Debug.Assert() method and a UnityEngine.Assertions.Assert class!!
    I hope they unify all debugging and assertions operations, and while at it maybe make the relationship with C#'s own debugging/assertions/tracing features clear.
     
  13. Jerry_Alexander

    Jerry_Alexander

    Joined:
    Jun 14, 2019
    Posts:
    6
    OK, so, I am not a fan of using inductive methods, but I wrote some code to let a GameObject with a TextMeshPro component report some of the information discussed in this thread. In my project, it seems that Unity uses C#'s
    DEBUG
    and
    TRACE
    symbols for the Editor Player and for builds marked as "Development Builds" in the settings, but not otherwise ("Release" versions). But Unity seem to override both the
    System.Diagnostics.Debug
    and
    System.Diagnostics.Trace
    Classes since code using
    System.Diagnostics.Debug.Write("hello")
    will not show up. The
    UNITY_ASSERTIONS
    symbol seems to be defined by default since even if it is absent in Unity's Player Settings it shows up in the editor player and development builds.
    Finally, both
    UnityEngine.Assertions.Assert.IsTrue(bool)
    and
    UnityEngine.Debug.Assert(bool)
    seem to be equivalent since they throw the same exception on Unity Editor's console.
    To find this out for yourself you can add the following script to a GameObject with a TextMeshPro component:

    Code (CSharp):
    1. // Checks whether certain (compilation) Symbols are defined
    2. // Checks whether System Diagnostics work
    3. // Checks whether UnityEngine.Assertions.Assert.IsTrue() is equivalent to UnityEngine.Debug()
    4. // Must attach a GameObject with the TextMeshPro component
    5. using System.Collections;
    6. using System.Collections.Generic;
    7. using UnityEngine;
    8. using TMPro;
    9.  
    10. public class SymbolChecker : MonoBehaviour
    11. {
    12.     private TextMeshPro Output;
    13.  
    14.     void Start()
    15.     {
    16.         Output = GetComponent<TextMeshPro>();
    17.     }
    18.  
    19.     void Update()
    20.     {
    21.         Output.text = ReportDebuggingInfo();
    22.         System.Diagnostics.Trace.Assert(false, "System's Assert is reporting");
    23.         UnityEngine.Assertions.Assert.IsTrue(3 == 4);
    24.         Debug.Assert(3 == 5);
    25.     }
    26.  
    27.     private string ReportDebuggingInfo()
    28.     {
    29.         return ReportDebugSymbol() + ReportTraceSymbol() + ReportUnityAssertionsSymbol() + ReportIsAttached();
    30.     }
    31.  
    32.     private string ReportIsAttached()
    33.     {
    34.         string result = "A Debugger is not attached\n";
    35.         if (System.Diagnostics.Debugger.IsAttached) result = "A Debugger is attached\n";
    36.         return  result;
    37.     }
    38.  
    39.     private string ReportDebugSymbol()
    40.     {
    41.         string result = "DEBUG Symbol Absent\n";
    42. #if DEBUG
    43.         result = "DEBUG Symbol Present\n";
    44. #endif
    45.         return result;
    46.     }
    47.  
    48.     private string ReportTraceSymbol()
    49.     {
    50.         string result = "TRACE Symbol Absent\n";
    51. #if TRACE
    52.         result = "TRACE Symbol Present\n";
    53. #endif
    54.         return result;
    55.     }
    56.  
    57.     private string ReportUnityAssertionsSymbol()
    58.     {
    59.         string result = "UNITY_ASSERTIONS Symbol Absent\n";
    60. #if UNITY_ASSERTIONS
    61.         result = "UNITY_ASSERTIONS Symbol Present\n";
    62. #endif
    63.         return result;
    64.  
    65.     }
    66.  
    67. }
    68.  
     
  14. Jerry_Alexander

    Jerry_Alexander

    Joined:
    Jun 14, 2019
    Posts:
    6
    Regarding my old post:
    "I hope they unify all debugging and assertions operations, and while at it maybe make the relationship with C#'s own debugging/assertions/tracing features clear."​
    I'd like to say that Unity 2019.4 and Visual Studio 2019 do have a common debugging mechanism. Though I haven't used it myself, the following page explains it: