fix.md

Natives

A native function is one in the server itself, either from SA:MP or a plugin. IsPlayerConnected is a SA:MP native, sscanf is a plugin native. Similarly, ShowPlayerDialog is a SA:MP native; calling it from your script will invoke some behaviour in the server. A native is normally declared as:

native IsPlayerConnected(playerid);
native ShowPlayerDialog(playerid, dialogid, style, caption[], info[], button1[], button2[]);

You can also set an internal and external name for a native:

native SCM(playerid, color, const message[]) = SendClientMessage;

(If you do use this example, I'll kick you). This defines the function SCM to be used in the script. SCM(playerid, COLOUR_RED, "hello"); will work, but will call the function called SendClientMessage in the server. This is slightly different to using #define for two reasons:

native SendClientMessage(playerid, color, const message[]);
#define SCM SendClientMessage

This defines a native called SendClientMessage and a text replacement called SCM. So any time SCM is seen it is replaced by SendClientMessage. This means that both SCM(playerid, COLOUR_RED, "hello"); and SendClientMessage(playerid, COLOUR_RED, "hello"); will work. In the first example SendClientMessage will NOT work - it is only the external (outside the script) name. The second reason is that this is a high-level replacement, not a text replacement. You can create many different versions of the same native:

native CreateObjectClose(modelid, Float:X, Float:Y, Float:Z, Float:rX, Float:rY, Float:rZ, Float:DrawDistance = 100.0) = CreateObject;
native CreateObjectMedium(modelid, Float:X, Float:Y, Float:Z, Float:rX, Float:rY, Float:rZ, Float:DrawDistance = 500.0) = CreateObject;
native CreateObjectFar(modelid, Float:X, Float:Y, Float:Z, Float:rX, Float:rY, Float:rZ, Float:DrawDistance = 1000.0) = CreateObject;

I changed the defaults of the last parameter there, just to show that you can. A good example of this is in pawn-plus, which uses native refinitions to declare string reference natives as just:

native print_s(String:s) = print;

An alternative instance is in YSI, which has a custom version of memcpy that takes a pointer instead of an array:

native rawMemcpy_(dest, src, index, numbytes, maxlength) = memcpy;

As opposed to:

native memcpy(dest[], const source[], index = 0, numbytes, maxlength = sizeof (dest));

Const-Correctness

There are two good articles on this already, but in short, some default natives are slightly wrong. They should take const strings, which tells the compiler that they don't modify their parameter. For example, this:

native SetTimer(funcname[], interval, repeating);

Should actually be:

native SetTimer(const funcname[], interval, repeating);

The old compiler didn't mind, the new one does. As a result, to avoid warnings, YSI also declares its own copies of some of these functions that it needs, to ensure that it always has the best version. Both the updated samp-stdlib and fixes.inc fix these declarations, but YSI is designed to work without either of those, and so throughout the code are several const-correct natives:

native YSI_Print(const string[]) = print;
native YSI_PrintF(const format[], GLOBAL_TAG_TYPES:...) = printf;

native YSI_SetTimer(const funcname[], interval, repeating) = SetTimer;
native YSI_SetTimerEx(const funcname[], interval, repeating, const format[], GLOBAL_TAG_TYPES:...) = SetTimerEx;

Hooking

Hooking is the term used to describe extending/fixing/enhancing another function (usually callbacks, but not always). The raw way to do this is ALS, where you write a new function to do some extra work, (optionally) call the original, then use the pre-processor to redirect future calls to the original function to your new function:

Hooked_SetTeamCount(count)
{
	printf("Called `SetTeamCount(%d);`", count);
	return SetTeamCount(count);
}

#define SetTeamCount Hooked_SetTeamCount

There are some other defines used to deal with multiple hooks, but they're not relevant here. So when this code, later on, is compiled:

public OnGameModeInit()
{
	SetTeamCount(5);
}

The pre-processor replaces this BEFORE the main compilation with (you can view this replacement output with -l):

public OnGameModeInit()
{
	Hooked_SetTeamCount(5);
}

The Bug

So now we have all the parts of the bug in place, and maybe you've already spotted the issue. We have two libraries, one of them (y_dialogs) hooks ShowPlayerDialog to extend it, the other one (y_testing) declares is own version of ShowPlayerDialog and uses that. If it just called ShowPlayerDialog() normally there'd be no problem, because that's the case that ALS is designed for. Instead, spread over multiple files, we end up with something like:

// The hook.
Dialogs_ShowPlayerDialog(playerid, dialogid, style, const caption[], const info[], const button1[], const button2[])
{
	return ShowPlayerDialog(playerid, dialogid, style, caption, info, button1, button2);
}
#define ShowPlayerDialog Dialogs_ShowPlayerDialog

// The redeclaration.
native Testing_ShowPlayerDialog(playerid, dialogid, style, const caption[], const info[], const button1[], const button2[]) = ShowPlayerDialog;

Testing_DoSomething(playerid)
{
	Testing_ShowPlayerDialog(playerid, 42, 1, "Hello", "world", "I'm", "wrong");
}

Compiling this with -l again, we see what the compiler is trying to compile:

Dialogs_ShowPlayerDialog(playerid, dialogid, style, const caption[], const info[], const button1[], const button2[])
{
	return ShowPlayerDialog(playerid, dialogid, style, caption, info, button1, button2);
}

native Testing_ShowPlayerDialog(playerid, dialogid, style, const caption[], const info[], const button1[], const button2[]) = Dialogs_ShowPlayerDialog;

Testing_DoSomething(playerid)
{
	Testing_ShowPlayerDialog(playerid, 42, 1, "Hello", "world", "I'm", "wrong");
}

We now have two DIFFERENT versions of Dialogs_ShowPlayerDialog. One is a pawn function defined in the mode, the other is a native in the server. At least, that's what the compiler thinks, because the #define has replaced the native external name. This is part of the reason why external names exist - so you can have a native with the same name as a pawn function without issues, as long as you rename the native in this way. So now, when Testing_ShowPlayerDialog is called the script attempts to call the native Dialogs_ShowPlayerDialog, which doesn't exist (actually, this is checked when the script starts, before any code is run).

Your Solution.

You suggested changing:

native Testing_ShowPlayerDialog(playerid, dialogid, style, const caption[], const info[], const button1[], const button2[]) = Dialogs_ShowPlayerDialog;

To:

#define Testing_ShowPlayerDialog ShowPlayerDialog

This almost works. If y_dialogs is included first Testing_ShowPlayerDialog will become ShowPlayerDialog which will in turn become Dialogs_ShowPlayerDialog and call the library hooked version. However, if y_dialogs is not included, Testing_ShowPlayerDialog will become ShowPlayerDialog, which might be the const-incorrect version from the default SA:MP includes, the version we were trying to avoid in the first place.

The Solution

The problem is that #define ShowPlayerDialog Dialogs_ShowPlayerDialog replaces ALL occurrences of ShowPlayerDialog, whereas we only want it to replace function calls, not native external names. For this, we need to use the pattern matching of defines. They will only do the replacement if the whole thing matches. To do this we add something more to the pattern, something that will match functions, but not natives:

#define ShowPlayerDialog(%0) Dialogs_ShowPlayerDialog(%0)

Now, ShowPlayerDialog has to look like a function call to be replaced. - ShowPlayerDialog; doesn't look like a function call, and won't be replaced. Sadly, from a macro point of view, this doesn't look like a function call either, and while previously was replaced, it now isn't:

ShowPlayerDialog(
	playerid,
	42,
	1,
	"No",
	"longer",
	"calls",
	"Dialogs_ShowPlayerDialog"
);

We need a pattern that still matches this, but not natives:

#define ShowPlayerDialog( Dialogs_ShowPlayerDialog(

It still won't work if you put the open bracket on a new line, but if you do that you've only yourself to blame!

This is the one line fix, but I also removed the duplicated natives to a common location, hence the bigger fix here:

https://gist.github.com/Y-Less/505de60f06bd3ef1b7c629c534eafcb7

Natives

Wanted to experiment for myself, so I set up the following "test bench":

#include <a_samp>

native MyCustomDisplay(const format[], {Float, _}:...) = printf;

main()
{
    printf("My custom %s", "message");
    MyCustomDisplay("Today is the %dth of December", 14);
}

From a quick read of your post, this shouldn't work? Perhaps I am misinterpreting.
The .lst file contains the following, amongst other things:

// native decl for printf, coming from console.inc of pawn-stdlib
native printf(const format[], {Float,_}:...);

// ...

// test bench itself
native MyCustomDisplay(const format[], {Float, _}:...) = printf;

main()
{
    printf("My custom %s", "message");
    MyCustomDisplay("Today is the %dth of December", 14);
}

This seems like a bad test bench since every time printf native will be declared and accessible to the code bit using it.
Let's have MyCustomDisplay point at some invalid native foo, and see what happens.

.lst output is as follows

native MyCustomDisplay(const format[], {Float, _}:...) = foo;

main()
{
    printf("My custom %s", "message");
    MyCustomDisplay("Today is the %dth of December", 14);
}

• MyCustomDisplay continues to stay "as is", which tells us that the native itself does not impact the .lst file.
  The .lst file seems to be the file containing the "post-preprocessor" contents that are fed into the compiler; it makes sense provided the below reasoning is correct.
• Code generated the .lst fine (as expected) but compiled, which came as a surprise initially.
  Wouldn't the compiler know that foo is not a native it has found within it's input (lst output)? My first guess would be to say no. This .amx may be interacting with other .amx that supply the functionality (native). Since it's on another .amx, it wouldn't be on the input for the compiler for this .amx; thus it is not enough for the compiler to detect that it cannot find the definition for a native, but rather it must take the programmer's claim that foo exists "elsewhere", and leave that promise to be checked at runtime, where it will try to call the function. Here I think that CallLocalFunction and CallRemoteFunction come into play. Would the runtime know whether to use the local or remote variant to try to access the function? One would think that at compile time, if it finds it within this script (that results in the .amx) internally, it would set some type of flag or something so that the runtime doesn't perform the more expensive remote variant (I'm claiming it's more expensive from what seems like a logical assumption to make - having to talk to another external script to then fetch something seems to be more work than just fetching that something from within yourself). When the runtime comes in, it will check for the foo promise, as it tries to call the MyCustomDisplay internal function. Here foo would be external since we can see that foo is not present in this script, but if it were another native within the script (such as the first printf example), would we still refer to the rhs of the assignment as external? For example:

native MyCustomDisplay(const format[], {Float, _}:...) = foo;
native AnotherCustomDisplay(const format[], {Float, _}:...) = MyCustomDisplay

I'd be tempted to say that in both scenarios, we still refer to MyCustomDisplay as an internal function.

Nonetheless, the issue with a non-found external function arises during runtime, which makes sense. The issue that the compiler will keep this promise, trusting the user to call a correct external function, is troublesome as it can cause these runtime errors - more likely than not this is why features like y_remote and so on exist - essentially to provide safe wrappers around local and remote function call variants. I've understood the concept of external/internal, I think, but the "SendClientMessage will NOT work" confused me. Perhaps it'd be applicable when more than one script is at play? IE when from the "current" script's POV SendClientMessage really is just external and the current script is unaware of it?

Const Correctness

I was aware of the const correctness issue, but I hadn't thought that the native definitions were used to circumvent it. It makes sense when you explained it like this, as it allows to "force" correct constness from the incorrect "vanilla SAMP" natives. Also makes sense that YSI would continue to use its own as to be independent from samp-stdlib and fixes.

Hooking

Here I can spend some time as well really grasping this. Preprocessor should come into play here as there are some text replacements going on. Let's dive in.
Originally, I've tried to have the example here be:

#include <a_samp>

Hooked_MainFunction()
{
    print("This is hooked main!");
    return main();
}

#define main Hooked_MainFunction

//native MyCustomDisplay(const format[], {Float, _}:...) = printf;

main()
{
    printf("My custom %s", "message");
    //MyCustomDisplay("Today is the %dth of December", 14);
}

However, it doesn't work as it replaces the definition of the original main with Hooked_MainFunction, resulting in the function being defined twice and thus an error.
If main is defined before the hook and redefine, what happens? .lst doesn't show replaced main def since the replacement happens afterwards, and the hook still returns to it correctly. Now, it compiles correctly, but with a warning "symbol is never used: "Hooked_MainFunction"". However, it does not print the expected "This is hooked main!" message when ran. I'm unaware of how YSI goes around the issue I think I'm facing that is the fact that main itself is called from outside (?) the script to access it, thus the replacement is effectively useless, and hence why it doesn't happen. Let's use another example, one with CreateVehicle. Test bench is now:

#include <a_samp>

Hooked_CreateVehicle(modelid, Float:x, Float:y, Float:z, Float:rotation, color1, color2, respawn_delay, addsiren=0)
{
    printf("Called `CreateVehicle` for model %d", modelid);
    return CreateVehicle(modelid, x, y, z, rotation, color1, color2, respawn_delay, addsiren);
}

#define CreateVehicle Hooked_CreateVehicle

main()
{
    CreateVehicle(411, 0.0, 0.0, 0.0, 0.0, 0, 0, 0, 0);
}

This one works as expected. By having the hook definition call the original first, then followed by the text replacement, all subsequent calls (and any other instances of the text 'CreateVehicle') are replaced with the text for the hooked function, thus effectively "swapping" the name of the original calls with the hooked one (with those aforementioned side effects of any other instances of the original function name). If the side effects are important for functionality (such as being the external function to an internal native declaration), the simple text replacement would not work as it would replace the external symbol too.

Bug

y_dialog_impl hooks ShowPlayerDialog via Dialog_ShowPlayerDialog. y_testing_entry defines an internal function YSI_ShowPlayerDialog (for itself) with ShowPlayerDialog as the external function.
If there's no dialog, only testing, then YSI_ShowPlayerDialog will just "piggy back" from samp's ShowPlayerDialog, and all is fine.
If there's no testing, only dialog, then ShowPlayerDialog will be hooked via Dialog_ShowPlayerDialog, and all is fine.
However, if both are combined (taking into account order of dialog first then testing), ShowPlayerDialog is first hooked through Dialog_ShowPlayerDialog. Thus, any later attempts (including at y_testing_entry) when they refer to ShowPlayerDialog they're actually referring to the hooked variant Dialog_ShowPlayerDialog. So, the native definition in y_testing_entry ends up pointing to the text replacement, which is not a valid native of SAMP, thus causing the runtime error of function not found.

My solution

That's why my solution won't work, because it doesn't take into account the importance of the rhs to the native definition in y_teting_entry to NOT be altered, but the hook still to work. So, need to have a way to replace only the function calls to the now hooked function.

Solution

First inclination then would be to do a C-style macro accounting for the parameter to the function, but with PAWN syntax obviously:

#define ShowPlayerDialog(%0) Dialog_ShowPlayerDialog(%0)

The problem with this is that it tries to force the call syntax of being all in the same line. If I'm not mistaken, here's the result

// this call will be replaced
ShowPlayerDialog(playerid, .... );
// however this one will not
ShowPlayerDialog(playerid,
                ...,
                )

Let's confirm this to be the case by implementing a simpler test bench to confirm the behavior:

#include <a_samp>

Hooked_CreateVehicle(modelid, Float:x, Float:y, Float:z, Float:rotation, color1, color2, respawn_delay, addsiren=0)
{
    printf("Called `CreateVehicle` for model %d", modelid);
    return CreateVehicle(modelid, x, y, z, rotation, color1, color2, respawn_delay, addsiren);
}

#define CreateVehicle(%0) Hooked_CreateVehicle(%0)

main()
{
    // this should work (be replaced)
    CreateVehicle(411, 0.0, 0.0, 0.0, 0.0, 0, 0, 0, 0);
    // this should not
    CreateVehicle(412, 
                    0.0, 0.0, 0.0, 0.0, 0, 0, 0, 0);
}

.lst output confirms that the claim is valid:

Hooked_CreateVehicle(modelid, Float:x, Float:y, Float:z, Float:rotation, color1, color2, respawn_delay, addsiren=0)
{
    printf("Called `CreateVehicle` for model %d", modelid);
    return CreateVehicle(modelid, x, y, z, rotation, color1, color2, respawn_delay, addsiren);
}

#line 69

main()
{

    Hooked_CreateVehicle(411, 0.0, 0.0, 0.0, 0.0, 0, 0, 0, 0);

    CreateVehicle(412, 
        0.0, 0.0, 0.0, 0.0, 0, 0, 0, 0);

}

So, the original problem remains. Need to support function calls, but not enforce their call syntax.
A function call, or at least the important part, is only of the form <name>( - nothing else. So, adjust the macro to work that way: #define CreateVehicle( Hooked_CreateVehicle(. A quick run after the adjustment has the .lst output confirming that the second variant is now hooked as well. As you've noted, it will not work if the function call is of the form

CreateVehicle
(
...

but since this is all internal code, and not exposed to an end user directly, there is no issue as long as the function calls have the opening parenthesis on the same line (ie matches the "function call pattern").

Longer fix

This fixes the issue for these two, but what if another component of YSI wants to access the ShowPlayerDialog functionality as well? It's easiest to export this native to a common place where everyone can use it if necessary. That place exists, and is within the core section of YSI. Specifically, since ShowPlayerDialog is a SAMP native, it's appropriate location is YSI_Core\y_core\y_samp_natives.inc. Thus, remove the native from y_testing_entry and place it there. That, alongside with the improved text replacement for function calls on y_dialog_impl, should address the problem entirely.