Pin down return value on failure

[qaisjp]

Recently there seems to be some confusion about what values functions should return when they encounter an error.

Historically, the following has been the general policy; (1):

• On failure, all functions should return false.

Somewhere along the way we realised functions that ordinarily return booleans (isPedDead, hasObjectPermissionTo, etc) should not return meaningful values (false) when errors are encountered, so we started doing something else for new functions; (2):

• If a successful function call would return a boolean value, a failed call should return nil.
• Otherwise, all other failed function calls return false.

Some new functions / pull requests have started to do something else; (3):

• Always return nil on failure, no matter the return type.

Things to pin down:

• Pick a system
  1. Stick to the current system (2)
  2. Begin adopting (3)?
  3. Something else?
• Do we want to modify old functions or should this only apply to new functions?

[qaisjp]

Okay, so my current POV is that (3) is a good way to go. I think it's nice because it's consistent for every function.

Nevertheless, I have been asking for PRs to stick to (2) since that's what is currently done.

I am unsure as to whether existing functions should change. I imagine a lot of existing code does if result == false instead of if not result.

[botder]

What do you think about this one?

mtasa-blue/Server/mods/deathmatch/logic/luadefs/CLuaACLDefs.cpp

Lines 890 to 891 in a547e4b

	int CLuaACLDefs::hasObjectPermissionTo(lua_State* luaVM)
	{

...

mtasa-blue/Server/mods/deathmatch/logic/luadefs/CLuaACLDefs.cpp

Lines 963 to 966 in a547e4b

	// Failed
	lua_pushnil(luaVM);
	return 1;
	}

I prefer to return nil on failure, no matter the return type (3).
I am not sure if changing existing functions, to return nil, would break scripts.

[qaisjp]

hasObjectPermissionTo is a getter that returns boolean values, so fits into (2).

[botder]

It's not actually a getter, because by definition, it doesn't simply return a private member variable of the player/resource/..., which gets passed to the function. It has to lookup the permission in the ACL for the object and then returns a boolean.

[qaisjp]

(2) isn't restricted to getters in that traditional sense, it is more about false having a special meaning.

So because false has a special meaning in hasObjectPermissionTo, (i.e. "no, this object doesn't have permission"), it fits into (2).

I've edited this issue to reflect this.

In fact, hasObjectPermissionTo was ahead of its time, doing (2) before we noticed we needed to do (2).

[sbx320]

I'd prefer another option: Hard error on usage mistakes. hasObjectPermissionTo(false) is obviously wrong and needs to be fixed at the call site. Relying on results from that call is also fairly pointless and regularly causes trouble in other parts of a script, for example by adding results to a table, which then incorrectly contains false instead of strings. This would also be more consistent with built-in Lua functions.

For new functions this could be applied easily. Applying this to old functions is a bit tricky since it could break badly written scripts and we'd be willing to break backwards compat (yet another reason for 1.6 ;)) with (wrong) code like setElementPosition(vehicle, "potato") setElementHealth(vehicle, 1000), which would currently throw a warning, but still set the health to 1000. With a hard error, setElementHealth would not be executed.

[qaisjp]

I would be in support of just erroring. Are there any other situations where we might want to use warnings, or would this eradicate the use of warnings completely? (Ignoring existing functions.)

[botder]

We could also add a "strict" mode, which could, if enabled, throw an error for already existing functions.

[qaisjp]

Okay, so let's hard error on usage mistakes, as per #821 (comment).

Let's also do something like a strict mode (#821 (comment))... how do we want to implement this?

Potential ways would be:

• per-resource meta <strict>true</strict>
• server-wide
• some others

[botder]

We could add server-wide, per-resource and per-script. Everyone would be happy.

[qaisjp]

Haha that's a joke right?

[botder]

Yes, only server-wide and per-resource is enough.

[forkerer]

It's been 3 months, has there been any progress with this? It's blocking a few pull request and doesn't seem like it's going to change soon

[qaisjp]

@sbx320 has been working on this — am I fine to assign this to you?

[qaisjp]

To clarify the functionality mentioned here is blocking pull requests:

Okay, so let's hard error on usage mistakes, as per #821 (comment).

"Strict mode" can be discussed and added later as a separate issue/feature.

[ghost]

I think what @sbx320 suggested is enough. Throwing hard error on usage mistake is good. The strict mode looks nice, but it might break scripts, and I can only imagine boomers using it.

[sbx320]

Current state: Stuff works, I've gotten stuck on some performance isuse, but that is resolved now. Only missing part now is to handle errors properly (right now the errors are handled, but the messages are empty). Plus a whole bunch of testing obviously.

[botder]

Okay, so let's hard error on usage mistakes, as per #821 (comment).

Let's also do something like a strict mode (#821 (comment))... how do we want to implement this?

Potential ways would be:

• per-resource meta <strict>true</strict>
• server-wide
• some others

I am going to close this issue now. See quote for final decision.

Before sbx320's pull request is done, the following code should be fine:

if (argStream.HasErrors())
{
    return luaL_error(luaVM, argStream.GetFullErrorMessage());
}

(Docs: http://pgl.yoyo.org/luai/i/luaL_error)

[qaisjp]

Sounds good!

[ccw808]

What about gathering stats to measure the impact of causing hard errors. i.e. Servers and clients upload info about usage mistakes

[AlexTMjugador]

I can't find any downside to gathering more anonymized information with consent of the server owners in order to make a better decision 👍