Phpinspectionsea: PHP7, missing return types declaration (optional)

[kalessil]

Original link: https://bitbucket.org/kalessil/phpinspectionsea/issues/285/suggestion-weak-warning-php-7-return-type
Author: @mfn

See original ticket

[kalessil]

@mfn : is this ticket still relevant (cleaning tickets up) - JetBrains were working hard in this direction last half-year?

[mfn]

Jup, still relevant. There's no existing inspection which specifically what the title of this issue is.

We got other stuff, like "Missing return tag" or "Incompatible return type" but not something like "Missing return type declaration" or whatever we want to call it.

[funivan]

We can implement even fixer to set specific return type.
And with php 7.1. we get nullable return types and void

[kalessil]

I'd actually target types from both 7.1/7.0 and analyzed all return statements in functions and methods.

[kalessil]

So, requirements engineering:

• inspection applied to non-magic methods from classes/traits;
• PHP language level is taken into account
  
  
  
  • PHP 7+: return types;
  
  
  • PHP 7.1+: nullable and void return types;
  
  
• method return types are resolved natively; with QF supporting following:
  
  
  
  • abstract method return type identified from DocBlock only;
  
  
  • nullable return types identification;
  
  
  • void return type identification;
  
  

@mfn: did I miss anything here?

[mfn]

did I miss anything here?

No expert 😄 but sounds reasonable to me.

inspection applied to non-abstract

I understand for non-magic methods but abstract .. might be useful(?), but not strong opinion about it.

[kalessil]

Let give a try, abstracts return type can be taken from DocBlock directly - easy to pick in fact.

[kalessil]

Status update:

• implemented with QickFix (QF declares fully qualified classes, not sure if this should be similar to ::class QF);
• tests needs to be added;

If anyone wants to play with: fresh binary pushed as well.

[funivan]

@kalessil install plugin from binary.

[funivan]

Notice. It will be cool to have option in the configuration panel "Import classes automatically"
I know that this is not so simple to do, but ti is very annoying to do it manually)

[kalessil]

Heh, looks like @funivan got addicted to quick-fixes =)

[funivan]

False positive:

 class A  {
    public function run(Request $request = null) {
      if (null !== $request->get('do')) {
        return new RedirectResponse('/');
      }
    }
  }

In this case we will break our code if we add RedirectResponse to the return type

[rentalhost]

For PHP 7.1, maybe it could uses a nullable type: ?RedirectResponse.

[kalessil]

PhpDoc is missing, no implicit null return - no chance to suggest nullable here.

[rentalhost]

I guess that for this case, when you don't have return as last statement, then you could consider that it coulds returns null too.

[kalessil]

Nope, that's not a feasible assumption - it can be throw exception or something else.

[kalessil]

method.getType().global(holder.getProject()).filterUnknown().getTypes() - that's how types got resolved, and missing null in resolved types totally makes sence.

[rentalhost]

function x () {
    if ($this->error) {
        throw new ErrorException;
    }

    if ($this->good) {
        return new Good;
    }
}

In this case, it still should returns ?Good, even if exception could happen (it doesn't affects the return typehint). If you have sure that you will always return Good, then if() is needless here, or you should create a new "generic" return that returns the Good too.

There are some real example that you have thought that can causes a problem?

[kalessil]

I guess I have to check the last statement as suggested @rentalhost ... x_x

[kalessil]

And I should admit that in spite of my analytical skills, I'd not come with this solution fast, so thank you @rentalhost

[rentalhost]

That is the advantage of open source world. We can "combine" our minds to improve application.

[kalessil]

And the fix is pushed with fresh binary, next feedback round?

EDIT: QF imports relative QN when possible, will release without auto-import functionality.

[kalessil]

Pilot version of the inspection, QF enhancement follow-up: #243

[funivan]

I will check new version tomorrow

On 12 Apr 2017 9:00 p.m., "Vladimir Reznichenko" [e-mail]
wrote:

Closed #28 https://github.com/kalessil/phpinspectionsea/issues/28.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kalessil/phpinspectionsea/issues/28#event-1040627820,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAZ8-PLXLpt0Cm9cH5jXPmts71A-zAFNks5rvRFQgaJpZM4Kof7W
.

[kalessil]

Super, thank you @funivan