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

Globally override return type for a method based on argument value #8758

Closed
Chi-teck opened this issue Nov 25, 2022 · 9 comments · Fixed by #8781
Closed

Globally override return type for a method based on argument value #8758

Chi-teck opened this issue Nov 25, 2022 · 9 comments · Fixed by #8781
Labels

Comments

@Chi-teck
Copy link
Contributor

Problem

UndefinedInterfaceMethod
Symfony\Component\Console\Helper\HelperInterface::ask does not exist (see https://psalm.dev/181)
$answer= $this->getHelper('question')->ask($input, $output, $question);

Workaround

I know, it can be handled like follows, but it is a bit annoying and makes the code too verbose.

/** @var \Symfony\Component\Console\Helper\QuestionHelper $helper */
$helper = $this->getHelper('question');
$answer = $helper->ask($input, $output, $question);

Question

Is there a way to establish return types once and for all?
Here is an example of how it works in PhpStorm.

<?php

namespace PHPSTORM_META {

  override(
    \Symfony\Component\Console\Helper\HelperSet::get(),
    map([
      'question' =>\Symfony\Component\Console\Helper\QuestionHelper::class,
      'process' =>\Symfony\Component\Console\Helper\ProcessHelper::class,
    ]),
  );

}
@psalm-github-bot
Copy link

Hey @Chi-teck, can you reproduce the issue on https://psalm.dev ?

@weirdan
Copy link
Collaborator

weirdan commented Nov 25, 2022

Psalm is supposed to read phpstorm.meta files:

if ($this->codebase->register_stub_files
&& $node->name
&& $node->name->parts === ['PHPSTORM_META']
) {
foreach ($node->stmts as $meta_stmt) {
if ($meta_stmt instanceof PhpParser\Node\Stmt\Expression
&& $meta_stmt->expr instanceof PhpParser\Node\Expr\FuncCall
&& $meta_stmt->expr->name instanceof PhpParser\Node\Name
&& $meta_stmt->expr->name->parts === ['override']
) {
PhpStormMetaScanner::handleOverride($meta_stmt->expr->getArgs(), $this->codebase);
}
}
}

@Chi-teck
Copy link
Contributor Author

Seems it reads. I see the following in the debug output.

Parsing /var/www/project//.phpstorm.meta.php
Deep scanning /var/www/project//.phpstorm.meta.php

However, it still produces UndefinedInterfaceMethod errors, while PhpStorm defines correct types for these helpers.

@weirdan
Copy link
Collaborator

weirdan commented Nov 25, 2022

while PhpStorm defines correct types for these helpers.

What's the phpstorm.meta definition for $this->getHelper() (whatever $this is)?

@Chi-teck
Copy link
Contributor Author

@weirdan I am not sure I understood you. The phpstorm.meta.php is in the issue summary.
$this is an instance of Symfony Console command class. All Symfony commands has this method as it defined in the base class.

https://github.com/symfony/symfony/blob/v6.2.0-RC1/src/Symfony/Component/Console/Command/Command.php#L689

@weirdan
Copy link
Collaborator

weirdan commented Nov 25, 2022

Your phpstorm.meta overrides HelperSet::get() return type, but you're calling Command::getHelper(). You would need to override the signature of Command::getHelper() separately.

@Chi-teck
Copy link
Contributor Author

@weirdan Sorry missed this. There is an another override statement in that file.
Here is the full example.
https://github.com/Chi-teck/drupal-code-generator/blob/3.x/.phpstorm.meta.php

@weirdan weirdan added the bug label Nov 25, 2022
@weirdan
Copy link
Collaborator

weirdan commented Nov 25, 2022

It seems we currently require you to specify the argument offset the mapping is based on:

&& $identifier->getArgs()
&& $identifier->getArgs()[0]->value instanceof PhpParser\Node\Scalar\LNumber

So it would work if you did the override like this:

  override(
    \Symfony\Component\Console\Command\Command::getHelper(0), // <==== note the argument offset
    map([
      'service_info' => \DrupalCodeGenerator\Helper\Drupal\ServiceInfo::class,
      'module_info' => \DrupalCodeGenerator\Helper\Drupal\ModuleInfo::class,
      'theme_info' => \DrupalCodeGenerator\Helper\Drupal\ThemeInfo::class,
      'hook_info' => \DrupalCodeGenerator\Helper\Drupal\HookInfo::class,
      'route_info' => \DrupalCodeGenerator\Helper\Drupal\RouteInfo::class,
      'permission_info' => \DrupalCodeGenerator\Helper\Drupal\PermissionInfo::class,
      'config_info' => \DrupalCodeGenerator\Helper\Drupal\ConfigInfo::class,
      'processor' => \DrupalCodeGenerator\Helper\Processor\ProcessorInterface::class,
      'dry_dumper' => \DrupalCodeGenerator\Helper\Dumper\DryDumper::class,
      'filesytem_dumper' => \DrupalCodeGenerator\Helper\Dumper\FileSystemDumper::class,
      'renderer' => \DrupalCodeGenerator\Helper\RendererInterface::class,
      'question' => \DrupalCodeGenerator\Helper\QuestionHelper::class,
      'result_printer' => \DrupalCodeGenerator\Helper\ResultPrinter::class,
    ])
  );

PHPStorm docs sidestep the question of what argument the mapping is based on entirely (https://www.jetbrains.com/help/phpstorm/ide-advanced-metadata.html#passing-a-string-literal), so it's either an undocumented feature or something that used to work previously but doesn't anymore.

Can you please test whether argument offsets work in PHPStorm? If they do, we would just need to default the offset to 0, and if they don't, we would have to drop that additional check and always map on the first argument.

@Chi-teck
Copy link
Contributor Author

I've just set 0 for the argument. PhpStorm still recognizes the helpers and Psalm is no longer complains about UndefinedInterfaceMethod. In fact, the generator itself sets the offset when generating phpstorm meta.
Thank you.

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

Successfully merging a pull request may close this issue.

2 participants