feat: Introduce AttributeAnalysis

[HypeMC]

Extracted from #7395

[coveralls]

coverage: 96.016% (-0.02%) from 96.036%
when pulling 89483cd on HypeMC:add-attribute-analysis
into 0e69940 on PHP-CS-Fixer:master.

[Wirone]

@keradus before you comment something like "I don't like stuff without actual usage" - this is for #7395 and #7880 as a common mechanism for achieving 2 separate needs.

@kubawerlos please take a look too, feedback about whether it's possible to build your change on top of this is really welcome 🙂.

[Wirone]

I am wondering if normalisation of FQNs is a responsibility of this analyzer, or not. Imagine:

<?php

namespace Foo;

use Bar\AB;

#[
    AB\Baz(prop: \'baz\'),
    \A\B\Qux(prop: new P\R()),
    Corge,
]
function foo() {}

right now AttributeAnalyzer::collect() returns something like:

new AttributeAnalysis(2, 39, [
    ['start' => 3, 'end' => 12, 'name' => 'AB\\Baz'],
    ['start' => 14, 'end' => 32, 'name' => '\\A\\B\\Qux'],
    ['start' => 34, 'end' => 35, 'name' => 'Corge']
])

and we need to decide if:

• it should take the namespace and imports into consideration, that means collecting:
  • Bar\AB\Baz for first attribute
  • Foo\Corge for second attribute
• it should trim leading backslash, that means collecting A\B\Qux

Personally I think it's out of the scope of this analyzer, as it does not have the context of a namespace it collects attributes in, and even if it's technically possible to determine it, it rather feels more natural if attributes' symbols are returned as-is. From fixers' point of view (e.g. fully_qualified_strict_types) it's even required to distinguish whether class used in attribute contains leading backslash or not, to decide if if should be fixed or not. Maybe we can add normalisedName later to the array returned by analyser, if there's a need for it.

Just thinking out loud, would like to hear your opinion too @HypeMC @kubawerlos .

[HypeMC]

@Wirone Good question. I was wondering about that myself. I think it comes down to how often this feature will be needed, which I admit is hard to know in advance. So far, the only feature that needs this is the OrderedAttributesFixer, and only for a specific case when sorting is done by specifying the exact attributes. Neither #7880 nor #7284 needed this functionality. On top of all that, I agree with you that it seems out of scope for this analyzer, so for now, maybe it'd be best to leave it out. If and when the need arises, the logic can be extracted from the OrderedAttributesFixer.

[Wirone]

I believe that #7880 requires FQCN, but I still think that analyzer should return symbols as-is, and FQCN resolution should be done in higher layer, which uses the analyzer. If fixer uses attribute analyzer and needs to operate on FQCNs, most probably it already has the logic for collecting imported symbols in current namespace. But as you said, it's hard to tell now, fortunately we can add resolved name later if needed without BC break 🙂.

[kubawerlos]

This PHPDoc is not part of the first AttributeAnalysis, while 2 PHPDocs are part of the attributes they precede, is that expected behaviour?

[HypeMC]

Yes, this is intentional. There's really no way to know whether a certain comment or docblock belongs to the attribute or to the entire method. Since it's more common to add a docblock for a method at the beginning, before any attributes, while adding docblocks or comments between attributes isn't, this approach made the most sense to me.

[Wirone]

Thanks @kubawerlos for the review, few minor additions from my side 🙂.

[Wirone]

@kubawerlos do you have any more doubts?

[Wirone]

Hah, one minor thing to improve 😅.

[Wirone]

Thank you very much @HypeMC 🍻!