Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add stronger tag checks to streamer.inc #435

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Y-Less
Copy link
Contributor

@Y-Less Y-Less commented Aug 7, 2022

Change List

This makes several changes to tags:

  1. Everything that accepts a boolean value (only 0 or 1) now have bool: tags, possibly defaulting to true or false.
  2. Everything that returns a boolean (Is/Has functions) now have bool: tags.
  3. Several other functions that return success/fail also have bool: tags, but fewer are done.
  4. The STRONG_TAGS/WEAK_TAGS/NO_TAGS defines have been adopted from samp-stdlib.
  5. STREAMER_MAX_TYPES and the type definitions have been converted to a proper enum.
  6. STREAMER_MAX_AREA_TYPES and the type definitions have been converted to a proper enum.
  7. STREAMER_MAX_OBJECT_TYPES and the type definitions have been converted to a proper enum.
  8. OBJECT_MATERIAL_SIZE:, MAPICON:, OBJECT_MATERIAL_TEXT_ALIGN:, and CP_TYPE: optional tags support added.

New Warnings

These changes may or may not affect existing code, depending on how the code was written.

Code already using the defines from the include should not be affected:

// This will now give a tag mismatch warning on the first parameter:
Streamer_GetFloatData(0, objectid, E_STREAMER_R_X, dest);

// This will continue to work correctly as before in all cases:
Streamer_GetFloatData(STREAMER_TYPE_OBJECT, objectid, E_STREAMER_R_X, dest);

When using the tag overhauled samp/open.mp includes some functions now require the existing defines:

// This will now give a tag mismatch warning on the last parameter only if using the enhanced SA:MP includes:
SetDynamicObjectMaterialText(objectid, materialindex, "text", .textalignment = 0);

// This will continue to work correctly as before in all cases:
SetDynamicObjectMaterialText(objectid, materialindex, "text", .textalignment = OBJECT_MATERIAL_TEXT_ALIGN_LEFT);

Additionally some functions have new defines within the enhanced includes:

// This will now give a tag mismatch warning on the first parameter only if using the enhanced SA:MP includes:
CreateDynamicRaceCP(3, 100.0, 134.0, 77.0, 145.0, 167.0, 83.0, 5.0);

// The new code in that case would look like:
CreateDynamicRaceCP(CP_TYPE_AIR_NORMAL, 100.0, 134.0, 77.0, 145.0, 167.0, 83.0, 5.0);

Possibly the most intrusive change is the boolean parameters:

// This will now give a tag mismatch warning on the final parameter:
Streamer_ToggleAllItems(playerid, STREAMER_TYPE_AREA, 1);

// The previous code was idiomatic, but should now become:
Streamer_ToggleAllItems(playerid, STREAMER_TYPE_AREA, true);

While everything above gave warnings, there is one breaking change - i.e introduces errors. This is unfortunate because logically tag mismatches between forward declarations and function definitions should also be a warning not an error, I don't know why the compiler implements them as the latter (Edit: operator overloads will give codegen problems). Because the streamer type now has a tag, the callbacks that take a streamer type must also have that tag:

forward Streamer_OnItemStreamIn(STREAMER_TYPE:type, STREAMER_ALL_TAGS:id, forplayerid);
forward Streamer_OnItemStreamOut(STREAMER_TYPE:type, STREAMER_ALL_TAGS:id, forplayerid);

Which means the publics become:

public Streamer_OnItemStreamIn(STREAMER_TYPE:type, STREAMER_ALL_TAGS:id, forplayerid)
{
}

public Streamer_OnItemStreamOut(STREAMER_TYPE:type, STREAMER_ALL_TAGS:id, forplayerid)
{
}

Continuing to use the old version below will now give "function heading differs from prototype" errors:

public Streamer_OnItemStreamIn(type, STREAMER_ALL_TAGS:id, forplayerid)
{
}

public Streamer_OnItemStreamOut(type, STREAMER_ALL_TAGS:id, forplayerid)
{
}

Fortunately there is a way to write code that works with both the old and new declarations, and that's to define the tag as "no tag" when this new include version isn't used:

#if !defined STREAMER_TYPE
	#define STREAMER_TYPE: _:
#endif

public Streamer_OnItemStreamIn(STREAMER_TYPE:type, STREAMER_ALL_TAGS:id, forplayerid)
{
}

public Streamer_OnItemStreamOut(STREAMER_TYPE:type, STREAMER_ALL_TAGS:id, forplayerid)
{
}

When the STREAMER_TYPE: tag exists this code will use it. When it doesn't it won't.

Explanation

Warnings are good things:

// This code is probably incorrect, but there's no obvious way to tell:
Streamer_GetDistanceToItem(0.0, 1.0, 2.0, 5, areaid, distance);

