Wire: Fix E2 functions to return default (non-nil) value

[CaptainPRICE]

Example:
entity:owner(): Go to source.
It is just one of them, there are more "broken" e2functions.

edit (new):
In order to elaborate on this issue, here is a simple example Expression 2 (source):

local NULL = noentity()
print("NULL:", NULL)                     # NULL:    [NULL Entity]
print("nil:", NULL:owner())              # nil: 
print("Equality:", NULL == NULL:owner()) # Equality:    0

local Array = array(
    (1) = NULL,
    (2) = NULL:owner()
)
print("Array Exists:\t1=" + Array:exists(1) + ", 2=" + Array:exists(2)) # Array Exists: 1=1, 2=0
print("Array Count:", Array:count())                                    # Array Count:  1

Output:

NULL:   [NULL Entity]
nil:    
Equality:    0
Array Exists:   1=1, 2=0
Array Count:    1

You can find old example here.

This is because Expression 2 does not have nil value type. Currently, it is inconsistent and thus such e2functions have to be fixed by returning a non-nil (default) value for its return type (i.e. if e2function returns an entity type, then it should return NULL instead of nil value).

I am working on it, you can find the pull request here.

[Divran]

pretty sure 95% of all functions in E2 that return entities will return nil instead of null when they fail. Also pretty sure that it works with either one, so it doesn't matter.

[bigdogmat]

It currently only works because all(?) entity methods currently check the entity using IsValid. This is actually a bit of a problem because this code for example

local Arr = array(
    1,
    2,
    noentity(),
    noentity():owner() # returns nil thus doesn't actually count in the array
)
print(Arr:count())

will print 3 instead of 4

[Divran]

yep, that's correct. Just saying, if you change one function from nil to null, that's not going to change much. Might as well change all of them in that case.

[CaptainPRICE]

Okay, scheduling this for later.

[TomyLobo]

Working as intended.

[TomyLobo]

@bigdogmat you shouldn't rely on R:count() anyway, since that is based on Lua's "#" operator.

[bigdogmat]

And? It's representing what should be a NULL entity as nil, thus breaking it when it shouldn't. How is this in any way working as intended?

[TomyLobo]

http://ideone.com/NZTt4m
My point is you should not rely on that operator in any way, shape or form, because it's so easy to break.
If you want to count elements, iterate and count. I think there's a function for that too.

[bigdogmat]

But it doesn't break so easily, it breaks if you have nil holes in your table. A NULL entity doesn't count as nil.

noentity()
noentity():owner()

You'd expect both of the above to count as elements in an array because the return type of both is entity, but that's not the case considering the second one returns nil, not NULL.

Edit:

If you want to count elements, iterate and count. I think there's a function for that too.

There is not, also you can't even iterate over it to get the count because it's represented as nil, thus foreach will not iterate over it. Only way would be to store the length of it somewhere else.

[CaptainPRICE]

Working as intended.

Tomy, Tomy, Tomy... nil != NULL. See what @bigdogmat just said.

[TomyLobo]

So you want invalid elements to count now?
That would indeed be a change in behaviour that will break people's code...

[TomyLobo]

Talking about breaking shit: have you ever considered 3rd party addons that might return nil as an entity from an E2 function?

So even if, as you suggest, we change all E2 functions to return NULL instead of nil, we still cannot remove the checks.
All we will have achieved is that we're now using an object that is heavier to handle.

[CaptainPRICE]

So you want invalid elements to count now?

Of course, they should be counted over in a loop!
Just like GLua does count over them, however that's not true for nil value of course, (G)Lua can't iterate through a nil value. And that is the problem... And also, there is no other function in E2 that will count nil values, because as I said it, (G)Lua can't iterate through them because they are nil values, they should be of Entity type instead (NULL is of Entity type, which represents the "invalid" entity). Thus nil is not the same as NULL, nil == NULL it will always evaluate to false.

That would indeed be a change in behaviour that will break people's code

Yeah, that is true, but at least it will not break the majority of the code.
Accept it since is already broken anyway, so its time to fix it and sometimes fixing it means breaking code, unfortunately it may only break the code which works with invalid entities and array/table (such as the one @bigdogmat had posted above).

have you ever considered 3rd party addons that might return nil as an entity from an E2 function

Yeah, but honestly that would be their problem then. Wire has IsValid checks (which returns false if you pass nil or "invalid" entity to it; We described it above). If you were to remove IsValid checks from Wire, it would cause a nil value errors instead of throwing NULL entity errors, which is completely misleading as it will introduce confusion then, because E2 doesn't have a nil value type in the first place..

So even if, as you suggest, we change all E2 functions to return NULL instead of nil, we still cannot remove the checks.

Wait-what. 😶 Why would you remove any of the checks?!?!
You should NOT remove any checks of the checks, just make sure that all e2functions never return a nil value that's all.

[bigdogmat]

So you want invalid elements to count now?

NULL is not an invalid element, it's used to represent entities that are invalid or have gone invalid (e.g. removed). nil is not NULL, nil is what Lua uses to say there's no value here.

