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

WIP: Add scanning using symbols #89

Merged
merged 30 commits into from Oct 16, 2020
Merged

Conversation

icanhazstring
Copy link
Member

@icanhazstring icanhazstring commented Jun 12, 2020

Work in Progress

This will rework how dependencies and usages are handled.
Previously only Namespaces were checked, which doesn't work well for constants or functions that are provided by a dependency but don't have a namespace.

More information see #85


How to test:

$ composer require --dev icanhazstring/composer-unused:dev-feature/symbol-scanning

Then run

$ composer unused -x

Current state:

  • suggest/provide not implemented in experimental mode
  • error handling not working correctly
  • should be able to scan psr0/4, classmap and file packages

@icanhazstring icanhazstring marked this pull request as draft June 12, 2020 09:36
@icanhazstring icanhazstring self-assigned this Jun 12, 2020
@icanhazstring icanhazstring changed the base branch from master to main June 12, 2020 11:33
@icanhazstring
Copy link
Member Author

icanhazstring commented Jun 12, 2020

Goto logic to load symbols from required dependencies:

$rootPackage = $composer->getPackage(); // self
$rootSymbolLoader = new RootSymbolLoader();
$dependencySymbolLoader = new DependencySymbolLoader(...);
$providedSymbols = [];

foreach ($rootPackage->getRequires() as $requiredPackage) {
    $composerPackage = $this->resolvePackage(...); // Create package for extension or find actual installed package
    $providedSymbols[] = $dependencySymbolLoader->load($package); // Get single generator for each dependency
}

$usedSymbols = $rootSymbolLoader->load($rootPackage); // Load add external symbols, ignoring symbols having name rootnamespace

// TODO: compare used against provided

@icanhazstring
Copy link
Member Author

Working example to compare a single Symbol against a required package:

$rootPackage = $composer->getPackage(); // self
//$rootSymbolLoader = new RootSymbolLoader();
$dependencySymbolLoader = new DependencySymbolLoader(
    new FunctionConstantSymbolProvider(
        new SymbolNameParser(
            (new ParserFactory())->create(ParserFactory::ONLY_PHP7),
            new SymbolNodeVisitor()
        ),
        new FileContentProvider()
    ),
    new SymbolLoader()
);

$requiredDependencies = [];

foreach ($rootPackage->getRequires() as $require) {
    $composerPackage = $resolvePackage($require);

    if ($composerPackage === null) {
        continue;
    }

    $requiredDependencies[] = new RequiredDependency(
        $composerPackage,
        (new SymbolList())->addAll(
            $dependencySymbolLoader->load($composerPackage)
        )
    );
}

$usedSymbols = [new Symbol(PackageInterface::class)];

foreach ($requiredDependencies as $requiredDependency) {
    foreach ($usedSymbols as $usedSymbol) {
        if ($requiredDependency->provides($usedSymbol)) {
            $requiredDependency->markUsed();
            continue 2;
        }
    }
}

@icanhazstring
Copy link
Member Author

Current having a problem with nested yield from of simple array. Might be related to php 7.4.6 bug with yield from.

Todo:

  • check against 7.4.7
  • reduce nesting of generators

@icanhazstring
Copy link
Member Author

TIL: you can't reuse a generator 😅

That was the problem. Using yield from on an already used generator leads to an error. So I currently used iterator_to_array as a quick fix to further implement the symbol scan. Which works fine for now.

@Jean85
Copy link
Contributor

Jean85 commented Aug 1, 2020

Quick question: do you expect performance decrease from this change?

@icanhazstring
Copy link
Member Author

icanhazstring commented Aug 1, 2020

Hi @Jean85. Good question. For the most part there would be not much difference.

The following steps are done

  • check on namespace (psr0/4)
  • scan files for symbols

With this in mind, most packages I could find are defining a namespace they provide. Only for special packages that only define a classmap or simply files, they would have more impact.

But I am trying to avoid that impact using generators. So the files will be only parsed when needed.

@icanhazstring
Copy link
Member Author

Also, the current idea is to have an option in place to activate the new behavior for people to try it out. So everything else works as before.

This way I can optimize the new implementation in different scopes.

Andreas Frömer added 8 commits September 16, 2020 17:04
This will be used with the new symbol matching
implementation

Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
This is to avoid a full scan of the complete package
structure which is not necessary. It represents the same
logic as before with composer-unused.

Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
This will provide a generator with all symbols for a given composer package

Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Andreas Frömer added 15 commits September 16, 2020 17:05
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Reduce the amount of used symbols by avoiding duplicates.

Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
This will be used for better console logging or logging
in general.

Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Andreas Frömer added 2 commits September 16, 2020 17:12
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
@icanhazstring
Copy link
Member Author

icanhazstring commented Sep 22, 2020

Noticed another problem that might come up and should be considered:
According to the composer documentation (https://getcomposer.org/doc/04-schema.md#classmap) autoload.classmap can have not only folders but files and folders with wildcards.

So the current implementation on src/Symbol/Loader/FileSymbolLoader.php does not cover all possibilities and should be covered by tests for these cases. One consideration might be to use glob to resolve the files and parse them all. But there might be a problem with glob and phar execution (which I had before)

Andreas Frömer added 3 commits September 23, 2020 23:52
Resolve issue with used extension symbols

Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
This was added to prevent stacking up symbols from previous
analysed packages.

Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
Signed-off-by: Andreas Frömer <andreas.froemer@check24.de>
@icanhazstring icanhazstring changed the base branch from main to 0.8.x October 16, 2020 14:25
@icanhazstring icanhazstring marked this pull request as ready for review October 16, 2020 14:32
@icanhazstring icanhazstring merged commit 67fe3db into 0.8.x Oct 16, 2020
@icanhazstring icanhazstring deleted the feature/symbol-scanning branch October 16, 2020 14:33
@icanhazstring
Copy link
Member Author

Merged the current work into 0.8.x dev branch.
I will create some issues that need to be resolved before this feature can go silent live.

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 this pull request may close these issues.

None yet

3 participants