phpstan / phpstan

PHP Static Analysis Tool - discover bugs in your code without running it!
https://phpstan.org/
MIT License
12.82k stars 866 forks source link

Central repository for function/method signatures and return types #846

Closed muglug closed 6 years ago

muglug commented 6 years ago

Continuing on from the discussion on f486280bf7746e20661f3c917f63749915f1be59, where the diff breaks the interface

Concerning a unified format:

Psalm uses three sources of information for builtin function/method lookups:

I wonder if it's necessary to combine both the signature map and stub files? The format of the function signature map is expressive enough for 99.5% of all functions and methods, and the remainder can be handled either on a case-by-case basis or by generically-typed stubs.

Additionally the CPU, memory, and storage constraints are optimised for each use case. From a maintenance perspective, @TysonAndre and I do a pretty good job of keeping our copies of the signature map in sync, and would welcome any additional information.

cc @Majkl578 @carusogabriel @greg0ire

TysonAndre commented 6 years ago

Thoughts: FunctionSignatureMap.php takes around 0.04 seconds to parse/load, which shouldn't be noticeable compared to the rest of analysis. Requiring the entire file isn't going to be slow. If you have differences in the way you analyze it, there's two approaches you could take that would still be efficient:

  1. Maintain your own copy, periodically sync the changes. Lazily create the object instances based on that format
  2. Maintain a file which is the list of differences (e.g. signatures to remove (should remove all alternates), signatures to replace, signatures to add, and add those at runtime.

On some systems/disks, I'd think loading hundreds of files could be slower than accessing a single larger file.


Phan's sources of information/inferences about internal functions:

  1. FunctionSignatureMap.php
  2. Reflection, or internal_stubs
  3. https://github.com/phan/phan/tree/master/src/Phan/Plugin/Internal (e.g. https://github.com/phan/phan/blob/master/src/Phan/Plugin/Internal/ArrayReturnTypeOverridePlugin.php for working with closures)
  4. Some older code in a function Analyzer which special cases functions that have multiple signatures, such as "implode" (Merged into Phan\Plugin\Internal)

A tangential feature: Phan also has https://github.com/phan/phan/tree/master/.phan/internal_stubs (Phan's FunctionSignatureMap isn't an exact substitute for ReflectionFunction/ReflectionMethod - A function can have multiple alternate signatures)

See https://github.com/phan/phan/blob/0.10.4/.phan/config.php#L427-L439 for an example of this.

internal_stubs is potentially useful to know if a function call would be guaranteed to result in a PHP Error (Too many/few arguments, TypeError)

Again, this is tangential, but having something like https://github.com/DefinitelyTyped/DefinitelyTyped (From typescript and node.js) would be useful for analyzing a project.


I wonder if it's necessary to combine both the signature map and stub files? The format of the function signature map is expressive enough for 99.5% of all functions and methods, and the remainder can be handled either on a case-by-case basis or by generically-typed stubs.

Also see https://github.com/phan/phan/issues/1438 . It's probably doable to import the signatures for phpstorm, but they're probably going to need a lot of manual fixing (E.g. phpstorm only supports a single signature, phpstorm might document a type as mixed instead of int|false, etc.


Another thought that I had was that a standardized format might be modified to include additional information. (E.g. using the array key of the number 1)

E.g.

TysonAndre commented 6 years ago

Also, there's around 4 sources of information for documentation that would be useful (for people using PHP) to have kept in sync:

(E.g. no complete contradictions in required/optional param counts, or types, or whether something is variadic or not for a given extension version)

  1. Reflection (zend arginfo). -- https://github.com/phan/phan/blob/master/internal/sanitycheck.php checks that that's in sync with FunctionSignatureMap. I filed issues for some of the inconsistencies (E.g. wrong required/optional parameter counts). But, I didn't have time for all of the remaining issues, and that doesn't include the majority of extensions, many of which have no arginfo set.
  2. docs.php.net -- https://github.com/phan/phan/blob/master/internal/internalsignatures.php imports that as FunctionSignatureMap
  3. FunctionSignature.php (Phan/Psalm)
  4. phpstorm stubs (E.g. FunctionSignature.php may say something is an int, phpstorm might say something is mixed/string).

    This can probably be imported by a mechanism similar to what is used for .phan/internal_stubs files

Those linked scripts are messy right now and should be refactored eventually

carusogabriel commented 6 years ago

Sorry if my question is going to be trivial, but: why not depends on native PHP's Reflection anymore?

muglug commented 6 years ago

@carusogabriel asked and answered here: https://github.com/phan/phan/issues/494

lookyman commented 6 years ago

I spoke with @artspb today and he told me that JetBrains doesn't tag their stubs repo because they don't need it, but they would be willing to do that so that other tools can use it. If that would be of use for you guys, just open a ticket in their issue tracker.

artspb commented 6 years ago

@lookyman Here's the issue https://youtrack.jetbrains.com/issue/WI-40958

@everyone Please feel free to comment there with your suggestions on how it should be done. We indeed don't need tags, so our goal is to make them usable for you.

ntzm commented 6 years ago

Not sure if anyone has seen, but they've started tagging now https://github.com/JetBrains/phpstorm-stubs/releases

muglug commented 6 years ago

I think this is long-ago fixed!