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

Packages with classmap autoloading are ignored #91

Closed
lulco opened this issue Jul 17, 2020 · 36 comments
Closed

Packages with classmap autoloading are ignored #91

lulco opened this issue Jul 17, 2020 · 36 comments
Assignees
Labels
duplicate This issue or pull request already exists enhancement New feature or request

Comments

@lulco
Copy link

lulco commented Jul 17, 2020

90% of packages I used in one of my project has classmap src autoloading, so they are ignored with reason "Package provides no namespace". They have some namespaces, just it is not mentioned in composer

Is there a way how to use your great library with this type of packages?

Thank you for your work!

@icanhazstring
Copy link
Member

Hi @lulco thanks for reaching out!

The problem in the current state is I am depending on the "string" from the psr0/psr4 value in composer.json of the packages to match against "provided namespaces".

However!
I am currently working on symbol resolving for the next release #89 #85 .
Using this, I will be able to scan through all sort of files and classmap alike to find which symbols a package provides.

I will have some time in the upcoming weeks to fully implement this feature.
In the current state only the "used symbols" are missing to be scanned. The symbol scanning for packages is already working.

So stay tuned :)

@icanhazstring icanhazstring added duplicate This issue or pull request already exists enhancement New feature or request labels Jul 17, 2020
@icanhazstring icanhazstring self-assigned this Jul 17, 2020
@icanhazstring
Copy link
Member

@lulco the new symbol parsing is in a good stand right now.
I would appreciate it if you could check out the PR #89 and test it against one of your projects for real-life coverage. 👍

@lulco
Copy link
Author

lulco commented Sep 16, 2020

I would like to, but I am not sure how to do it. I have this in composer.json:

{
    "require": {
        "icanhazstring/composer-unused": "dev-feature/symbol-scanning as 0.8.0"
    }
}

executing vendor/bin/composer-unused outputs the same as before:

 Ignored packages
 ○ lulco/redis-proxy (Package provides no namespace) <--  package with classmap autoloading

@icanhazstring
Copy link
Member

icanhazstring commented Sep 16, 2020

You need to pass the -x option to the executable to execute the experimental mode which uses the new symbol scanning.

$ vendor/bin/composer-unused -x

@lulco
Copy link
Author

lulco commented Sep 16, 2020

Ah now I read the description of PR :)

So I added option -x and now all packages are marked as unused :))

Results
-------

Found 0 used, 41 unused and 0 ignored packages

 Used packages

 Unused packages
 ✗ php
 ✗ ext-json
 ✗ ext-iconv
 ✗ ext-filter
 ✗ ext-simplexml
 ✗ ext-pdo
 ✗ lulco/redis-proxy

@icanhazstring
Copy link
Member

icanhazstring commented Sep 16, 2020

Haha ok, not what I expected.
Could you give me your full composer.json? (the require section might be also enough)

So I can check it :)

@lulco
Copy link
Author

lulco commented Sep 16, 2020

I had to create some minimal repository, so here it is:

{
    "require": {
        "php": ">= 7.3",
        "lulco/redis-proxy": "^0.4.2",
        "nette/database": "^3.0"
    },
    "autoload": {
        "classmap": ["app"]
    }
}

In app folder I have just one class which uses RedisProxy.

namespace ComposerUnusedTest\Redis;

use RedisProxy\RedisProxy;

class MyRedis
{
    /** @var RedisProxy */
    private $redisProxy;

    public function __construct(RedisProxy $redisProxy)
    {
        $this->redisProxy = $redisProxy;
    }
}

I have composer unused installed "globally" in directory ~/tests/composer-unused so I execute it this way:

cd my-app
~/tests/composer-unused/vendor/bin/composer-unused -x


Results
-------

Found 0 used, 3 unused and 0 ignored packages

 Used packages

 Unused packages
 ✗ php
 ✗ lulco/redis-proxy
 ✗ nette/database

 Ignored packages

@icanhazstring
Copy link
Member

Alright, will check that. Thanks 👍

@icanhazstring
Copy link
Member

Ah, I see what is wrong here.
The problem is not the dependencies, but the root composer package itself - my bad 🤦

I located the problem here: https://github.com/composer-unused/composer-unused/pull/89/files#diff-a4550cf56729e9395c835d913c390bc2R44