// But after fixing tag mismatch warning, the mistake becomes clear:
Streamer_GetDistanceToItem(0.0, 1.0, 2.0, STREAMER_TYPE_3D_TEXT_LABEL, areaid, distance);
// This code is incorrect - global objects are type 0:
AttachDynamicAreaToObject(areaid, globalObjectID, .type = 1);

// This code is incorrect - player objects are type 1:
AttachDynamicAreaToObject(areaid, playerObjectID, .type = 2);

// This code is incorrect - dynamic objects are type 2 (3 doesn't exist):
AttachDynamicAreaToObject(areaid, dynamicObjectID, .type = 3);

Using the correct names will fix the bug instantly because they are right by definition

// This code is correct by definition:
AttachDynamicAreaToObject(areaid, globalObjectID, .type = STREAMER_OBJECT_TYPE_GLOBAL);

// This code is correct by definition:
AttachDynamicAreaToObject(areaid, playerObjectID, .type = STREAMER_OBJECT_TYPE_PLAYER);

// This code is correct by definition:
AttachDynamicAreaToObject(areaid, dynamicObjectID, .type = STREAMER_OBJECT_TYPE_DYNAMIC);

Or replacing the numbers with their exact names will show the problem instantly:

// The bug here becomes obvious from the symbol names:
AttachDynamicAreaToObject(areaid, globalObjectID, .type = STREAMER_OBJECT_TYPE_PLAYER);

// The bug here becomes obvious from the symbol names:
AttachDynamicAreaToObject(areaid, playerObjectID, .type = STREAMER_OBJECT_TYPE_DYNAMIC);

// This line can't even be completed because there's no define for object type 3:
AttachDynamicAreaToObject(areaid, dynamicObjectID, .type = ???);

Using strict typing strongly encourages people to use the named defines all the time to enable the compiler to help with hard to spot mistakes.

Configuration

For whatever reason some people hate warnings and would rather turn them off than actually use them to improve their code (I assume they think that no warnings is more important than no bugs). This despite the fact that warnings do not prevent compilation at all and can be ignored if you just squint a bit. Anyway, for the sake of those people, these new tags can be disabled.

There are actually three levels of warning enhancements:

WEAK_TAGS

This is the default and gives warnings when untagged values are passed to tagged values (e.g. passing 1 instead of STREAMER_TYPE_PICKUP). Basically everything documented above. So named because this is pawn weak tag semantics, achieved by prefixing all the new tags with a lower-case letter (t_).

// This is the default, to encourage adoption.
//#define WEAK_TAGS
#include <a_samp>
#include <streamer>

STRONG_TAGS

In addition to the weak tag warnings this adds warnings when tagged values are passed to untagged values. The only place this really matters in the streamer include is GetDynamicAreaType, which is currently the only function with a new tag type for its return value (I may have missed some others, feel free to point them out). So named because this is pawn strong tag semantics, achieved by prefixing all the new tags with an upper-case letter (T_).

Old code, no warning:

#include <a_samp>
#include <streamer>

new type = GetDynamicAreaType(areaid);

New weak code, no warning:

#define WEAK_TAGS
#include <a_samp>
#include <streamer>

new type = GetDynamicAreaType(areaid);

New strong code, tag mismatch warning:

#define STRONG_TAGS
#include <a_samp>
#include <streamer>

new type = GetDynamicAreaType(areaid);

New strong code, fixed (will also work in all other cases):

#define STRONG_TAGS
#include <a_samp>
#include <streamer>

new STREAMER_AREA_TYPE:type = GetDynamicAreaType(areaid);

NO_TAGS

Disables all the new tags (except bool: currently):

#define NO_TAGS
#include <a_samp>
#include <streamer>

// No warning.
new STREAMER_AREA_TYPE:type1 = GetDynamicAreaType(areaid);

// No warning.
new type2 = GetDynamicAreaType(areaid);

// No warning
CreateDynamicMapIcon(1043.0, -436.0, 5.0, 7, 0xFF0000FF, .style = 2);

// No warning
CreateDynamicMapIcon(1043.0, -436.0, 5.0, 7, 0xFF0000FF, .style = MAPICON_GLOBAL_CHECKPOINT);

See Also

https://github.com/pawn-lang/samp-stdlib/tree/consistency-overhaul#tags

Note that this branch has long been accepted for merge, but still isn't. It will be soon.

## Change List

This makes several changes to tags:

