Ability to add custom `PhpParser\NodeVisitor` to alter AST attributes

[janedbal]

Currently, we are using custom ReferenceExtractorInterface which needs some extra attributes to be present in PhpParser\Node (similar to what NodeConnectingVisitor adds). Currently, there is no way to add it, so I was forced to copy most logic from NikicPhpParser in our custom ParserInterface.

It would be much better if I could just tag my visitor to be added automatically (similar to what PHPStan supports):

-
    class: PhpParser\NodeVisitor\NodeConnectingVisitor
    tags:
      - node_visitors

[patrickkusebauch]

Would you mind going into more detail about what are you trying to achieve? Ideally with some example? It would help me out with designing the interface that you would need to solve your problem.

[janedbal]

I mean something like this: https://github.com/qossmic/deptrac-src/pull/21

[patrickkusebauch]

Ok, I understand what you want, I just don't understand why you want it.

What is the use of a NodeVisitor without a ReferenceExtractor to take advantage of it? And once you do, what is the point of having an extra reference without a DependencyEmitter to convert the reference to a dependency?

That's why I am asking what problem are you trying to solve. You are providing me with a solution, but I have no idea to what problem.

[janedbal]

I want some usages not to be treated as dependency. We have custom implementation of "friend methods":

#[Friend([OnlyAllowed::class, 'caller'])]
public function method(): void
{
}

And those references are just "backreferences" of allowed callers. Not a real dependency. So in order to exclude those, I wrapped your ClassConstantExtractor with ours. And that one need to know context of where the ClassConstFetch is used. Hence the NodeVisitor that provides that.

[patrickkusebauch]

A PR (https://github.com/qossmic/deptrac-src/pull/11) introduces DependencyContext to provide metadata about where the dependency occurred. It expands on the DependencyType and FileOccurence value objects. Would this work for you? If it provided information like: "Occurred within an attribute", "In a method attribute", or "As an attribute parameter". Which of those would you need? Would it cover your use case? Do you have other use cases?

I am wary of introducing more extension points, rather would better utilize the existing ones. As you have already alluded, you had to wrap ClassConstantExtractor, now you also have to add a NodeVisitor, that seems like a lot of work, that also requires to know lot of the deptrac internals, I would like to avoid that if possible.

[janedbal]

"Occurred within an attribute" is not enough, I need to target that specific "Friend" one.

[janedbal]

But maybe my usecase is so edgecase it does not need to be supported with an easy way. Hard to guess though..

[olsavmic]

I would argue that the proposed https://github.com/qossmic/deptrac-src/pull/21 is a sweet spot between introducing a public extension point in the Deptrac API, and overriding the internals (which is something we have to do right now - overriding the PhpParser implementation), or, at the extreme case, forking the repo.

I'd say that @janedbal's use-case is a very special requirement, but at the same time, I see very little reason NOT TO allow the advanced users such customizations. It doesn't necessarily become a public API of deptrac, we're totally OK with taking the risk of internals changing. But it allows to easily add necessary customizations for large codebases with custom rules, that can later be converted to supported configuration options in deptrac :)