I only take psr-0 and psr-4 from the root package (your project) into account.
Which for you ofc does not work. I simply need to do the same as for foreign packages.

Will give you an update as soon as I have something. But good use case, thanks 👍

@icanhazstring
Copy link
Member

@lulco try again. I think I fixed the implementation accordingly :)
At least my test case with classmap works so that the symbols can be found.

@lulco
Copy link
Author

lulco commented Sep 16, 2020

Well, sorry :) still not working for me...

I have added one more package (tomaj/nette-api using psr-4 autoloading):

composer.json:

{
    "require": {
        "php": ">= 7.3",
        "lulco/redis-proxy": "^0.4.2",
        "tomaj/nette-api": "^2.0",
        "nette/database": "^3.0"
    },
    "autoload": {
        "classmap": ["app"]
    }
}

and implementation of ApiHandler

MyApiHandler.php

<?php

namespace ComposerUnusedTest\Api;

use Nette\Http\IResponse;
use Tomaj\NetteApi\Handlers\BaseHandler;
use Tomaj\NetteApi\Response\JsonApiResponse;
use Tomaj\NetteApi\Response\ResponseInterface;

class MyApiHandler extends BaseHandler
{
    public function handle(array $params): ResponseInterface
    {
        return new JsonApiResponse(IResponse::S200_OK, ['message' => 'DONE']);
    }
}

~/tests/composer-unused/vendor/bin/composer-unused -x

Results
-------

Found 1 used, 3 unused and 0 ignored packages

 Used packages
 ✓ tomaj/nette-api

 Unused packages
 ✗ php
 ✗ lulco/redis-proxy
 ✗ nette/database

 Ignored packages

Running without -x option:

~/tests/composer-unused/vendor/bin/composer-unused

Loading packages
----------------

 4/4 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ! [NOTE] Found 2 package(s) to be checked.                                                                             

Scanning files from basedir /var/www/composer-unused-test
---------------------------------------------------------

 2/2 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

Results
-------

Found 2 used, 0 unused and 2 ignored packages

 Used packages
 ✓ php (required by: tomaj/nette-api)
 ✓ tomaj/nette-api

 Unused packages

 Ignored packages
 ○ lulco/redis-proxy (Package provides no namespace)
 ○ nette/database (Package provides no namespace)

So this package is found as used.

But, I am sure I use php :) and not only in tomaj/nette-api package (as second run mentioned).
Also as I wrote before, I have implemented class MyRedis which uses class from lulco/redis-proxy package.
The only unused package should be nette/database

@icanhazstring
Copy link
Member

Well. Hm. Problem with php. You are not using any symbol (class, constant, function) from php itself. So it is detected as unused. 🤔

Need to think about that.
The other, I'll use your code as test then in full. Let's see.

@lulco
Copy link
Author

lulco commented Sep 17, 2020

Yeah I understand :) but I use another php keywords "class", "extends" etc...

Now I am thinking about this: I am not using any php 7.3 feature, so it would be super cool if this was the reason why php is marked as unused :) but probably it would be the whole new package :D

@icanhazstring
Copy link
Member

icanhazstring commented Sep 17, 2020

Yep but class and extends are not a symbol but a keyword.
So I think if at least "one" symbol of the root package was found, we should expect that "PHP" is used ;)

For the version check: We could print out a notice that no feature was used from the required version. But then again, this is totally up to the author what versions he/she wants to support :)

@icanhazstring
Copy link
Member

icanhazstring commented Sep 18, 2020

Good news on the packages tho.
They should now be found as used.

Problem was that the FileSymbol parsing was only taking autoload.files into account, but not autoload.classmap.
Now both methods are supported and should resolve in providing the correct symbols that might be used.

As for the php package. The problem persists, still looking for a good way to mark it used.
But non the less: Thanks for testing with me. Much appreciate it 🙏

@lulco
Copy link
Author

lulco commented Sep 21, 2020

Hi, I just download actual version of the branch and it looks promissing :) packages are marked as used now.

But I found few problems:

  • ext-json is marked as unused even if I use function json_encode / json_decode
  • if I add library lulco/phoenix and run composer-unused --no-progress -x it is marked as used even if I don't use any symbol from it
  • false positive "used packages" if I don't remove imports (uses) from class, for example MyApiHandler:
