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

Idea: @psalm-never-throw, alike to @psalm-pure, but promises to not throw/warn #2912

Open
Ocramius opened this issue Mar 5, 2020 · 18 comments

Comments

@Ocramius
Copy link
Contributor

Ocramius commented Mar 5, 2020

Similar to @psalm-pure, it may be interesting to have something like @psalm-never-throw, in which the guarantees are:

  1. the code does not kill the process (yes, the engine can still go out of memory or out of stack frames)
  2. the code does not raise any notices/warnings
  3. the code does not throw exceptions
  4. all thrown exceptions are caught

This is interesting when declaring interfaces with strong guarantees, such as:

/** @psalm-never-throw */
interface Worker
{
    function process(Queue $queue): void;
}

The code in any Worker#run() implementations must therefore only rely on other @psalm-never-throw implementations, or catch any exceptions declared in code that has @throws.

Triggering errors/warnings, exit, die(), as well as the throw construct are forbidden in classes/functions annotated with @psalm-never-throw.

No further guarantees are really possible with the language, but this should play nicely with libraries such as webmozart/assert, or thecodingmachine/safe.

Again, feel free to shoot this down: I'm only throwing this out here as an idea 😉

A list of examples where this is useful:

  1. components such as event listeners (hooks): event listeners are loathed because they can crash the caller in most traditional PHP installations, breaking the entire workflow
  2. process managers/policies, such as [Event] -> IO [Command], which should not crash the caller
  3. workers, which have the responsibility (best effort) of staying alive
  4. event loops
  5. error handlers, which should not themselves produce more errors

EDIT: as highlighted by @ondrejmirtes in #2912 (comment), @throws void is a viable alternative syntax

@fnandot
Copy link

fnandot commented Mar 5, 2020

Good idea, the naming you proposed is more verbose but I would propose @psalm-safe, meaning that's safe to be called without throwing exceptions/errors. What do you think? 🤔

@Ocramius
Copy link
Contributor Author

Ocramius commented Mar 5, 2020

@letnando @psalm-safe seems easily confused with @psalm-pure. For me, a method that does not throw, yet does modify global state, is not safe 🤷‍♀️

@bdsl
Copy link
Contributor

bdsl commented Mar 5, 2020

I wonder if @psalm-throw-only would be a good addition to this, to guarantee that a function only throws exceptions that it declares with @throws.

Then presumably a @psalm-never-throw function could call an @psalm-throw-only function as long as it catches all thrown exceptions.

It's a bit reminiscent of Java's checked exceptions, but with function by function opt-in checking.

@Ocramius
Copy link
Contributor Author

Ocramius commented Mar 5, 2020

@bdsl doesn't @psalm-never-return already do that, when combined with @throws?

@bdsl
Copy link
Contributor

bdsl commented Mar 5, 2020

Ah no, I didn't mean that a psalm-throw-only function must not return - I meant that it must not throw or exit except by throwing one of its declared exceptions. It can still return.

@M1ke
Copy link
Contributor

M1ke commented Mar 5, 2020

For that case would it just make sense to treat @throws as that guarantee anyway? Like, if you're saying @throws FileNotFoundException but your code can also throw IllegalArgumentException you're being misleading surely? (at risk of taking @Ocramius' point off topic...)

@ondrejmirtes
Copy link

PHPStan uses @throws void for this. For example if the parent method has some @throws Exception and you don't want to inherit these tags, you need to use @throws void above the overriden method.

@ondrejmirtes
Copy link

One package that I think takes advantage of this is https://github.com/pepakriz/phpstan-exception-rules/.

@Ocramius
Copy link
Contributor Author

Ocramius commented Mar 5, 2020

Didn't know about @throws void: interesting solution that doesn't need additional/new syntax!

@sebastianbergmann
Copy link
Contributor

I would prefer @psalm-never-throw over @throws void.

@bdsl
Copy link
Contributor

bdsl commented Mar 5, 2020

If we wanted a solution without new syntax I'd write it as @throws never rather than thows void. never already exists in Psalm as an alias for the bottom type never-return.

@bdsl
Copy link
Contributor

bdsl commented Mar 5, 2020

@M1ke I don't think I'd expect @throws to be a gurantee that nothing else will be thrown - I only use @throws to declare exceptions that I think the imediate caller might be interested in catching or redeclaring. For instance I wouldn't generally declare @throws Doctrine\DBAL\Exception on a repository.

@ondrejmirtes
Copy link

@bdsl Yes, this distinction is called checked/unchecked exceptions. You should declare throws and be catching/rethrowing exceptions that are interesting for the application logic and are expected to happen. Something like CustomerCanNoLongerBuyThisProductException.

Unchecked exceptions are for stuff that can break but shouldn't be handled in code, but instead prevented by validations, or fixed in infrastructure. This for example includes PDOException (when the database disconnects) or NegativeIntegerException.

@malukenho
Copy link
Contributor

Doesn't @throw void breaks IDE IntelliSense?

@muglug
Copy link
Collaborator

muglug commented Mar 6, 2020

I have a couple of thoughts, but I'm not sure of their usefulness.

  1. We could make throwing inside a pure/mutation-free context (or calling a function/method that can throw) a specific sort of issue (e.g. ThrowInsidePureFunction)
  2. The code that triggers ThrowInsidePureFunction would be subject to a config flag being manually set to false, maybe allowThrowInsidePureFunctions or similar.

This would mean that a given project's maintainer could decide to opt in to this behaviour globally, then suppress it either at the directory, file, class or function level.

I do something similar for @throws checks – if checkForThrowsDocblock="true" in your config then Psalm will emit MissingThrowsDocblock issues.

@Ocramius
Copy link
Contributor Author

Ocramius commented Mar 6, 2020

Not really what I had in mind, since the example above (workers, listeners, etc) are all but pure code 🤔

@muglug
Copy link
Collaborator

muglug commented Mar 6, 2020

Oh sorry, I thought this was an extra protection on top of pure functions & immutable classes, not a separate (but related) track

@muglug
Copy link
Collaborator

muglug commented Mar 6, 2020

In that case this really is identical to @psalm-immutable/@psalm-pure in terms of enforcement – if I'm reading it correctly the property must be on every single call:

/**
 * @psalm-never-throws
 */
class SomeClass {
  public function someMethod(Foo $foo) {
    $foo->thisMethodMustHaveANeverThrowAnnotation();
    echo (string) $foo; // so should this  __toString()
    echo $foo->aMagicGetWouldNeedItToo;
  }
}

I can see the actual downstream enforcement of that being a bit of a slog – you'd have to make a potentially non-trivial amount of code changes to pass these checks (the same is obviously true of pure/immutable annotations, but at least that cost/benefit ratio is widely understood by the FP community).

This is not something I'm likely to want to implement in Psalm for myself, but I'll gladly accept a PR – I just created a special mattwontfix label. For this and other similar things where I invite others' contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants