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

Generic opening brace placement sniffs do not correctly support function return types #1931

Closed
jrfnl opened this issue Mar 12, 2018 · 5 comments
Milestone

Comments

@jrfnl
Copy link
Contributor

jrfnl commented Mar 12, 2018

The auto-fixer in the Generic.Functions.OpeningFunctionBraceKernighanRitchie sniff currently will break when a function has a return type declaration which is not on the same line as the closing parenthesis of the function declaration.

Example code:

// This works fine:
function myFunction(): Something
{
    return null;
}

// This will create a fixer conflict:
function myFunction($a, $lot, $of, $params)
    : array
{
    return null;
}
@gsherwood
Copy link
Member

I think the solution is to make sure this code doesn't produce an error:

function myFunction($a, $lot, $of, $params)
    : array {
    return null;
}

Because the opening brace is technically on the same line as the declaration, but the declaration is split over multiple lines, which is the job of another sniff to check.

That will also make it consistent with this code, which also produces no error:

function myFunction(
    $a,
    $lot,
    $of,
    $params
) {
    return null;
}

@gsherwood gsherwood changed the title Generic/OpeningFunctionBraceKernighanRitchie: handling of return type declarations Generic opening brace placement sniffs do not correctly support function return types Mar 12, 2018
gsherwood added a commit that referenced this issue Mar 12, 2018
@gsherwood
Copy link
Member

Should be fixed now. Thanks for reporting it.

@gsherwood gsherwood added this to the 3.3.0 milestone Mar 12, 2018
@jrfnl
Copy link
Contributor Author

jrfnl commented Mar 12, 2018

@gsherwood Thanks for working on this.

Unfortunately, the fix has issues with trailing comments:

function myFunction($a, $lot, $of, $params)
    : array // comment
{
    return null;
}

function myFunction($a, $lot, $of, $params)
    : array // phpcs:ignore Standard.Category.Sniff -- for reasons.
{
    return null;
}

When run against Generic.Functions.OpeningFunctionBraceKernighanRitchie, I would expect this to be fixed as (or an error thrown, but not auto-fixed):

function myFunction($a, $lot, $of, $params)
    : array { // comment
    return null;
}

function myFunction($a, $lot, $of, $params)
    : array { // phpcs:ignore Standard.Category.Sniff -- for reasons.
    return null;
}

In reality, this will now be fixed (broken) as:

function myFunction($a, $lot, $of, $params)
    : array {
 // comment
    return null;
}

function myFunction($a, $lot, $of, $params)
    : array {
 // phpcs:ignore Standard.Category.Sniff -- for reasons.
    return null;
}

@gsherwood
Copy link
Member

gsherwood commented Mar 12, 2018

@jrfnl That's not a broken fix - that's how this sniff fixes the brace position when there is a comment on the end. There's a unit test for it already (added here: 5c2a8a1) although it's been fixing code like that for a lot longer.

Edit: I will add more tests to confirm this behaviour across multi-line declarations as well.

gsherwood added a commit that referenced this issue Mar 12, 2018
@jrfnl
Copy link
Contributor Author

jrfnl commented Mar 12, 2018

@gsherwood For the "normal" comment I can understand this to be a valid fix, however for the new PHPCS annotations, it changes the meaning of the annotation completely, so I honestly do think something needs to be done about that.

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

No branches or pull requests

2 participants