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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Aug 14, 2019

Conversation

@fancyweb
Copy link
Contributor

commented Feb 20, 2019

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 馃槙
@stof

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

* Vendor code extending other vendor core triggers the deprecations confused

That's totally fine IMO. We already do that. We should maybe only whitelist things from same vendor (or maybe same namespace) as we do in some other places.

@fancyweb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

Yes we already do that for all deprecations but they are rarely triggered by vendor code unlike these new ones. For example, on the homepage of the Symfony demo, there is 3 deprecations atm. With the "return types" deprecations, there are 34.

@stof

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

The thing is, our deprecation reporting tool is meant to tell you when you are ready to migrate to the new major version safely (when there is no deprecation anymore). If some vendor you use rely on the deprecated stuff, you cannot migrate.

Other deprecations are also regularly coming from vendors, and you need to know about them too.

@fancyweb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

I'm fine with that. I just wanted to highlight the fact that if this feature ends up being merged, people are gonna find A LOT MORE of vendor deprecations than what they are used to.

@nicolas-grekas nicolas-grekas added this to the next milestone Feb 21, 2019

@fancyweb fancyweb force-pushed the fancyweb:handle-return-types branch 3 times, most recently from e791814 to 924141c Mar 6, 2019

@fancyweb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Just updated my code to handle the cases from the official PHPDoc types pages https://docs.phpdoc.org/guides/types.html

@stof

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

Triggering a deprecation based on the fact that the interface has @return is wrong. This phpdoc does not imply that the next version will switch to using an actual return type. That would trigger far too many useless deprecations.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 5, 2019

Triggering a deprecation based on the fact that the interface has @return is wrong

wrong is too strong. I agree with the risk you describe. What we're seeking for here is a way to warn ppl they should add return type hints before we add some ourselves.

Let's do as in #30987: keep the deprecation only for Symfony\*. I would also suggest triggering it on PHP 7.2 only so that ppl can actually do something about the notices.

@fancyweb

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

@nicolas-grekas Why PHP 7.2? Don't you mean the lowest PHP version supported by each returnable type ? eg : 7.1 for void, 7.0 for string, etc.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Because only PHP7.2 has the require type variance that allows adding the return type hint without generating a fatal error when the base interface doesn't.

@derrabus

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

Because only PHP7.2 has the require type variance that allows adding the return type hint without generating a fatal error when the base interface doesn't.

PHP 7.1 already allows you to do that. https://3v4l.org/5lN5J

@fancyweb

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

That's what I thought too.

BTW, the help I need here would be to compare both proposed implementations (dedicated functions vs nested conditins) with Blackfire to check the performance impact (since I don't have a subscription myself).

Also TODO :

  • add the check on Symfony\* and the PHP version

@fancyweb fancyweb force-pushed the fancyweb:handle-return-types branch 2 times, most recently from 8e781fe to 20ac604 Jun 28, 2019

@fancyweb

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

Just rebased + took last comments into account.

The deprecations are triggered for any classes that extends / implements a Symfony class / interface and if the PHP version is >= 7.1. 馃憖 But isn't this runtime check useless since the minimum required PHP version by the component is currently 7.2.9?

The failed tests caused by those new deprecations can partially help us to add strict return types in the code base.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

So let's say, I have an event subscriber implementing EventSubscriberInterface with public static function getSubscribedEvents(). My IDE generated a stub function for me, without a return type declaration and with the whole docblock copied over from the interface. Assuming that the interface will receive an : array return type, my subscriber will break when upgrading to Symfony 5 without any prior deprecation notice. imho, I must get a deprecation here, no matter what my docblock says.

If the IDE copies the docblock instead of generating an {@inheritdoc}, that's where the bug is. You won't get a deprecation in this situation that's correct, and there is nothing we can do about it. Note that the logic triggering deprecations doesn't have to be bullet proof. It has to be useful enough to help the mass of the code migrate. Achieving 100% accuracy is not a goal.

Other vendors might have different strategies for upgrading doc blocks to return types. Maybe a library chooses to turn @return array into : iterable. Maybe a vendors chose to use : self for @return $this 鈥 we've decided against that strategy, but that doesn't render the alternative invalid.

Yes, and that's fine: the alternative is worse anyway (hard BC breaks all around or no return types ever for libs).

The @return annotation never had the semantics of "If we eventually add a return type declaration, it's going to be the very type we've annotated here.". We can decide that for the Symfony codebase, @return can/should/must be interpreted that way. But we cannot make that decision for everyone.

Yes we can, we're all in the same boat. Showing where a library can/should be improved is part of a virtous circle that will benefit everyone.

Also, even in Symfony we have doc blocks that intentionally lie (e.g. #32411) because of undocumented internal behavior. I could imagine that other projects have that kind of skeletons in the closet as well.

Then we'll notice when adding real return types - that's the big benefit of them - not being able to cheat. And we'll have to fix them in 4.x. Virtuous circle also.

I would really prefer to solve this problem for Symfony first. And maybe we can find a way for vendors to opt-in to our return type deperecations afterwards? Please, let's not lecture the php world on how to write doc blocks.

Merging will allow validating or invalidating the proposal. Let's not be shy. We'll be in a much better position to fine tune the behavior if we have real world feedback.

@derrabus

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

If the IDE copies the docblock instead of generating an {@inheritdoc}, that's where the bug is.

I strongly disagree. PhpStorm has done this by default ever since, so we need to acknowledge that kind of code is out there. You and I might not consider this habit to be best practice, however it is neither disallowed nor inherently wrong. And with all due respect for novice developers: the kind of developer that keeps the copied doc block might benefit the most from a properly working deprecation system.

Also, this is just an example. It is not disallowed to repeat a @return annotation when implementing an interface. In fact, I have worked with teams in the past that encouraged this practice through their own CS guidelines.

You won't get a deprecation in this situation that's correct, and there is nothing we can do about it.

My example a direct userland implementation of a Symfony interface that will 100% break on upgrade. How's that case undetectable?

Note that the logic triggering deprecations doesn't have to be bullet proof. It has to be useful enough to help the mass of the code migrate. Achieving 100% accuracy is not a goal.

Fair enough. But I would consider my example as pretty essential to the main goal of this PR: Tell the developer, which classes are going to break in Symfony 5 because of newly introduced return type declarations. Not having this kind of notification is what currently holds us back from adding return types to master. Everything else is icing on the cake.

Other vendors might have different strategies for upgrading doc blocks to return types. Maybe a library chooses to turn @return array into : iterable. Maybe a vendors chose to use : self for @return $this 鈥 we've decided against that strategy, but that doesn't render the alternative invalid.

Yes, and that's fine

Absolutely not: We would be luring the developer into adding a return type that is incompatible with the type that the library vendor eventually might chose to add. In that case the upgrade path would become a lot harder.

the alternative is worse anyway (hard BC breaks all around or no return types ever for libs).

I'm not sure if those are the only alternatives.

Again: I don't question the intentions behind this PR. It is very well motivated and I do want this PR to be merged in some form. But I think that the current concept is flawed:

  • Whether a class is going to break in Symfony 5 has nothing to do with its doc block.
  • We're guessing an upgrade path for all php libraries in the world that might turn out to be totally wrong.
  • We're making assumptions about correct and incorrect phpdoc annotations that are stricter than the current (de-facto) standard.

I get the impression that by aiming at fixing the return type problem for the whole world, we're missing the target that we originally aimed at: Create a smooth upgrade path from Symfony 4 to 5.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

My example a direct userland implementation of a Symfony interface that will 100% break on upgrade. How's that case undetectable?

Because you're forgetting about third party bundles that do need an upgrade path too: we need to provide them a way to be deprecation-free with 4.4. Annotations are the way.

We would be luring the developer into adding a return type that is incompatible with the type that the library vendor eventually might chose to add. In that case the upgrade path would become a lot harder.

This sounds scary but I'm not scared: I'm asking for real-world feedback instead because not doing something by fear of something else that might not happen in practice would be a missed opportunity.

There is zero logic currently in DebugClassLoader that is tightly coupled to Symfony. We did something generic here already, that fits the PHP world.

@derrabus

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Because you're forgetting about third party bundles that do need an upgrade path too: we need to provide them a way to be deprecation-free with 4.4. Annotations are the way.

If they would break with Symfony 5, they shouldn't be deprecation-free in 4.4. How many bundles still trigger deprecations because they instantiate a TreeBuilder without $name or dispatch an event with the old order of arguments? The very fact that a bundle triggers deprecations triggers developers to fix them. 馃槂

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

If they would break with Symfony 5, they shouldn't be deprecation-free in 4.4. How many bundles still trigger deprecations because they instantiate a TreeBuilder without $name or dispatch an event with the old order of arguments? The very fact that a bundle triggers deprecations triggers developers to fix them. smiley

the way we want it to work, and achieved doing so is the following:

  • ppl update their deps the best they can
  • they fix all deprecations until there are none left
  • they allow the next major version and do composer up
  • profit (in practice: fix the remaining)

This means bundles should be deprecation free before the bump. Exactly like using Symfony 4.4 is running deprecation-free before the bump despite the return types being not set yet.

@derrabus

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

This means bundles should be deprecation free before the bump.

Exactly. And that means the bundles must have adopted the return types added with Symfony 5, because they're going to break with the bump otherwise. A bundle creator can add return types and at the same time remain compatible with Symfony 4.4 (example). So the path to get the bundle deprecation-free is to add the necessary return types.

@derrabus

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

If the IDE copies the docblock instead of generating an {@inheritdoc}, that's where the bug is.

We actually can find this pattern in the Symfony codebase as well. I've fixed the Twig bridge as an example: #33145.

fabpot added a commit that referenced this pull request Aug 13, 2019

minor #33145 [TwigBridge] Replaced plain doc block copies with inheri鈥
鈥doc (derrabus)

This PR was merged into the 3.4 branch.

Discussion
----------

[TwigBridge] Replaced plain doc block copies with inheritdoc

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | preparation for #30323
| License       | MIT
| Doc PR        | N/A

Commits
-------

e50d3bc [TwigBridge] Replaced plain doc block copies with inheritdoc.
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

that means the bundles must have adopted the return types added with Symfony 5

That's only for packages that rely on some Symfony type. If a dep depends on Symfony, we can assume they don't allow v5 in their composer.json file. Nothing will break until this is relaxed, because v5 will just not be installable yet. How do you know? By trying and seeing composer refuse to upgrade to v5, composer why for the details. The next question is: when does one reach this point in the migration process? Only one possible answer: when you're deprecation-free, because it's pointless to try v5 when you're not yet. But then, the dep that blocks us doesn't use return-types yet! Because if it were, v5 would be allowed for sure! You see the chicken and egg situation?

Now, let's take the group of deps that don't rely on Symfony. Basically, they don't care with what we're doing. So they won't do anything that we could ask them to do, because that'd be added maintainance burden for them, or just because getting a message is hard, provided we have the reach. And that's fair. I don't believe adding an opt-in configuration would work for any dep past the one we have direct influence over.

But I can bet on the fact that deps do want to use types, especially if they already use docblock annotations. I'd bet on this because return-types help maintaining a codebase.

So: yes, there will be docblocks that won't be accurate. But that'll be caught immediately, by just looking at the failure a real return type will trigger. And btw, ppl don't need us to add real return types right now. They're already allowed to do so.

We can help by showing which types would be needed where.
This is exactly what the strategy in this PR provides.

It's true that this won't report annotations that duplicate a parent @return. But that's not an issue if we ship the very next tool we're going to need for the Symfony codebase: one that turns @return into return type declarations.

This PR is creating this forward path, which, I believe, solves all issues mentionned above and creates the smoother process I can think of.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

PR is green BTW :)

@Tobion

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

My example a direct userland implementation of a Symfony interface that will 100% break on upgrade. How's that case undetectable?

Because you're forgetting about third party bundles that do need an upgrade path too: we need to provide them a way to be deprecation-free with 4.4. Annotations are the way.

I think ignoring all user-land classes that have auto-generated phpdocs based on the parent makes this new feature useless in alot of cases. Such code exists alot, probably because that is the default behavior of phpstorm. I don't think we can realistically ignore this.
So I don't think return phpdcocs are the right solution to opting-out of these warnings. Can't we simply ignore all missing return types in vendor libraries? At the end this feature is for end-users and missing return types are irrelevant in vendors. Hm thinking about this, if people split their apps in bundles, then their vendors are not irrelevant. Maybe we can do a combination of both? return phpdocs in vendors are ok for opting out but they are not enough for non-vendor classes?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

return phpdocs in vendors are ok for opting out but they are not enough for non-vendor classes?

I think that's a good plan. Because there is no simple way to decide vendor vs non-vendor at runtime, I think we're going to have to ask ppl to declare their namespace when they're at this step in the migration process.

This means the 1st step would be this PR, exactly as is now. Then, we would ask ppl to run a command / their tests again, but this time with an env var that would give their App\ prefix.
This could either just report the missing spots, or patch directly the source.

We're going to need this tooling too, to migrate this very codebase. Let's see what the next PRs will provide on this path.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

I'm now merging to unlock progress on the topic. Let's keep the discussion open, we'll continue fine-tuning the upgrade path until stable is out.

@nicolas-grekas nicolas-grekas force-pushed the fancyweb:handle-return-types branch from f2354ee to aa338c8 Aug 14, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Thank you @fancyweb.

@nicolas-grekas nicolas-grekas merged commit aa338c8 into symfony:4.4 Aug 14, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Aug 14, 2019

feature #30323 [ErrorHandler] trigger deprecation in DebugClassLoader鈥
鈥 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

@fancyweb fancyweb deleted the fancyweb:handle-return-types branch Aug 14, 2019

@fancyweb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

Thank you @nicolas-grekas 馃槄 馃榿

@mdlutz24

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

This is pretty exciting, and we have been playing around with running Drupal on the 4.4-dev branch to see how to manage this.

We are seeing a few complaints where the Symfony interface @return is ?object, and our implementation uses {@inheritdoc}. Since object is not a php return type, we can't type hint it. I suppose we could quiet the deprecation by adding an @return ?object of our own, but our coding standards don't like it. What's the expected best practice here?

@derrabus

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Since object is not a php return type

It is since php 7.2 馃

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

@mdlutz24 nice to see early interest from Drupal :) @return object|null could be the way to go, you might want to update your CS rules to account for this if that makes sense. Since this PR is merged, I invite you to continue discussing on #33235, which seems the most appropriate to me. We're at a quite early stage on the topic and things are open for changes.

@mdlutz24

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

@derrabus you know that's true and that might be enough, actually. The branch where we actually have to fix this will end up with 7.2 or 7.3 as a minimum, though my test container is 7.1 at the moment.

@mdlutz24

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

@nicolas-grekas it's very early for us too, but as we get closer to branching Drupal 9, we've been trying to be more proactive and involved in SF discussion and development, and running regular tests against symfony -dev instead of waiting for releases. We are definitely interested in helping fine tune this, and hopefully using it or a technique like it ourselves to prep our contrib modules for eventual return type hinting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.