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

[Debug] throw deprecation when @return void is found #30123

Closed
nicolas-grekas opened this issue Feb 9, 2019 · 7 comments
Closed

[Debug] throw deprecation when @return void is found #30123

nicolas-grekas opened this issue Feb 9, 2019 · 7 comments

Comments

@nicolas-grekas
Copy link
Member

The DebugClassLoader does lightweight static analysis at autoload time already and triggers deprecations when some annotations in a docblock hint it to do so.

When this class finds a method that doesn't have void as return type but whose parent has the @return void annotation, we should trigger a deprecation - unless it also has the same annotation.
The should hint ppl to add the void return type immediately in their code and be ready for next-major of the parent class, which will be able to smoothly turn the annotation to a real void declaration.

ping @fancyweb @GuilhemN since you contributed similar rules in the past.

@nicolas-grekas nicolas-grekas added the Help wanted Issues and PRs which are looking for volunteers to complete them. label Feb 9, 2019
@theofidry
Copy link
Contributor

@nicolas-grekas wouldn't that be both tricky & incomplete since traditionally:

function foo();

implies:

function foo(): void;

Whereas if something was returned, we have:

/**
 * @return mixed
 */
function foo();

?

@nicolas-grekas
Copy link
Member Author

function foo(); implies: function foo(): void;

That's maybe conceptually correct - you can't rely on anything else - but that's technically inaccurate - you can return anything with such a signature.

That's why it works - we only care about technicalities here.

@theofidry
Copy link
Contributor

theofidry commented Feb 9, 2019 via email

@nicolas-grekas
Copy link
Member Author

From the fabbot failure on #30124 we can even say there are none :) Which is nice actually as ppl can start using it and express backward-compatible semantics for void.

@fancyweb
Copy link
Contributor

fancyweb commented Feb 9, 2019

@nicolas-grekas Why should we do it only for the void return type ?

@nicolas-grekas
Copy link
Member Author

Good question :)
We can do it for other return types IF they can be expressed as real return type indeed.

@fancyweb
Copy link
Contributor

fancyweb commented Feb 9, 2019

I'm going to work on it.

@nicolas-grekas nicolas-grekas removed the Help wanted Issues and PRs which are looking for volunteers to complete them. label Feb 25, 2019
nicolas-grekas added a commit that referenced this issue Aug 14, 2019
… when child class misses a return type (fancyweb, nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[ErrorHandler] trigger deprecation in DebugClassLoader when child class misses a return type

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #30123
| License       | MIT
| Doc PR        | TODO

I wanted to push something to show the advancement and get feedback.

I pushed two versions : one with dedicated functions for code clarity (DebugClassLoader.php) and one withtout (DebugClassLoader___.php). It would be nice if some people with Blackfire could compare the performances.

So let's be clear, we are never gonna be able to cover all cases! We can however cover the vast majority.

Current non covered cases and problems :
- We assume that if there is more than 2 returned types, we cannot do anything. Even if it could technically be possible.
- We assume that any returned type that doesn't fit our "returnable" types list is a class. We don't check at all if this class actualy exists.
- We don't handle spaces in types. The types stop at the first space.
- That means we don't handle (yet) the callable type with spaces (cf #29969)
- Vendor code extending other vendor core triggers the deprecations 😕

Commits
-------

aa338c8 Import return annotations from vendors
10fc13e [ErrorHandler] Handle return types in DebugClassLoader
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

No branches or pull requests

3 participants