Skip to content

Conversation

widoz
Copy link
Member

@widoz widoz commented Apr 13, 2018

#12

return false;
}

return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

The lines:

if (!$followedBySemicolon) {
   return false;
}

return true;

could just be the single line:

return $followedBySemicolon;

But besides of that, you are forgetting white spaces :)

What could be done instead, is change line 525

$nextToReturn = $file->findNext([T_WHITESPACE], $returnPosition, null, true, null, true);

in

$nextToReturn = $file->findNext([T_WHITESPACE, T_NULL], $returnPosition, null, true, null, true);

So, we search the token after the return *excluding white spaces and null. Which means that we have to return true only if what remain is the end of line...

so line 526 could became just:

return ($tokens[$nextToReturn]['code'] ?? '') === T_SEMICOLON;

We save 10 lines of code, and we make sure that it works also for things like return null; which makes the PR code fail...

Copy link
Member Author

@widoz widoz Apr 14, 2018

Choose a reason for hiding this comment

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

followedBySemicolon: I thought reading the variable names the reading get facilitated since we want to known if there we have a valid return statement.

Ok I'll change it.

Regarding the space I was thinking you refered to the space after the return but not the one before the end statement.

Is there a reason why I want to have a space between the null and the ;?
When we search using findNext excluding [T_WHITESPACE, T_NULL] means the T_NULL must be present otherwise the returned value will be incorrect?

Copy link
Member Author

@widoz widoz Apr 14, 2018

Choose a reason for hiding this comment

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

When you says

we make sure that it works also for things like return null; which makes the PR code fail...

Trying a fixture using the code below the tests pass.

function hh(): void {
    return null;
}

I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why I want to have a space between the null and the ;?

The things is not if there's a reason. This is an helper that returns true/false if the target function respectively, return null/void or not. You know nothing about where this helper will be used.

Assuming that you have a code like this:

function test(): void {
  return ;
}

and you run this helper to this function, with your old code the the helper had returned false, and the sniff as result had shown an error like "Non-void return for void return type.", which off course is misleading, because the function actually return void like the return type states.

Maybe there will be another sniff that will check for that space and will show a proper warning message if the space is not allowed by the styles... but the responsibility of the helper is to check if the function has a non-void return type, and because PHP ignore white spaces, the helper needs to ignore white spaces as well.

@gmazzap gmazzap merged commit d1a3748 into inpsyde:master Apr 15, 2018
@widoz widoz deleted the 12-missing-return-type branch April 17, 2018 20:53
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