Like in GLua, NULL counts as an element in an array. The problem is half of the entity functions return nil instead of NULL when they fail.

noentity()
noentity():owner()

Both of the above are defined as returning an entity in E2, however the second one returns nil instead of NULL. If you don't see the logically inconsistency here then I don't know how else to show it to you.

That would indeed be a change in behaviour that will break people's code...

If anything this would fix people's code. Hell in my case it'd fix code considering I assumed this to work correctly anyway. Likewise this is an edge case of where it matters, so I doubt it'll affect many people.

Talking about breaking shit: have you ever considered 3rd party addons that might return nil as an entity from an E2 function?

So even if, as you suggest, we change all E2 functions to return NULL instead of nil, we still cannot remove the checks.

I never suggested removing the checks, and yes it's still a problem with 3rd party addons. Just because it's a problem with 3rd party addons doesn't mean we can fix what's broken here?

All we will have achieved is that we're now using an object that is heavier to handle.

How is it heavier to handle?

[oldmud0]

How is it heavier to handle?

I think he's referring to the fact that NULL counts as a full-blown object whereas nil is a "zero-byte" object.

[bigdogmat]

I think he's referring to the fact that NULL counts as a full-blown object whereas nil is a "zero-byte" object.

Ah, but even then, NULL is a constant so the amount it'd take up here would be negligible

[CaptainPRICE]

I think he's referring to the fact that NULL counts as a full-blown object whereas nil is a "zero-byte" object.

That's just ridicules if that is what he's referring to.
Why would you even compare what is "heavier" when it is not even functional in the first place..

In my opinion, there are no doubts, this is a bug and it is a HAVE-TO fix.

[CaptainPRICE]

@TomyLobo Here you go:
nil-null-test.lua

I have also updated OP, added Expression 2 example.

[TomyLobo]

@CaptainPRICE actually managed to make a valid argument for this change on Steam so I'm reopening this.

I trust that he's going to detail it in a comment or the OP.

[bigdogmat]

I don't know of an argument that should have to be made past nil and NULL being treated the same. Nonetheless at least it's reopened.

[adosikas]

@CaptainPRICE i don't think your "simple" E2 is helping with demonstrating the issue (2 functions, 3 timers, non-essential code, propcore). And i wouldn't call running error() a crash, especially not when trying to explain a issue that might cause an actual error or crash.

For comparison this is simple and to the point (feel free to replace yours in the top comment with it):

print("NULL:", noentity()) # NULL:  [NULL Entity]   
print("nil:", noentity():owner()) # nil:    
print("equal:", noentity()==noentity():owner()) # equal:    0

local Array=array(noentity(), noentity():owner())
print("Exists: 1:", Array:exists(1), "2:", Array:exists(2)) # Exists: 1:    1   2:  0
#Actually not sure if both should return 0 or 1 here, but it shouldn't be different

[CaptainPRICE]

Updated Expression 2 code example to simplify the demonstration of this issue.

@adosikas Thanks for your contribution.

[CaptainPRICE]

I have also came across a few e2functions which returns a completely different value, for example, they return a string value instead of a number (as specified by return type)... (if i could guess, it is copy+paste error) 😆
Found another one, it mixes boolean with number value... Another one mixes nil and number... Another one returning GLua Vector instead of E2 vector (table) value... just wow.
I will be fixing those as well!

[Divran]

Links to these other ones please

[CaptainPRICE]

Links to these other ones please

Oh, well you will see them, I am committing this nicely, it is all locally at the moment, fixed over 50 functions (with invalid return value, and nil and sht), 50+ commits...
So, it should *not be a problem for you to follow when I finally push it to PR :)

[bigdogmat]

Ok, so looking into this more I've found more inconsistencies.

For example, you can do

Arr = array(noentity())

And Arr will contain a NULL entity. However

Arr[1, entity] = noentity()

Won't put anything in Arr because the setter function for arrays checks the type of value using the types type checker. Which for entities also checks if it's valid. This ties into #1350 and gives us another reason to not check if the entity is valid in the type checker.

Guess this doesn't directly tie into this issue, however it's another inconsistency when dealing with NULL entities.

[CaptainPRICE]

Which for entities also checks if it's valid. [...] gives us another reason to not check if the entity is valid in the type checker. [...] however it's another inconsistency when dealing with NULL entities.

Yeah, I agree.
"Type checker" should only check if the value's type is correct such as if type(value) returns Entity (or either use isentity GLua function), but it should not check if the Entity object itself is also valid. Type checker's job should not reject invalid entities (NULL)!
Because since it does that, it results in inconsistent behavior. That is obviously a bug in "type checker".

Guess this doesn't directly tie into this issue

These seems to be related in a way, but "type checker inconsistency" is a whole new a discussion for a new issue (Go ahead and create it, unless it is already there. I am too busy fixing this issue here.)

[CaptainPRICE]

Dead. I do still have a backup of the repository on my old hard drive tho.. I don't know if I have enough motivation to ditch this. RIP...