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

Rule on function prototypes mixed for any/int and ref/non-ref variables #992

Open
FortyTwoFortyTwo opened this issue Oct 30, 2024 · 7 comments

Comments

@FortyTwoFortyTwo
Copy link
Contributor

FortyTwoFortyTwo commented Oct 30, 2024

In 1.11, for function prototypes you're able to mix between any and non-any variables and not able to mix between ref and non-ref, but in 1.12.0.7165 this rule seems to be all over the place on what's allowed and not allowed.

  • No longer able to assign &int param to &any callback, yet still able to assign int param to any callback as normal.
  • Able to mostly mix between ref and non-ref on param and callback

ReturnFunction in this test example are only used to silence unused variable warnings.

typedef CallbackCell = function void (any value);
typedef CallbackRef = function void (any &value);

public void OnPluginStart()
{
	// 4 tests below does not report error in 1.11
	
	CallbackCell cellany = CellAny;	// no error
	ReturnFunction(cellany);
	
	CallbackCell cellint = CellInt;	// no error
	ReturnFunction(cellint);
	
	CallbackRef refany = RefAny;	// no error
	ReturnFunction(refany);
	
	CallbackRef refint = RefInt;	// error
	ReturnFunction(refint);
	
	// 4 tests below does report error in 1.11
	
	CallbackCell badcellany = RefAny;	// no error
	ReturnFunction(badcellany);
	
	CallbackCell badcellint = RefInt;	// no error
	ReturnFunction(badcellint);
	
	CallbackRef badrefany = CellAny;	// no error
	ReturnFunction(badrefany);
	
	CallbackRef badrefint = CellInt;	// error
	ReturnFunction(badrefint);
}

void CellAny(any value) {}
void CellInt(int value) {}
void RefAny(any &value) {}
void RefInt(int &value) {}

Function ReturnFunction(Function func)
{
	return func;
}
@FortyTwoFortyTwo FortyTwoFortyTwo changed the title Rule on Function prototype rule mixed on any/int and ref/non-ref variables Rule on function prototypes mixed on any/int and ref/non-ref variables Oct 30, 2024
@FortyTwoFortyTwo FortyTwoFortyTwo changed the title Rule on function prototypes mixed on any/int and ref/non-ref variables Rule on function prototypes mixed for any/int and ref/non-ref variables Oct 30, 2024
@dvander
Copy link
Member

dvander commented Nov 29, 2024

SourcePawn Compiler 1.11
Copyright (c) 1997-2006 ITB CompuPhase
Copyright (c) 2004-2021 AlliedModders LLC

test.sp(22) : error 100: function prototypes do not match
test.sp(25) : error 100: function prototypes do not match
test.sp(28) : error 100: function prototypes do not match
test.sp(31) : error 100: function prototypes do not match

@FortyTwoFortyTwo
Copy link
Contributor Author

1.11 does correctly report these four as not matching, but 1.12 gives a different result, where it now errors at line 17 and no errors on line 22, 25, and 28.

This makes it that RefInt(int &value) can no longer be used on CallbackRef = function void (any &value), but can instead be used on CallbackCell = function void (any value). Which don't make sense for ref function &value not able to be used on ref callback &value, but can on non-ref callback value

SourcePawn Compiler 1.12.0.7165
Copyright (c) 1997-2006 ITB CompuPhase
Copyright (c) 2004-2021 AlliedModders LLC

42test.sp(17) : error 100: function prototypes do not match
    17 |         CallbackRef refint = RefInt;        // error
-----------------------------^

42test.sp(31) : error 100: function prototypes do not match
    31 |         CallbackRef badrefint = CellInt;        // error
-----------------------------^

dvander added a commit that referenced this issue Nov 29, 2024
Bug: issue #992
Test: new test cases
dvander added a commit that referenced this issue Nov 29, 2024
Bug: issue #992
Test: new test cases
@dvander
Copy link
Member

dvander commented Nov 29, 2024

Thanks, my eyes glazed over on the first look. I've fixed the lack of errors which was definitely a serious bug.

Re: any& coercing to int&... I'm on the fence. I don't love it, but it's not really unsafe. If it's something legitimately useful to a lot of plugins, I'll consider it.

@dvander
Copy link
Member

dvander commented Nov 29, 2024

Allowing this coercion affected 0 plugins in the corpus...

@FortyTwoFortyTwo
Copy link
Contributor Author

In 1.12.0.7165 there is still support for any coercing to int, but now not any& coercing to int& which seems odd to me.

non-ref coercing is very helpful for functions like CreateTimer and RequestFrame, passing over data param. But for ref coercing I only had one case this occurred to me, and that's from my own typeset. Don't know if anyone else used it too.

@dvander
Copy link
Member

dvander commented Nov 30, 2024

Do you have a testcase for non-ref coercing that doesn't work?

@FortyTwoFortyTwo
Copy link
Contributor Author

I think non-ref coercing all works fine for both 1.11 and 1.12, able to use any on int. It's just the case of whenever to allow ref coercing in 1.12, like any& to int&.

typedef CallbackCell = function void (any value);
typedef CallbackRef = function void (any &value);

public void OnPluginStart()
{
	// coercing 'int' param to 'any' callback, works fine
	CallbackCell cellint = CellInt;
	ReturnFunction(cellint);
	
	// coercing 'int&' param to 'any&' callback, function prototypes do not match
	CallbackRef refint = RefInt;
	ReturnFunction(refint);
}

void CellInt(int value) {}
void RefInt(int &value) {}

Function ReturnFunction(Function func)
{
	return func;
}
SourcePawn Compiler 1.12.0.7170
Copyright (c) 1997-2006 ITB CompuPhase
Copyright (c) 2004-2024 AlliedModders LLC

42test.sp(11) : error 100: function prototypes do not match
    11 |         CallbackRef refint = RefInt;
-----------------------------^

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

No branches or pull requests

2 participants