PluginManagerSetsCacheBackendRule false positive

[neclimdul]

reviewing some warnings I found a false positive from the plugin manager cache rule.

The triggering code looks something like this:

  /**
   * Constructs a MagicPluginManager object.
   *
   * @param string $type
   *   The plugin type, for example source.
   * @param \Traversable $namespaces
   *   An object that implements \Traversable which contains the root paths
   *   keyed by the corresponding namespace to look for plugin implementations.
   * @param \Drupal\Core\Cache\CacheBackendInterface $cache_backend
   *   Cache backend instance to use.
   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
   *   The module handler.
   */
  public function __construct($type, \Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler) {
    parent::__construct("Plugin/magic/$type", $namespaces, $module_handler, $this->pluginInterfaces[$type], $this->typeAnnotations[$type]);
    $this->alterInfo('magic_' . $type);
    $this->setCacheBackend($cache_backend, 'magic_' . $type . '_plugins');
  }

As you can se, the cache backend is actually set.

Stepping through the rule though something interesting happens on this line.

https://github.com/mglaman/phpstan-drupal/blob/ffdf3686f3cc3d8c3b3cfece7896dfc5e77d9e51/src/Rules/Drupal/PluginManager/PluginManagerSetsCacheBackendRule.php#L68-L70

Instead of String_, this type is actually \PhpParser\Node\Expr\BinaryOp\Concat because, its concatenating strings.

[neclimdul]

I wonder if this might be providing some false positives or false successes in some of these other places too.

src/Rules/Deprecations/GetDeprecatedServiceRule.php:        if (!$serviceName instanceof Node\Scalar\String_) {
src/Rules/Deprecations/StaticServiceDeprecatedServiceRule.php:        if (!$serviceName instanceof Node\Scalar\String_) {
src/Rules/Drupal/PluginManager/PluginManagerSetsCacheBackendRule.php:                if (!$cacheKey instanceof Node\Scalar\String_) {
src/Rules/Drupal/PluginManager/PluginManagerSetsCacheBackendRule.php:                            if (($item->value instanceof Node\Scalar\String_) &&
src/Rules/Drupal/RenderCallbackRule.php:        if (!$key instanceof Node\Scalar\String_) {
src/Type/ContainerDynamicReturnTypeExtension.php:        if (!$arg1 instanceof String_) {
src/Type/DrupalServiceDynamicReturnTypeExtension.php:        if (!$arg1 instanceof String_) {

[mglaman]

This code is a NIGHTMARE that was written 3 years ago. It needs to be revamped to modern PHPStan

[mglaman]

Main fix:

$cacheKeyType = $scope->getType($cacheKey);
$is_not_empty_string = $cacheKeyType->value();

[mglaman]

The code just needs to be updated to latest PHPStan types, that was written before they existed.

[neclimdul]

I did bang on this a bit over the weekend and the check was easy... but the effects where problematic. Directly after the string check, it tries to get the value and validate it against the tag list. When its _String, but when its Concat, or a variable or... it crashes. I got stalled trying to find a generic way to resolve the value.

I'm afraid its a lost cause though because if say the variable is a constructor argument(like my example) there is no final string.