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

[8.x] Declare that abort(), dd() and kill() never return #39108

Merged
merged 6 commits into from
Oct 6, 2021
Merged

[8.x] Declare that abort(), dd() and kill() never return #39108

merged 6 commits into from
Oct 6, 2021

Conversation

caugner
Copy link
Contributor

@caugner caugner commented Oct 6, 2021

This helps static analyzers like PHPStan and Psalm in cases like the following:

$user = User::find($user_id);

if (!$user) {
  abort(404);
}

// Without declaring `abort()` to @return never, Psalm reports a `PossiblyNullArgument` here:
processUser($user);

function processUser(User $user) {
  // ...
}

@caugner caugner changed the title [8.x] Mark that abort() never returns [8.x] Declare that abort() never returns Oct 6, 2021
@bert-w
Copy link
Contributor

bert-w commented Oct 6, 2021

Strictly speaking this is a PHP 8.1 feature right? The @return never isn't used anywhere yet in the code. There are numerous other spots with exit(1); and functions with only throws that do not return anything either. You might want to have it all in a single PR.

@caugner
Copy link
Contributor Author

caugner commented Oct 6, 2021

Strictly speaking this is a PHP 8.1 feature right?

Strictly speaking, never is not (yet) a valid PHPDoc type, but it is recognized by both PHPStorm and Visual Studio Code, and most importantly it is backward compatible (when used in the PHPDoc).

There are numerous other spots with exit(1); and functions with only throws that do not return anything either.

I'm not aware of any other function in Laravel that never returns, but feel free to point those out. This PR focusses on abort(), because afaik it is widely used.

You might want to have it all in a single PR.

Generally speaking, it's easier to get a smaller PR merged (i.e. approved).

@caugner
Copy link
Contributor Author

caugner commented Oct 6, 2021

Actually, dd() is another function that never returns, so it might be worth adding never there as well.

Update: But the global dd() comes from symfony/var-dumper not, from Laravel itself. So never could only be added to Builder::dd() and Stringable::dd().

@bert-w
Copy link
Contributor

bert-w commented Oct 6, 2021

The ones with guaranteed throw statements are harder to find, but there are some more apparent functions with exit(1); like dd() and kill() (Illuminate\Queue\Worker) in the Laravel sourcecode. I count 5 occurrences using a quick search.

@caugner
Copy link
Contributor Author

caugner commented Oct 6, 2021

See symfony/symfony#43343 for adding never to the global dd() function (provided by symfony/var-dumper).

@caugner caugner changed the title [8.x] Declare that abort() never returns [8.x] Declare that abort() / dd() / kill() never return Oct 6, 2021
@caugner caugner changed the title [8.x] Declare that abort() / dd() / kill() never return [8.x] Declare that abort(), dd() and kill() never return Oct 6, 2021
@caugner
Copy link
Contributor Author

caugner commented Oct 6, 2021

@bert-w Feel free to list any other functions that never return, but please do note that this PR does not claim to be exhaustive. It is useful as is.

@taylorotwell taylorotwell merged commit 8a565fa into laravel:8.x Oct 6, 2021
@caugner caugner deleted the patch-2 branch October 6, 2021 14:08
victorvilella pushed a commit to cdsistemas/framework that referenced this pull request Oct 12, 2021
* abort() never returns

* Application::abort() never returns

* App::abort() never returns

* Queue\Worker::kill() never returns

* Database\Query::dd() never returns

* Stringable::dd() never returns
@caugner
Copy link
Contributor Author

caugner commented Oct 13, 2021

@TBlindaruk Thanks for the 8.64.0 release today! 🎉 I noticed that this PR didn't make it into the release notes, and just wanted to ask if this is intentional or whether I did something wrong on my side?

@driesvints
Copy link
Member

@caugner things like DocBlock changes aren't always added to the release notes but they have no effect on the public API.

@caugner
Copy link
Contributor Author

caugner commented Oct 13, 2021

@driesvints Thanks, that makes sense. So release notes entries are written manually. I would have just expected an entry for this, because it might cause additional warnings reported by IDEs and static analyzers, and the release notes don't give a hint why.

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.

4 participants