Skip deprecated

[brightbyte]

Allow deprecated code to be ignored

When refactoring a framework or library to improve layering, old code often cannot be removed immediately. Instead, it has to be marked as @deprecated, and kept around for a relase or two. When that is the case, we generally want to be able to ignore violations triggered by deprecated code, since we already know that we will remove it as soon as possible, and we want to focus on making the new code clean. Sometimes it's not even possible to avoid the offending dependency in the deprecated code, for structural reasons - that may well have been the reason for deprecating that code in the first place.

Handling of deprecation tags has been implemented following the example of isInternal. To avoid awkward method signatures with several booleanc params, ClassLikeReference has been changed to hold an array of tags, instead of a separate isDeprecated flag in addition to the existing isInternal flag.

TODO: Ignore code in deprecated methods, not just classes that are deprecated in their entirety.

NOTE: this goes on top of #1318

[patrickkusebauch]

I understand and follow the reasoning behind this feature and I think it is a good one. What I am uncertain about is this - why is this "native" solution preferable to writing an extension rule?

[patrickkusebauch]

I like the idea of exposing all annotations/tags at ClassLikeReference level. This would be sufficient for anybody to write the required rules themselves. In such a case we should collect all the available tags/annotations not just specific ones.

[brightbyte]

I like the idea of exposing all annotations/tags at ClassLikeReference level. This would be sufficient for anybody to write the required rules themselves. In such a case we should collect all the available tags/annotations not just specific ones.

Yes, I have been thinking in the same direction... that way, would could also apply layers based on a regex matching the value of a certain tag, etc.

The reason I went for a native solution is that I didn't see a way to achieve this with an extension. I'm still new to this code base, I only started to look at it two days ago.

Exposing all tags on the ClassLikeReference sounds like a good idea. Alternatively, perhaps there could be an event triggered inside FileReferenceVisitor that would allow extensions to extract edditional information from the AST.

[patrickkusebauch]

I am happy to direct you there. The event that extensions can process is the ProcessEvent class. It has a dependentReference: https://github.com/qossmic/deptrac/blob/4bb7ddc8eefaca00e57c2d4e041e30bd020b5f07/src/Contract/Analyser/ProcessEvent.php#L25C32-L25C32

This interface, in turn, exposes the underlying token: https://github.com/qossmic/deptrac/blob/4bb7ddc8eefaca00e57c2d4e041e30bd020b5f07/src/Contract/Ast/TokenReferenceInterface.php#L14C43-L14C43

Therefore, TokenInterface would likely be the best place to expose it: https://github.com/qossmic/deptrac/blob/main/src/Contract/Ast/TokenInterface.php

[brightbyte]

Therefore, TokenInterface would likely be the best place to expose it: https://github.com/qossmic/deptrac/blob/main/src/Contract/Ast/TokenInterface.php

That interface is also used for some enum types. I don't have experience with enums in PHP... is it OK to add more stuff to that interface? Also, I have been trying to understand the relationship with TokenType - it seems to me like every instance of TokenInterface would have a TokenType, so the interface should have a getType() method... but that's not the case, and there is only three token types, but five implementation of TokenInterface.

In other words, I'm a little confused about what exactly TokenInterface represents.

[brightbyte]

I understand and follow the reasoning behind this feature and I think it is a good one. What I am uncertain about is this - why is this "native" solution preferable to writing an extension rule?

One thing that I want from theis feature is ignoring dependenciesthat originate from deprecated code. This PR just allows deprecated classes to depend on anything, and that could be done with a rule set just as well.

However, consdier the following scenario: Class A depends on class B, even though it should not. The reason is code in the methods A::foo. I want to be able to ignore this dependency if A::foo is deprecated. That is, I want to skip the analysis of deprecated code, so it has no impact at all.

What would be the beast way to achieve this?

[patrickkusebauch]

However, consdier the following scenario: Class A depends on class B, even though it should not. The reason is code in the methods A::foo. I want to be able to ignore this dependency if A::foo is deprecated. That is, I want to skip the analysis of deprecated code, so it has no impact at all.

We don't have an answer for this yet. Not on the level of individual deprecated methods. At the level of class, sure.

[patrickkusebauch]

Also, this is hard to review as there are 2 PRs mushed together. The skipDeprecated and Internal tag. Can you rework the PR, so that the skipDeprecated functionality is isolated there?

[brightbyte]

Also, this is hard to review as there are 2 PRs mushed together. The skipDeprecated and Internal tag. Can you rework the PR, so that the skipDeprecated functionality is isolated there?

Yea, Github doesn't do stacked PRs (or does it?). Since this needs the functionality of the other PRs, the only way I know to clean this up is to way until the others are merged, and then rebase. Is there a better way? I don't use Github much, we use Gerrit at Wikimedia...

[patrickkusebauch]

There are ways. The best one I found is to target this PR to the other PRs branch instead of main.

[brightbyte]

There are ways. The best one I found is to target this PR to the other PRs branch instead of main.

Hm, yes... but it seems like I can't pick another PR as the base branch. I can only pick branches that exist in this repo, not branches that exist in my fork. Am I missing something?

[patrickkusebauch]

Hmm yes, I see, both the source and the target are in the fork, so that does not work. You could do the PR in your fork then, I would be happy to review it there, but I am not sure if that would be of value.

[patrickkusebauch]

Just a few small adjustments, nothing major and it is good to go.

[patrickkusebauch]

This should be a function on the AnalyserConfig, not on DeptracConfig.

[patrickkusebauch]

This would not even work when an analyser would be present as it would get overridden by the analyser serialized to array.

[patrickkusebauch]

please make it final.

[patrickkusebauch]

Is this supposed to be FromDeprecatedHandler?

[patrickkusebauch]

I will leave it up to @gennadigennadigennadi if this is something we want in the core of deptrac. The support is there for any user to implement it themselves as an extension. The question is if we want this to exist out of the box.

[brightbyte]

I will leave it up to @gennadigennadigennadi if this is something we want in the core of deptrac. The support is there for any user to implement it themselves as an extension. The question is if we want this to exist out of the box.

I am looking into implementing this in a way that would allow deptrac to ignore depenndencies in deprecated methods as well. As far as I can see, the current extension mechanism doesn't allow that. We'd need a new kind of event, and even then, it would be fiddly.

I will try to come up with a solution based on modifying FileReferenceVisitor. Then we can look into how the same effect could be achieved with an extension.

My main problem right now is that method declarations are processed in FunctionLikeExtractor, there is no higher-level object representing them. So I have no place to put the information that they are deprecated.

[brightbyte]

@patrickkusebauch different question: Looking at FileLikeExtractor, I see this code for processing parameter types:

            if (null !== $param->type) {
                foreach ($this->typeResolver->resolvePHPParserTypes($typeScope, $param->type) as $classLikeName) {
                    $referenceBuilder->parameter($classLikeName, $param->type->getLine());
                }
            }
        }

This doesn't look like it takes into account type declarations from doc tags, only "real" type hints. Am I missing something?

[brightbyte]

I am abandoning this in favor of #1359.
I'm still interested in exploring the possibility of implementing this as an extension, btu I don't see a way to do this right now. The new PR implements a proof of concept that may serve as a basis for discussion.