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

no_spaces_after_function_name applies to non-functions #7516

Closed
kamil-tekiela opened this issue Dec 7, 2023 · 6 comments
Closed

no_spaces_after_function_name applies to non-functions #7516

kamil-tekiela opened this issue Dec 7, 2023 · 6 comments
Labels

Comments

@kamil-tekiela
Copy link
Contributor

When using the no_spaces_after_function_name fixer it changes spaces around parentheses after language constructs such as include or echo. In fact, they are listed as examples of "functions" in the fixer documentation.

This fixer should not work on language constructs that happen to use parentheses. Main examples are require, include, echo, print. It's fine to apply this rule to language constructs that use parentheses in their syntax, although a little dubious.

-echo ('Hello World!');
+echo('Hello World!');

This should not happen. It's wrong and can lead to introduction of bugs in code as the visual separation between the construct and the operand is gone.

Please fix this fixer.

@Wirone
Copy link
Member

Wirone commented Dec 7, 2023

Hi @kamil-tekiela! It looks like duplicate (or very close) of #5712, but at the same time it's working as expected 🙂.

I understand that your personal preference is different, but I don't get how single space or lack of it can lead to bugs 🤔? Does it really matter if it's actual function call or language construct with parentheses? From my perspective it's better to keep consistency across the code.

Closing as this works as intended (described with tests).

@Wirone Wirone closed this as not planned Won't fix, can't repro, duplicate, stale Dec 7, 2023
@kamil-tekiela
Copy link
Contributor Author

kamil-tekiela commented Dec 7, 2023

Yes, it does matter. The lack of space isn't a bug on its own, but it leads to visual confusion that can be a cause for bugs.

Taken from the official documentation https://www.php.net/manual/en/function.print.php#refsect1-function.print-notes

print (1 + 2) * 3;
print(1 + 2) * 3;

Which one is easier to understand that the parentheses are mathematical operators and not part of the print syntax?

The same can happen with include or require:

$a = require ('hello.php') . ' world!'; // parentheses in the wrong place

Of course, PHP developers should never put useless parentheses after language constructs*, but it doesn't mean that a PHP fixer should endorse this practice. If users put parentheses there by mistake then at least PHP fixer should make it absolutely clear that it is not a function and not apply the fix to it. The distinction is very important to prevent bugs.

At the very least, the example in the tool's documentation should not use language constructs. This sends the wrong message.

* As mentioned in the linked duplicate report, sometimes the parentheses are important and put there intentionally.

@Wirone
Copy link
Member

Wirone commented Dec 7, 2023

Your initial example included function-like call, and in that case space does not matter. Now you're pointing to language construct with expression, that was fixed in #7430 and should work properly. Issue mentioned before probably can be closed too, I'll verify it when I sit with computer.

@keradus
Copy link
Member

keradus commented Dec 7, 2023

Issue mentioned before probably can be closed too, I'll verify it when I sit with computer.

it did. closed.

@keradus
Copy link
Member

keradus commented Dec 7, 2023

for most of the usecases, I believe we are already on good side thanks to #7430.

if you believe that for echo (1) (but not echo (1)*2, as already handled), we should also not remove the space, I'm open to receive a contribution to make that happen, to fix the docs you already identified and to remove the useless parentheses, ultimately forming echo (1) -> echo 1.
Big thanks for your support to make that happen, @kamil-tekiela !

@keradus
Copy link
Member

keradus commented Dec 7, 2023

ha, wait, it's there. no_unneeded_control_parentheses rule

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

No branches or pull requests

3 participants