1. Everything that accepts a boolean value (only `0` or `1`) now have `bool:` tags, possibly defaulting to `true` or `false`.
2. Everything that returns a boolean (`Is`/`Has` functions) now have `bool:` tags.
3. Several other functions that return success/fail also have `bool:` tags, but fewer are done.
4. The `STONG_TAGS`/`WEAK_TAGS`/`NO_TAGS` defines have been adopted from samp-stdlib.
5. `STREAMER_MAX_TYPES` and the type definitions have been converted to a proper enum.
6. `STREAMER_MAX_AREA_TYPES` and the type definitions have been converted to a proper enum.
7. `STREAMER_MAX_OBJECT_TYPES` and the type definitions have been converted to a proper enum.
8. `OBJECT_MATERIAL_SIZE:`, `MAPICON:`, `OBJECT_MATERIAL_TEXT_ALIGN:`, and `CP_TYPE:` optional tags support added.

## New Warnings

These changes may or may not affect existing code, depending on how the code was written.

Code already using the defines from the include should not be affected:

```pawn
// This will now give a tag mismatch warning on the first parameter:
Streamer_GetFloatData(0, objectid, E_STREAMER_R_X, dest);

// This will continue to work correctly as before in all cases:
Streamer_GetFloatData(STREAMER_TYPE_OBJECT, objectid, E_STREAMER_R_X, dest);
```

When using the tag overhauled samp/open.mp includes some functions now require the existing defines:

```pawn
// This will now give a tag mismatch warning on the last parameter only if using the enhanced SA:MP includes:
SetDynamicObjectMaterialText(objectid, materialindex, "text", .textalignment = 0);

// This will continue to work correctly as before in all cases:
SetDynamicObjectMaterialText(objectid, materialindex, "text", .textalignment = OBJECT_MATERIAL_TEXT_ALIGN_LEFT);
```

Additionally some functions have new defines within the enhanced includes:

```pawn
// This will now give a tag mismatch warning on the first parameter only if using the enhanced SA:MP includes:
CreateDynamicRaceCP(3, 100.0, 134.0, 77.0, 145.0, 167.0, 83.0, 5.0);

// The new code in that case would look like:
CreateDynamicRaceCP(CP_TYPE_AIR_NORMAL, 100.0, 134.0, 77.0, 145.0, 167.0, 83.0, 5.0);
```

Possibly the most intrusive change is the boolean parameters:

```pawn
// This will now give a tag mismatch warning on the final parameter:
Streamer_ToggleAllItems(playerid, STREAMER_TYPE_AREA, 1);

// The previous code was idiomatic, but should now become:
Streamer_ToggleAllItems(playerid, STREAMER_TYPE_AREA, true);
```

While everything above gave warnings, there is one breaking change - i.e introduces errors.  This is unfortunate because logically tag mismatches between forward declarations and function definitions should also be a warning not an error, I don't know why the compiler implements them as the latter.  Because the streamer type now has a tag, the callbacks that take a streamer type must also have that tag:

```pawn
forward Streamer_OnItemStreamIn(STREAMER_TYPE:type, STREAMER_ALL_TAGS:id, forplayerid);
forward Streamer_OnItemStreamOut(STREAMER_TYPE:type, STREAMER_ALL_TAGS:id, forplayerid);
```

Which means the publics become:

```pawn
public Streamer_OnItemStreamIn(STREAMER_TYPE:type, STREAMER_ALL_TAGS:id, forplayerid)
{
}

public Streamer_OnItemStreamOut(STREAMER_TYPE:type, STREAMER_ALL_TAGS:id, forplayerid)
{
}
```

Continuing to use the old version below will now give "function heading differs from prototype" errors:

```pawn
public Streamer_OnItemStreamIn(type, STREAMER_ALL_TAGS:id, forplayerid)
{
}

public Streamer_OnItemStreamOut(type, STREAMER_ALL_TAGS:id, forplayerid)
{
}
```

Fortunately there is a way to write code that works with both the old and new declarations, and that's to define the tag as "no tag" when this new include version isn't used:

```pawn
#if !defined STREAMER_TYPE
	#define STREAMER_TYPE: _:
#endif

public Streamer_OnItemStreamIn(STREAMER_TYPE:type, STREAMER_ALL_TAGS:id, forplayerid)
{
}

public Streamer_OnItemStreamOut(STREAMER_TYPE:type, STREAMER_ALL_TAGS:id, forplayerid)
{
}
```

When the `STREAMER_TYPE:` tag exists this code will use it.  When it doesn't it won't.

## Explanation

Warnings are *good* things:

```pawn
// This code is probably incorrect, but there's no obvious way to tell:
Streamer_GetDistanceToItem(0.0, 1.0, 2.0, 5, areaid, distance);

// But after fixing tag mismatch warning, the mistake becomes clear:
Streamer_GetDistanceToItem(0.0, 1.0, 2.0, STREAMER_TYPE_3D_TEXT_LABEL, areaid, distance);
```

```pawn
// This code is incorrect - global objects are type 0:
AttachDynamicAreaToObject(areaid, globalObjectID, .type = 1);

// This code is incorrect - player objects are type 1:
AttachDynamicAreaToObject(areaid, playerObjectID, .type = 2);

// This code is incorrect - dynamic objects are type 2 (3 doesn't exist):
AttachDynamicAreaToObject(areaid, dynamicObjectID, .type = 3);
```

Using the correct names will fix the bug instantly because they are right by definition

```pawn
// This code is correct by definition:
AttachDynamicAreaToObject(areaid, globalObjectID, .type = STREAMER_OBJECT_TYPE_GLOBAL);

// This code is correct by definition:
AttachDynamicAreaToObject(areaid, playerObjectID, .type = STREAMER_OBJECT_TYPE_PLAYER);

// This code is correct by definition:
AttachDynamicAreaToObject(areaid, dynamicObjectID, .type = STREAMER_OBJECT_TYPE_DYNAMIC);
```

Or replacing the numbers with their exact names will show the problem instantly:

```pawn
// The bug here becomes obvious from the symbol names:
AttachDynamicAreaToObject(areaid, globalObjectID, .type = STREAMER_OBJECT_TYPE_PLAYER);

// The bug here becomes obvious from the symbol names:
AttachDynamicAreaToObject(areaid, playerObjectID, .type = STREAMER_OBJECT_TYPE_DYNAMIC);

// This line can't even be completed because there's no define for object type 3:
AttachDynamicAreaToObject(areaid, dynamicObjectID, .type = ???);
```

Using strict typing strongly encourages people to use the named defines all the time to enable the compiler to help with hard to spot mistakes.

## Configuration

For whatever reason some people hate warnings and would rather turn them off than actually use them to improve their code (I assume they think that no warnings is more important than no bugs).  This despite the fact that warnings do not prevent compilation at all and can be ignored if you just squint a bit.  Anyway, for the sake of those people, these new tags can be disabled.

There are actually three levels of warning enhancements:

### `WEAK_TAGS`

This is the default and gives warnings when untagged values are passed to tagged values (e.g. passing `1` instead of `STREAMER_TYPE_PICKUP`).  Basically everything documented above.  So named because this is pawn *weak tag* semantics, achieved by prefixing all the new tags with a lower-case letter (`t_`).

```pawn
// This is the default, to encourage adoption.
//#define WEAK_TAGS
#include <a_samp>
#include <streamer>
```

### `STRONG_TAGS`

In addition to the weak tag warnings this adds warnings when tagged values are passed to untagged values.  The only place this really matters in the streamer include is `GetDynamicAreaType`, which is currently the only function with a new tag type for its return value (I may have missed some others, feel free to point them out).  So named because this is pawn *strong tag* semantics, achieved by prefixing all the new tags with an upper-case letter (`T_`).

Old code, no warning:

```pawn
#include <a_samp>
#include <streamer>

new type = GetDynamicAreaType(areaid);
```

New weak code, no warning:

```pawn
#define WEAK_TAGS
#include <a_samp>
#include <streamer>

new type = GetDynamicAreaType(areaid);
```

New strong code, tag mismatch warning:

```pawn
#define STRONG_TAGS
#include <a_samp>
#include <streamer>

new type = GetDynamicAreaType(areaid);
```

New strong code, fixed (will also work in all other cases):

```pawn
#define STRONG_TAGS
#include <a_samp>
#include <streamer>

new STREAMER_AREA_TYPE:type = GetDynamicAreaType(areaid);
```

### `NO_TAGS`

Disables all the new tags (except `bool:` currently):

```pawn
#define NO_TAGS
#include <a_samp>
#include <streamer>

// No warning.
new STREAMER_AREA_TYPE:type1 = GetDynamicAreaType(areaid);

// No warning.
new type2 = GetDynamicAreaType(areaid);

// No warning
CreateDynamicMapIcon(1043.0, -436.0, 5.0, 7, 0xFF0000FF, .style = 2);

// No warning
CreateDynamicMapIcon(1043.0, -436.0, 5.0, 7, 0xFF0000FF, .style = MAPICON_GLOBAL_CHECKPOINT);
```

## See Also

https://github.com/pawn-lang/samp-stdlib/tree/consistency-overhaul#tags

Note that this branch has long been accepted for merge, but still isn't.  It will be soon.
@karimcambridge
Copy link

karimcambridge commented Sep 5, 2022

Does this fix include file 2.9.4 and 2.9.5 causing the amx to die on Run time error 17: "Invalid/unsupported P-code file format"?

Edit: nope, it doesn't for me ;\

@Y-Less
Copy link
Contributor Author

Y-Less commented Sep 11, 2022

Does this fix include file 2.9.4 and 2.9.5 causing the amx to die on Run time error 17: "Invalid/unsupported P-code file format"?

Edit: nope, it doesn't for me ;\

I've never seen that error, I don't know what causes it sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants