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

(support) "TooManyTemplateParams" for ReflectionAttribute in local, but not in online sandbox #6619

Closed
donquixote opened this issue Oct 9, 2021 · 18 comments

Comments

@donquixote
Copy link
Contributor

donquixote commented Oct 9, 2021

https://psalm.dev/r/5446aafa14
This online sandbox version works just fine

However, in my local env, I get this:

ERROR: TooManyTemplateParams - C.php:14:11 - ReflectionAttribute<T:N\C as object> has too many template params, expecting 0 (see https://psalm.dev/184)
  private \ReflectionAttribute $r;


ERROR: TooManyTemplateParams - C.php:18:13 - ReflectionAttribute<T:N\C as object> has too many template params, expecting 0 (see https://psalm.dev/184)
   * @param \ReflectionAttribute<T> $r

Why might this be? What can I do to debug?


About my local installation:

  • PHP 8.0.11
  • composer.json with vimeo/psalm as the only dependency. I tried with 4.x-dev (same commit id as the online demo) and with 4.10 stable.
  • single custom php file C.php, code as in the demo.
  • psalm.xml created with ./vendor/bin/psalm --init, then set error level = 6 or less.
@orklah
Copy link
Collaborator

orklah commented Oct 9, 2021

Please share some code for your errors.

Keep in mind that psalm.dev runs on PHP 8.1. changes may have been made on specific classes

@donquixote
Copy link
Contributor Author

Sorry, I had the wrong link in my initial post. Fixed.

@donquixote
Copy link
Contributor Author

(to ping the bot)
https://psalm.dev/r/5446aafa14

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/5446aafa14
<?php

declare(strict_types=1);

namespace N;

/**
 * @template T as object
 */
class C {
  /**
   * @var \ReflectionAttribute<T>
   */
  private \ReflectionAttribute $r;

  /**
   *
   * @param \ReflectionAttribute<T> $r
   * @param \ReflectionClass<T> $rc
   */
  function __construct(\ReflectionAttribute $r, \ReflectionClass $rc) {
    $this->r = $r;
  }
}
Psalm output (using commit 950b21d):

No issues!

@donquixote
Copy link
Contributor Author

Keep in mind that psalm.dev runs on PHP 8.1. changes may have been made on specific classes

Still happening in my local with PHP 8.1.

@orklah
Copy link
Collaborator

orklah commented Oct 9, 2021

Still happening in my local with PHP 8.1.

Can you add --php-version=8.1 to be sure?

I don't reproduce locally when running in PHP 8.0 and --php-version=8.1. I do reproduce when running PHP 8.0 without --php-version=8.1

@donquixote
Copy link
Contributor Author

Very interesting.
For me it is now a question of caching.
If I run with --no-cache --php-version=7.4 or --no-cache, I do get the warnings.
If I run with --no-cache --php-version=8.0 or --no-cache --php-version=8.1, I get no warning.

So I see the following problems:

  • With --php-version=7.4, but running on PHP 8, psalm only sees the native class, not the stub.
  • With --php-version=8.0, but running on PHP 7.4, I assume it will find the stub but not the native class? not sure.
  • Without --no-cache, classes from different php version are stuck in the cache. I think the caches should be isolated or cleared by php version.
  • (I was going to add one more point but now I'm on the phone and I forgot :) )

@orklah
Copy link
Collaborator

orklah commented Oct 9, 2021

Yeah, cache issue are probable when switching things like that.

But to get back to the initial point, what is exactly your issue? Do you have a valid piece of code you expect to run on a specific version and where Psalm tells you it's invalid on this version?

@donquixote
Copy link
Contributor Author

For me it would be desirable to have forward support for attributes.
I am working on a package for attributes discovery in PHP < 8, but there is one "native" implementation that uses native attributes implementation. Similar to https://github.com/spiral/attributes, but I want to speed up the parsing (spiral uses nikic/php-parser).
So in PhpStorm I set the PHP version to 7.4, which I assume informs psalm's --php-version parameter when running it with PhpStorm inspections.

@donquixote
Copy link
Contributor Author

Could this be solved by using my own stub? I'm afraid PhpStorm will get confused for duplicate classes.

@orklah
Copy link
Collaborator

orklah commented Oct 9, 2021

Psalm uses the lowest supported php version from your composer.json file to know which version should be used to analyze.

You can probably use your own stubs yeah, PHPStorm won't read them if you put them in an extension not recognized as php (for example .phpstub)

@donquixote
Copy link
Contributor Author

Just for some context:
This is the class where I wanted to use \ReflectionAttribute<T>: https://psalm.dev/r/208b9aee01
This is the interface: https://psalm.dev/r/0e2d6477b7

I think the following could be improved in psalm:

  • Isolate caching by php version
  • Deal with the case where the actual php version is different from the --php-version parameter, and we get inconsistency between stubs and native classes. Perhaps all we need here is better reporting, so that the user gets an idea how to solve it.
  • Forward support for new features.

For my own project I will see how far I get by adding my own stubs.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/208b9aee01
<?php

declare(strict_types=1);

namespace Donquixote\QuickAttributes\RawAttribute;

/**
 * @template-covariant T as object
 *
 * @template-implements RawAttributeInterface<T>
 */
class RawAttribute_NativeReflection implements RawAttributeInterface {

  /**
   * @var \ReflectionAttribute<T>
   */
  private \ReflectionAttribute $attribute;

  /**
   * Constructor.
   *
   * @param \ReflectionAttribute $attribute
   */
  public function __construct(\ReflectionAttribute $attribute) {
    if (PHP_VERSION_ID < 80000) {
      throw new \RuntimeException('This class requires PHP 8+.');
    }
    $this->attribute = $attribute;
  }

  /**
   * @return class-string<T>
   */
  public function getName(): string {
    return $this->attribute->getName();
  }

  /**
   * {@inheritdoc}
   */
  public function getArguments(): array {
    return $this->attribute->getArguments();
  }

}
Psalm output (using commit 950b21d):

ERROR: UndefinedClass - 12:48 - Class, interface or enum named Donquixote\QuickAttributes\RawAttribute\RawAttributeInterface does not exist
https://psalm.dev/r/0e2d6477b7
<?php

declare(strict_types=1);

namespace Donquixote\QuickAttributes\RawAttribute;

/**
 * @template-covariant T as object
 */
interface RawAttributeInterface {

  /**
   * @return class-string<T>
   */
  public function getName(): string;

  /**
   * @return array
   *
   * @throws \ReflectionException
   */
  public function getArguments(): array;

}
Psalm output (using commit 950b21d):

No issues!

@donquixote
Copy link
Contributor Author

I think the following could be improved in psalm:

Perhaps another idea could be per-file php version, or even per statement, by looking at "if (PHP_VERSION_ID < ...)" or @since 8.0 docs? I am going to assume this is too hard to do, but it would be quite useful for packages that provide optional support for higher php versions.

@donquixote
Copy link
Contributor Author

For my own project I will see how far I get by adding my own stubs.

For others who find this thread:
It works after creating the stub in stubs/ReflectionAttribute.phpstub, and then adding this in psalm.xml:

  <stubs>
    <file name="stubs/ReflectionAttribute.phpstub"/>
  </stubs>

@orklah
Copy link
Collaborator

orklah commented Oct 9, 2021

Caching could probably be improved yeah but it rarely come up in issues, so not a priority.

In theory, we have very few cases where there are difference between the run version and described version. If you have, please share a minimal reproducer.

I'm not sure what you mean by forward support exactly.

And yeah, flow analysis is pretty complex without adding different php version with different stubs in the mix

@donquixote
Copy link
Contributor Author

I'm not sure what you mean by forward support exactly.

Perhaps my idea is also still quite vague :)
In general, the idea is to have a package with minimum php version e.g. 7.2, 7.3 or 7.4, but with some code that requires PHP 8, either for optional functionality or to switch between a built-in vs a userland implementation.
For this it would be useful to act as if a class like \ReflectionAttribute exists even when running with PHP 7.4 as the package php version. Or to act as if the #[..] attribute syntax works even in lower php versions.

This could scale up if an ecosystem like Drupal or symfony starts using userland-parsed attributes, while still supporting PHP 7.4 or lower.

Perhaps there are examples other than attributes, but for now this is all I can think of.

But it seems for now a lot of this can be solved with custom stubs. So I should see how far I get with this before I open more issues :)

@orklah
Copy link
Collaborator

orklah commented Oct 9, 2021

I'll close this in the meantime.

Please open 1 issue per idea/problem.

Also keep in mind that your needs as a library maintainer could be the opposite of the need of the majority of users. In most cases, we actually want to prevent users to use a feature that is not yet available in their lowest supported version

@orklah orklah closed this as completed Oct 9, 2021
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

No branches or pull requests

2 participants