<?php

namespace ComposerUnusedTest\Api;

use Tomaj\NetteApi\Handlers\BaseHandler; // <--- this import (use) is forgotten here, it is not used, but thanks to it, package tomaj/nette-api is marked as used

class MyApiHandler
{
    public function handle(array $params)
    {
        return ['message' => 'DONE'];
    }
}

Not sure if it is a bug :) I am using php-cs-fixer to find unused uses, so I would remove it and then your script will mark tomaj/nette-api package as unused

@icanhazstring
Copy link
Member

ext-json is marked as unused even if I use function json_encode / json_decode

Seems I have introduced a bug, should still be possible. Will look into it 👍

if I add library lulco/phoenix and run composer-unused --no-progress -x it is marked as used even if I don't use any symbol from it

Another test case. Awesome thanks 😅👍

false positive "used packages" if I don't remove imports (uses) from class, for example MyApiHandler:

This is currently by design. I only parse constants or functions from Namespaces and only use imports. Another feature for the future would be to have a more in depth analysis on how many times a package is used. Maybe one could consider removing the extension and only use a single part of the code.

@icanhazstring
Copy link
Member

@icanhazstring
Copy link
Member

Hi @lulco, I have found a series of small performance impacts as well as bugs inside it.
With the last commit, I think I resolved some of them.

So for example, ext-json should now be marked as used 👍

@lulco
Copy link
Author

lulco commented Sep 24, 2020

Hi @icanhazstring, I have tested current version and ext-json works like expected.
Unfortunately the issue with package lulco/phoenix which is installed but not used, still persists :(

@icanhazstring
Copy link
Member

Will take another look. Thanks for testing 🙏

@lulco
Copy link
Author

lulco commented Sep 24, 2020

I have some observation:

It is not only package lulco/phoenix but also nette/database which is installed but not used.
If I completely delete class MyRedis which looks like:

<?php

namespace ComposerUnusedTest\Redis;

use RedisProxy\RedisProxy;

class MyRedis
{
    /** @var RedisProxy */
    private $redisProxy;

    public function __construct(RedisProxy $redisProxy)
    {
        $this->redisProxy = $redisProxy;
    }
}

all of them (lulco/redis-proxy, nette/database, lulco/phoenix) are marked as unused. The only package which is used in this class was redis-proxy

Sooo I moved lulco/redis-proxy to the end of composer.json

{
    "require": {
        "php": ">= 7.3",
        "ext-json": "*",
        "tomaj/nette-api": "^2.0",
        "nette/database": "^3.0",
        "lulco/phoenix": "^1.3",
        "lulco/redis-proxy": "^0.4.2"
    },
    "autoload": {
        "classmap": ["app"]
    }
}

Run composer update and then run composer-unused command and voila, results are correct.

@icanhazstring
Copy link
Member

Wait what? 🤔
OK I think I need to do some serious digging there. But thanks for the additional information 👍

@icanhazstring
Copy link
Member

icanhazstring commented Sep 30, 2020

Not sure if it is a bug :) I am using php-cs-fixer to find unused uses, so I would remove it and then your script will mark tomaj/nette-api package as unused

I will add some kind of best practices section for composer-unused. As composer unused should always run last. After linting and static analysis to ensure that the code is clean as it should be.

@icanhazstring
Copy link
Member

icanhazstring commented Sep 30, 2020

Ok, I can now confirm that composer-unused -x behaves differently depending on the order of require packages.

{
  "require": {
    "php": ">= 7.3",
    "ext-json": "*",
    "lulco/redis-proxy": "^0.4.2",
    "tomaj/nette-api": "^2.0",
    "nette/database": "^3.0",
    "lulco/phoenix": "^1.3"
  },
  "autoload": {
    "classmap": ["app"]
  }
}

 Used packages
 ✓ lulco/redis-proxy
 ✓ nette/database
 ✓ lulco/phoenix
{
  "require": {
    "php": ">= 7.3",
    "ext-json": "*",
    "tomaj/nette-api": "^2.0",
    "nette/database": "^3.0",
    "lulco/phoenix": "^1.3",
    "lulco/redis-proxy": "^0.4.2"
  },
  "autoload": {
    "classmap": ["app"]
  }
}

 Used packages
 ✓ lulco/redis-proxy

I do have slighty idea where this is coming from. I will add some new behavior to dump the found symbols to io in order to debug the properly :)

@lulco
Copy link
Author

lulco commented Sep 30, 2020

great I will wait for new version to test :)

@icanhazstring
Copy link
Member

icanhazstring commented Sep 30, 2020

Ok found the issue.
The problem was located at the PHPParser NodeVisitor stacking up previously analyzed symbols from previous packages.
So every further package that was getting scanned had the previous existing symbols.

This is why the order of the packages mattered.
Should now work regardless of the order :)

@icanhazstring
Copy link
Member

Hi @lulco I have merged the PR #89 with a new development branch 0.8.x.
This way I can keep track of things that don't work out right now with the new symbol scanning.

Can you check again if the current state is working for you?
Thanks 🙏

@lulco
Copy link
Author

lulco commented Oct 19, 2020

Hi @icanhazstring :) sorry, not really work for me :( I've installed 0.8.x-dev branch.

My repo now contains only MyRedis class in app dir

<?php

namespace ComposerUnusedTest\Redis;

use RedisProxy\RedisProxy;

class MyRedis
{
    /** @var RedisProxy */
    private $redisProxy;

    public function __construct(RedisProxy $redisProxy)
    {
        $this->redisProxy = $redisProxy;
    }
}

and composer.json

{
    "require": {
        "php": ">= 7.3",
        "ext-json": "*",
        "lulco/redis-proxy": "^0.4.2",
        "tomaj/nette-api": "^2.0",
        "nette/database": "^3.0",
        "lulco/phoenix": "^1.3"
    },
    "autoload": {
        "classmap": ["app"]
    }
}

It seems like only psr packages are analyzed. Did I miss something?

~/tests/composer-unused/vendor/bin/composer-unused


Loading packages
----------------

 6/6 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ! [NOTE] Found 3 package(s) to be checked.                                                                             

Scanning files from basedir /var/www/composer-unused-test
---------------------------------------------------------

 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

Results
-------

Found 2 used, 1 unused and 3 ignored packages

 Used packages
 ✓ php (required by: tomaj/nette-api)
 ✓ ext-json (required by: tomaj/nette-api)

 Unused packages
 ✗ tomaj/nette-api

 Ignored packages
 ○ lulco/redis-proxy (Package provides no namespace)
 ○ nette/database (Package provides no namespace)
 ○ lulco/phoenix (Package provides no namespace)

@icanhazstring
Copy link
Member

Hm. Did you run using composer unused -x since the output seems to be the "old" one.

@lulco
Copy link
Author

lulco commented Oct 19, 2020

yes, but I got error The "-x" option does not exist.

@icanhazstring
Copy link
Member

Ah yes. My bad, the current main branch has an alias for 0.8.x-dev. Could you try require dev-0.8.x? That should be the branch then.

@lulco
Copy link
Author

lulco commented Oct 19, 2020

composer cannot install it:

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - The requested package icanhazstring/composer-unused dev-0.8.x exists as icanhazstring/composer-unused[0.1.0, 0.1.1, 0.2.0, 0.3.0, 0.3.1, 0.4.0, 0.5.0, 0.5.1, 0.5.2, 0.5.3, 0.5.4, 0.5.5, 0.5.6, 0.6.0, 0.6.1, 0.6.2, 0.7.0, 0.7.1, 0.7.2, 0.7.3, 0.7.4, 0.8.x-dev, dev-feature/captainhook, dev-feature/dependency-suggested-by, dev-feature/symbol-scanning, dev-fix/extends-implements, dev-main] but these are rejected by your constraint.

@icanhazstring
Copy link
Member

Wow ok sorry. I did fuck up ;)
I have removed the branch-alias from the main branch and removed also the version on packagist.
You should now be able to require it using

composer require icanhazstring/composer-unused:0.8.x-dev

And run it via composer unused -x.
Sorry for the inconvenience.

@lulco
Copy link
Author

lulco commented Oct 19, 2020

Great, now it works :)

I think, we can close this issue as resolved. Thank you

@icanhazstring
Copy link
Member

Awesome. Thanks for testing 🙏
Will close this then with 0.8.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants