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

Fix misleading pass by reference error message #10639

Merged
merged 2 commits into from Jul 18, 2023

Conversation

kocsismate
Copy link
Member

Currently, when a reference parameter is passed by value, the Argument #x ($y) cannot be passed by reference is written in the error message. This is utterly misleading, since I would assume that the argument mustn't be passed by reference. Now, this is fixed by using the must be passed by reference wording..

@@ -12,7 +12,7 @@ change(list($val) = $array);
var_dump($array);
?>
--EXPECTF--
Fatal error: Uncaught Error: change(): Argument #1 ($ref) cannot be passed by reference in %s:%d
Fatal error: Uncaught Error: change(): Argument #1 ($ref) must be passed by reference in %s:%d
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree with the wording, the issue is that the thing being passed to the function parameter is a value for which a reference cannot be taken, as it's only variables that can be passed by references.

Your wording suggests that I should do something like change(&(list($val) = $array)) which is non sensical.

Better wording may be: Argument #1 ($ref) is a value and therefore cannot be passed by reference in %s:%d

@iluuu1994
Copy link
Member

I guess the most clear would be something like Argument #x ($y) must be passable by reference, although I don't like that wording.

Argument #x ($y) cannot be passed by reference could be interpreted as "was passed by reference but shouldn't be", Argument #x ($y) must be passed by reference as "you need to do something to pass this value by reference", when that is done automatically by the engine.

@bwoebi
Copy link
Member

bwoebi commented Feb 22, 2023

Argument #x ($y) must be passed by reference, but the passed value could not be promoted to reference - something along these lines would be most clear I think: explaining the requirement and then why this fails. No need to be very concise here, error messages should be clear.

@Danack
Copy link
Contributor

Danack commented Feb 22, 2023

Maybe Argument #x ($y) must be a variable passed by reference (not a value) as the result needs to be storable in the reference, which can't be done with values in %s:%d

Also, light blue is a great colour.

@kocsismate
Copy link
Member Author

kocsismate commented Feb 23, 2023

How would Argument #x ($y) must be a variable in order to be able to pass it by reference sound to you? Is it unnatural/misleading?

Edit: maybe Argument #x ($y) must be a variable in order to pass it by reference?

@iluuu1994
Copy link
Member

Is it accurate though? References can be created from properties or array DIMs too.

@kocsismate
Copy link
Member Author

Is it accurate though? References can be created from properties or array DIMs too.

Duh. I'm out of ideas then..

@TimWolla
Copy link
Member

Argument #1 (&$foo) is a reference parameter. The passed value is not writable and thus cannot be passed as a reference.

This suggestion specifically adds the & in front of the $ in the error message. In the first sentence it clearly states this fact once more. The second sentence explains why what the developer is doing is wrong. I'm open to more appropriate phrasing for the second sentence, but I believe the first one is good.

@iluuu1994
Copy link
Member

"Temporary value cannot be used as a reference" might also be an option.

@internalsystemerror
Copy link

My 2 cents: must be referrable

@kocsismate
Copy link
Member Author

What about Argument #x ($y) couldn't be passed by reference then? I think the past tense makes the problem clearer. Apart from this, I like George's suggestion the most: Argument #1 ($ref) is a value and therefore cannot be passed by reference.

@iluuu1994
Copy link
Member

@kocsismate That sounds better, I'd just go for "could not" as we generally seem to avoid abbreviations.

@kocsismate kocsismate merged commit c322da0 into php:master Jul 18, 2023
10 of 12 checks passed
@kocsismate kocsismate deleted the fix-reference-error-message branch July 18, 2023 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants