Hey!

Thanks for your PR. You are targeting branch "7.4" but it seems your PR description refers to branch "7.4 for features / 6.4, 7.2, or 7.3 for bug fixes".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

I wrote my opinion on #60429 (comment) before:

I voted against #[NoDiscard], so did @fabpot and I'm going to be 👎 here also: this attribute only contributes to make reading PHP code look like syntax soup, with no real value.

I see, never thought about have them on wither methods, as I maybe not personally fan of that pattern 😆.

For me it really only thought it handy for Security related things, like UriSigner::checkRequest, Security::isGranted, ... where dangerous things could happen if somebody forget to check the false case (which we did have in one of our projects, where people thought checkRequest would throw exception and not require handle the return).

I get it. But this looks like a can of worms to me. You did spot the issue, and Q&A is the way to find such issue IMHO.
I understand the security-related aspect which gives some weight to the proposal but I'm still dubious.

@nicolas-grekas

I understand the security-related aspect which gives some weight to the proposal but I'm still dubious.

Let me give an example from few weeks ago in favor of #[NoDiscard]. User can login in 2 ways: classic login form and code in the URL, with more to come. Controller code was similar to this:

private function loginViaCode(Request $request): Response
{
    // find user, check the code in URL query first...

    $this->security->login($user);

    return $this->redirectToRoute('some_route');
}

It all worked, couldn't be any simpler. But later the task was updated: when the user logs in, check if they are synced with Stripe and redirect them to sync page if they are not. So I hooked into LoginSuccessEvent with high priority:

public function __invoke(LoginSuccessEvent $event): void
{
    // check if front-facing route, check $user->isSyncedWithStripe(), early return here
 
    // not synced: doesn't matter how they are logged in, redirect them to sync page
    $route = $this->router->generate('stripe_sync_page');
    $response = new RedirectResponse($route);
    $event->setResponse($response);
    $event->stopPropagation();
 }

But it didn't work; listener was triggered, but user didn't get redirected to stripe_sync_page. Only after I dig into the code I noticed there is a Response|null as return. So I changed the code to:

private function loginViaCode(Request $request): Response
{
     // find user, check the code in URL query first...

    return $this->security->login($user) ?? $this->redirectToRoute('some_route');
}

And then it all started to work.

I think this is a perfect use-case for #[NoDiscard]: not everyone will dig into the code and because the docs are huge, it is easy to miss things. But with an attribute, it would have been much easier to understand the problem: I would get an exception immediately, without reading the code or documentation:

// Security.php

#[NoDiscard('as event hooks and authenticator can put their own response')
public function login(UserInterface $user, ...): Response|null
{}

The problem: Adding #[NoDiscard] would trigger exception to other users who made the same mistake as I did. So I think it should be added in next major version because it is a BC break.

Couldn't all this be done with a custom PHPStan rule? Could be added to phpstan-symfony

@yguedidi

Couldn't all this be done with a custom PHPStan rule? Could be added to phpstan-symfony

Yes, that is true, I just didn't want to put even more text to the comment above. For a silly reason, I added PossiblyUnusedReturnValue suppression in psalm temporarily, and then I forgot about it. Worse: I did it because of just one place where I really didn't need the return type, from my own code 🤦

But not everyone checks for unused return type, and it would also require extra rule (there is one, but can't find it) in phpstan. Where as having an exception with clear message would work on language level. So for as long as users can change their static analysis level, I don't think Symfony or any other package should rely on this check.

There is more. I never use fluent setters, but vast majority of the users do. Even maker bundle creates entities with fluent setter. So if users enable global detection of unused return types, all that code will start to report errors. So it is highly likely that users will simply disable the check on global level (i.e. not just on Entity directory), and then forget about it.

@zmitic thanks for the example. If I understand correctly, you worked on the listener and you spent some time figuring out why it didn't work. The NoDiscard would have saved you some time, but it wouldn't really have prevented you from shipping the issue to prod, since you experienced it during the dev cycle.

On the other end, ignoring the return value of the login() call could be intentional, to prefer another behavior after login.
Adding NoDiscard here will force users that know what they want to write extra syntax soup, not forgetting the BC break you're mentioning indeed.

All this is why I think NoDiscard is not an improvement to anything. Yes, it would have saved you some time in this situation, but there are hundreds ways one can mess up before figuring out the correct code. This special case is not so special to me. It's just a random one that shouldn't deserve so much (syntax) attention.

I'm 👎 on this one as well. Now that return types are everywhere, you should (almost) never ignore the return of a function/method other than void. I also believe that IDEs issue warnings when the return of a function is not used.

Finally, I have the feeling that if we add it here, why not add it to all codebase methods that aren't void?

@alexandre-daubois fluent interfaces are an example typical the return don't need to managed handled. So no it's not about always where there is no void return type.

What methods require to handle a return are methods which are pure (which warn if you don't handle return type), previous PR was declined with adding pure annotation somebody extending UriSigner class could have impure implementation (one which has side effects).

But I understand that the line is hard to define for Symfony itself, why I personally only see it as part of Security related services. So I think we will end in the same state as for pure that it might be better as part of phpstan-symfony package.

Let's close, thanks for proposing and for the discussion.