Internal - Refactor AstRunner to support parsing more than classes

[patrickkusebauch]

Right now the whole AstRunner part of the codebase presumes that we are parsing classes only. This assumption runs deep across the codebase.

Right now we have an issue requesting support for parsing function https://github.com/qossmic/deptrac/issues/331 Also, there is another issue requesting support for parsing global variables/superglobals https://github.com/qossmic/deptrac/issues/473

After a Slack discussion with @dbrumann, I created this issue, where we can discuss what/how needs to be changed.

cc @smoench, @slde-flash we would welcome your input on this, as you are more familiar with this part of the codebase.

[smoench]

In theory it shouldn't be that hard. The analyzing part always starts with a file (AstFileReference) where all class references (AstClassReference) are being added. We could add also additional information to the AstFileReference such as functions or global variables. PHPParser gives us the information we just need to collect them. We probably have to add two visitors for analysing functions and global variables. The FileReferenceBuilder needs to be extended to adapt those information.

[patrickkusebauch]

From what I saw this would be exactly my approach as well. My biggest issue right now is with Qossmic\Deptrac\AstRunner\AstMap\ClassLikeName. It is heavily used in defining dependencies and also in processing output. This would need to change and dependencies would have to be more agnostic about the types. Only then I think we can add the new visitors and have them properly propagated into the rest of the package. As since it would be quite a big change a @dbrumann took over the maintenance only recently, he wanted for you @smoench to weigh in on this.

[patrickkusebauch]

I would even argue that already we should have 2 visitors. The ClassReference we have now and UseReference.

Counting use statements is another chapter in and of itself.

• The implementation is problematic as mentioned https://github.com/qossmic/deptrac/pull/571#issuecomment-862087004. It is also bundled with ClassReferenceVisitor while having absolutely nothing to do with the class. It only affects the scope. I could see at most - FileA depends on ClassB (from FileB) via use statement
• It is "near impossible" to resolve correctly - even Nikic/PHPParser does not make the distinction between namespace and class use https://github.com/nikic/PHP-Parser/blob/master/lib/PhpParser/Node/Stmt/Use_.php#L15. This resulted in the issue https://github.com/qossmic/deptrac/issues/536

• It is not intuitive for users as demonstrated by https://github.com/qossmic/deptrac/issues/569, where I completely understand that if I have exactly 1 dependency, I would expect it to be fully qualified usage of another class and not unused use statement.

  These are just examples. I would therefore argue for not ever counting use statements. That would be a breaking change though.

  All of the issues (about use statements and others) be IMHO resolved following steps:

  1. using dedicated *Visitors.
     • ClassReferenceVisitor - remove counting use statements
     • UseReferenceVisitor - count use statements separately
     • FunctionReferenceVisitor - function definitions
     • SuperglobalReferenceVisitor - usage of superglobals.
  2. Expanding AstFileReference to include the result of these Visitors.
  3. Rewrite DependencyInterface and BasicEmitter to work with general "name tokens" instead of ClassLikeName

[dbrumann]

I just noticed Tim is probably no longer using his old handle, so another friendly ping to @timglabisch for your thoughts on this

[timglabisch]

as far as I remember this is not so trivial. but quite feasible. if I remember correctly you would have to exchange AstClassReference for something that works with the FQN.

For example if you would have a file like:

<?php

namespace foo\Bar;

function abc() {

}

you could map this to foo\Bar\abc().

this get ultra complicated if you would like to support something like this:

`

<?php

namespace foo\Bar;

return function() {

}

// or

return $someVar;

any good examples why this feature is important?

[patrickkusebauch]

you could map this to foo\Bar\abc().

This is exactly how I imagine the "name token" would look like for the function

this get ultra complicated if you would like to support something like this:

I agree. But I don't think this behaviour is supported even for (anonymous) classes. IMHO there is no reason for functions to be exceptional in that regard.

any good examples why this feature is important?

For variables, I think superglobals are quite an important use case. For functions:

• Functional programming paradigm written applications in general
• named function are also often created to substitute missing language features.

[timglabisch]

@patrickkusebauch can you give me an example for the superglobal case?

[patrickkusebauch]

@timglabisch Sure. For example, in a "traditional" MVC/MVP architecture, I might want to constrain the usage of GET/POST/COOKIE/FILES/REQUEST superglobals to Presenters only. While I might not mind if ENV or SERVER are accessed in the Model.

[timglabisch]

i am not sure if we really need support for superglobals, deptrac is primary written for well structured codebases to keep an eye on the layers in larger projects. for ages i didn't saw a superglobal in the wild, most times frameworks just make sure you don't access them. From my perspective i would try to use tools like psalm to make sure that really no one is using superglobals at all.

[patrickkusebauch]

I tend to agree. I only use superglobals in the bootstrap file of the framework, when I need to change configuration based on (mostly) ENV values. Otherwise, I am accessing via the framework. I am just being the devil's advocate for the open issue here.

[timglabisch]

but supporting functions and constants would be a great addtion.

here are some examples:

const FooXXXXX = "bar";

class FooXXXXX {

}

function A() {
    function B() {}
}

trap: FQN for the Class and the Constant could overlap.

[patrickkusebauch]

Either way, if we want to support anything else other than classes, it requires rewriting DependencyInterface and BasicEmmiter as outlined above. Then it can be added to AstFileReference

[patrickkusebauch]

Anyways, to move this forward a little bit. Does anybody see anything wrong with this approach?

1. Create new TokenLikeName interface for ClassLikeName
2. Make DependencyInterface and BasicEmmiter use this interface instead of ClassLikeName
3. For each new supported type add:

   
   class (Function/Constant/Superglobal)LikeName implements TokenLikeName
   {
   public function toString():string {
   return ''; //text representation of the token for console output
   }
   }

class (Function/Constant/Superglobal)ReferenceVisitor extends NodeVisitorAbstract {}

and add the result of the visitor to `AstFileReference`?

I think this could work quite nicely. WDYT?

[dbrumann]

I think, we are all in agreement that supporting functions/constants would be nice and if superglobals easily come with and then I won't mind.

My only requirement is, that we do this in at least 2 steps:

1. Introduce the new interface and use that
2. Introduce the functionality one by one (in any order):
   1. function
   2. constant
   3. superglobals

As far as I can tell, introducing the interface should not have any BC impacts, at least not for what we consider BC, e.g. changing the commands in any user facing way (input, output) or changing the depfile. It would not be the end of the world, but that is something I would like to avoid with such a big change to make it easier to compare when we notice any side effects.

[MGatner]

Just chiming in to say: I would use this heavily as well! View files are "procedural" but (in my case) should have some of the tightest restrictions. I'm just getting started with deptrac and a refactor seems like it would be over my head, but let me know how I can help!

[patrickkusebauch]

@MGatner I am already working on this heavily, so don't worry it is coming. To me, the biggest help would be unintuitively you picking up other issues. If I can focus solely on this, I would finish it faster than me working on several issues simultaneously.

[MGatner]

@patrickkusebauch Loud and clear! Still digging in over here but once I have my head around it I will take a peek at what is out there. Thanks for your work, I look forward to what you've got!

[patrickkusebauch]

Suggested token names to avoid confusion in the output:

class     - as is
function  - functionName()      <-- note the bracket at the end
variable  - $variableName       <-- note the dollar sign at the beginning