trigger_error makes Psalm assume the code exits

[williamdes]

This one is similar and psalm thinks that trigger_error makes the code exit I suppose:

https://psalm.dev/r/38c501845b

Ref: #6109

Originally posted by @orklah in #6109 (comment)

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/38c501845b

<?php
class A {
    
    public function __construct()
    {
    global $fooBarCondition, $cookieTheme;
   
    $configThemeExists = true;

    if ($fooBarCondition) {
        trigger_error(
                'Default theme %s not found!',
                E_USER_ERROR
        );
        $configThemeExists = false;
    }
    
        if ($cookieTheme) {
            return;
        }
    
    if ($configThemeExists) {
    }
    }
}

Psalm output (using commit c5190f0):

ERROR: RedundantCondition - 22:9 - if (true) is redundant

INFO: UnusedVariable - 15:9 - $configThemeExists is never referenced or the value is not used

[orklah]

@muglug @weirdan Do you have an opinion?
Psalm considers that trigger_error stop the execution here

psalm/src/Psalm/Internal/Analyzer/ScopeAnalyzer.php

Line 107 in 0a7d254

&& $stmt->expr->name->parts === ['trigger_error']

but not everywhere. (see #6109). What should be the correct behaviour?

[williamdes]

Note: we custom handle calls for trigger_error

[weirdan]

trigger_error() return type (specifically cases when it returns never) depends on the error handler behaviour. Since that's external to Psalm and cannot be easily inferred, Psalm should have a sensible default matching default error handler behaviour and let users override that. I see two options here:

1. Let users stub trigger_error() and use the stub signature consistently. In practice this means we need to remove hardcoded handling we have currently, and just provide a default, overridable, stub.
2. Add an option that would specify error handler signature, and infer trigger_error() return type from that. It would also allow enforcing error handler compatibility.

[orklah]

I like the second option. trigger_error has a default behaviour that is valid for a lot of project that don't touch set_error_handler and when the error_handler use the same behaviour as the default one.
That means we could have an option whose default means that Psalm will use the default behaviour of trigger_error. Other values for this option could be "none" where trigger_error never die and "always" where it always die.

I think this could be handled with a return type provider accessing config, right?

[weirdan]

I think this could be handled with a return type provider accessing config, right?

Sounds good.

That means we could have an option whose default means that Psalm will use the default behaviour of trigger_error. Other values for this option could be "none" where trigger_error never die and "always" where it always die.

This sounds sensible, but so is assuming trigger_error(..., E_USER_ERROR) never returns 🤷.

Let's explore the problem space:

There are four possible values for $error_level: E_USER_ERROR, E_USER_WARNING, E_USER_NOTICE and E_USER_DEPRECATED. Default return type for trigger_error() is @return ($error_level is E_USER_ERROR ? never : true). Theoretically speaking, someone could have a custom handler resulting in @return ($error_level is E_USER_DEPRECATED ? never : true), i.e. continue on everything but deprecation errors and your approach would not allow to configure that.

People may also have different error handlers for some code parts, and the only sensible option I see that would allow to configure that is to make it per-file / dir.

set_error_handler() is also sometimes used to temporary suppress errors (e.g. https://github.com/nikic/FastRoute/blob/2230b97550f2d18c35c0ab1fc7ba32613381cd05/src/Cache/FileCache.php#L79-L95), but those are generally errors emitted by PHP itself and not by trigger_error().

So I would propose something like this:

<triggerError returnType="never"> <!-- project default -->
   <!-- override for a certain files / folder -->
   <signature returnType="($error_level is E_USER_DEPRECATED ? never : true)">
     <directory name="src/Modules/FunnyModule"/>
     <file name="src/strange.script.php"/>
  </signature>

  <signature returnType="default"> <!-- php default -->
    <directory name="vendor"/>
  </signature>

  <signature returnType="true"> <!-- those have errors suppressed and trigger_error() always returns -->
    <file name="src/Cache.php"/>
    <!-- this could also be extended to support something like <referencedMethod> -->
  </signature>
</errorHandler>

[orklah]

That's well thought of, but I feel like it's too complex of a solution for the problem it tries to solve.

If we try to be real here, out of all the projects that will ever use Psalm and out of the projects that have a error handler that will be none on 'default', 'none' or 'always', a few will ever look for the documentation to configurate it just right instead of just suppressing errors

I'll try to propose the basic implementation soon, we can always build on it next if we want to be more thorough

[williamdes]

I am not sure a config option is best, could you analyse the implementations of set_error_handler ?
That would avoid having to document all tricky possibilities a human could imagine

[weirdan]

I am not sure a config option is best, could you analyse the implementations of set_error_handler ?

That would basically force us to consider all those tricky points I raised above. The problem with set_error_handler() is that it's global but is set at runtime, and Psalm is mostly unaware of include/require graph, especially when it passes through autoloader.

Global state is hard to analyze 🤷

[orklah]

This should be basically fixed by the introduction of the config in #6142 unless the beahvior of trigger_error is complex