BYOND says src is null

ID:73469
 
Jun 20 2009, 11:38 am
Resolved
Using a default proc argument value such as arg=src.var sometimes caused strange error messages, such as "cannot read null.var".
BYOND Version:441
Operating System:Windows Vista Home Premium
Web Browser:Firefox 3.0.11
Applies to:Dream Maker
Status: Resolved (512.1386)
This issue has been resolved.
Duplicates:id:122232, id:729348
Descriptive Problem Summary:
I get a runtime error: Cannot read null.hands_free error when trying to read src.hands_free, even though src shouldn't be null and a check has been done first to ensure that it isn't.

Code Snippet (if applicable) to Reproduce Problem:
/mob/human/proc/set_hand(obj/item/I, hand=src.hand)
    if(!src||!src.hands_free||src.lying)return //This line causes an error
    //...
    
/obj/item/MouseDrop(over_object)
    var/mob/human/user=usr
    if(!istype(user)||!(src in view(1))) return ..()
    if(istype(over_object, /obj/HudUI/hand))
        var/obj/HudUI/hand/H=over_object
        user.set_hand(src, H.hand)
        user.Update()


Expected Results:
The expression is evaluated and the proc returns if needed. Even if src was null for some reason., the !src check should prevent an error.

Actual Results:
BYOND seems to think src is null even though the given value for src in the error message below proves that it isn't.
runtime error: Cannot read null.hands_free
proc name: set hand (/mob/human/proc/set_hand)
  source file: human.dm,239
  usr: Nickr5 (/mob/human/native)
  src: Nickr5 (/mob/human/native)
  call stack:
Nickr5 (/mob/human/native): set hand(the tomahawk (/obj/item/weapon/axe), 0)
the tomahawk (/obj/item/weapon/axe): MouseDrop(the hand (/obj/HudUI/hand), the grass (3,1,1) (/turf/grass), null, "mapwindow.map", "mapwindow.map", "icon-x=15;icon-y=17;left=1;scr...")



Does the problem occur:
Every time? Or how often? Every time.
In other games? Not as far as I can tell. I wasn't able to duplicate the problem in a separate project.
In other user accounts? Not sure.
On other computers? Not sure.

When does the problem NOT occur?
When one of the workarounds below are used.

Workarounds:
Replacing the problematic line with this causes everything work fine:
if(!src.vars.Find("hands_free")||!src.hands_free||src.lying)return


Placing ASSERT(src) at the beginning of the set_hand() proc also fixes the probem.
Jun 20 2009, 6:15 pm (Edited on Jun 21 2009, 11:44 am)
Well, considering it's || and not &&, even if src is null, it would continue to check the next condition.

if (!cake || !cake.pie) return
// is cake true or false?
// false: continue to check other values (we need a true)
// true: end

if (!cake && !cake.pie) return
// if cake true or false?
// false: end, skip (they all have to be true)
// true: continue to check other values


Of course I could be wrong.

Though it makes sense the ASSERT makes it escape properly, ASSERT basically translating to if (!value) return.

edit: crap. !cake would return true if cake was null, effectively ending the check... I was tired. =\
Jun 21 2009, 5:36 am
Keeth wrote:
Well, considering it's || and not &&, even if src is null, it would continue to check the next condition.

Depending on the situation both an || and an && could kill as soon as they found a false, which would be the case here. || would be the proper way to go here since he's returning.

However, regardless of the way he set up his if(), if the src in a BYOND proc is missing, that proc should automatically kill itself.
Jun 22 2009, 12:16 pm
I've seen cases of this before and can't tell why it happens. This is a somewhat deep bug and very much an old one, but I suspect it's more fixable than a typical compiler black-box bug, so prospects for a fix are better.
Jul 26 2009, 11:06 am
I believe this is related to the bug I commented on in this thread:
http://www.byond.com/developer/forum/?id=674857&view=1

In short, if you have a proc argument with a default value that's a member variable, it screws up the way the compiler caches the dereferencing of variables in the bytecode.
Aug 23 2009, 10:48 pm (Edited on Aug 24 2009, 5:27 am)
3am posts are bad for people...
Aug 24 2009, 4:11 am
Yeah, Hobnob explained this.
Hobnob wrote:
In short, if you have a proc argument with a default value that's a member variable, it screws up the way the compiler caches the dereferencing of variables in the bytecode.

Jan 19 2013, 6:42 pm
Huge bump, but in case it helps, I found this workaround. Perhaps it could also shed some light on the bug itself.

atom
    proc/test1(i=src.name)
        // error
        world << src.name

    proc/test2(i=name)
        // error
        world << name

    proc/test3(i=src.name)
        // no error
        world << name

    proc/test4(i=name)
        // no error
        world << src.name
Feb 22 2014, 7:57 pm
Bump
Sep 10 2017, 5:52 pm
Bumping because /tg/ ran into this again
Sep 10 2017, 8:16 pm
This is an error in the guts of the compiler.

It can temporarily be prevented by inserting an innocuous setting on the proc.

    proc/test1(i=src.name)
        set invisibility = 101
        world << src.name


There's another thread on this currently active to boot, which Lummox resolved to prevent clutter.
Sep 10 2017, 11:18 pm
This is the main thread that he couldn't find. =P

Anywho, my new rule is that every time we waste more than an hour because of a byond bug, i bump the bug report.
Sep 11 2017, 9:35 am
This is something I've been meaning to look at in the guts of the compiler, since I'm in better shape to do that now. It's just taken a low priority.

Internally, all default-value declarations for arguments compile into code that does this:

if(arg == null) arg = default_value

Something is going wrong with this statement when it's a src var. A foolproof workaround of course is to include the code explicitly rather than in the argument definition.
Sep 11 2017, 10:30 am
Now turned out to be a good time to look closer into that, and I believe I've solved the problem.

The compiler and the interpreter use an internal "loc" var that refers to the last object that had a var or proc access--be it src, usr, another var, or whatever. On the compiler side, it's working with absolute var paths, so it's not actually working based on the value of the vars but based on whether they might be different.

Repeated statements like src.a=1, src.b=2, etc. are actually compiled so that src is only looked up once, and it goes in the internal loc var after that. Therefore the first case is encoded as src.a, but the second is just "b" by name. The compiler knows this, but it also knows it needs to look out for special cases where it's possible that the loc var could have changed, so that it doesn't try to do this shortcut.

Well what happened here was that the routine responsible for spitting out the if() block for default arguments did not call an internal routine called LastSrcModified(), which tells it not to rely on the last loc. A regular if/else block would have done this, but the quick-and-dirty one output by this argument init did not. Thus, if the next var access isn't one that changes its source, it gets encoded in the shortcut form. And when you go to run the proc, if that if() block is skipped because you supplied a non-null value for the argument, then the value of the internal loc var is not what the compiler expected it to be.

This explains why so many things caused the issue to "reset" and not happen, and it also means that multiple args evaluated this way in the same proc would have exacerbated the problem amongst each other.
Sep 25 2017, 12:26 pm
Lummox JR resolved issue with message:
Using a default proc argument value such as arg=src.var sometimes caused strange error messages, such as "cannot read null.var".