PHPDoc types sniff

[james-cnz]

This is a replacement for the old Moodle PHPdoc check (moodlecheck) checks relating to PHPDoc types. It's a port of https://github.com/moodlehq/moodle-local_moodlecheck/pull/120 .

It's a significant improvement over the old checks (at least last time I looked at them). It supports DNF types, and checks types for validity, recognises namespaces, use aliases, and inheritence and implementation of classes and interfaces in the current file.

I've pulled across the PHPDoc type specific fixtures from the old checker, and my checker reports the contained issues, although not with the same messages. I'm not sure if there's some standard to be followed with error messages and codes?

Is someone else OK with removing the corresponding checks from the old checker, or should I do this?

Things checked

Classes

• Templates names are present and valid.
• Template types, if present, are well formed.

Functions

• Named functions have a PHPDoc comment. (Perhaps this doesn't ultimately belong in the PHPDoc types sniff, but I don't think it's currently checked elsewhere.)
• Template names are present and valid.
• Template types, if present, are well formed.
• Number of parameters matches.
• Parameter types are present and well formed.
• Parameter names are present and valid.
• Parameter types match.
• Parameter splat usages matches.
• Parameter names match.
• There aren't multiple @return tags.
• Return type is present and well formed.
• Return type matches.

Variables

• Class variables and constants have a PHPDoc comment. (Perhaps this doesn't ultimately belong in the PHPDoc types sniff, but I don't think it's currently checked elsewhere.)
• There is at least one @var tag.
• And not more than one.
• The type is well formed.
• The type matches.

Misc

• The PHPDoc types sniff successfully parsed the file. (This may be annoying if multiple sniffs report failing to parse the file, but I think it's better than silently failing, and giving the impression that the sniffs passed.)

Things not checked

• Function parameter pass by reference '&' usage matches. (The old checker disallows this in PHPDoc comments entirely, so requiring it to match would make it impossible to satisfy both checkers.)
• Function return tag is given other than for constructors. (The old checker doesn't check this.)
• Tags (@template, @param, @return, @var) aren't used in inappropriate places. (The old checker doesn't check this.)
• A @const tag isn't used. (I guess this is part of a check for invalid tags generally, and doesn't belong in the PHPDoc type sniff? But in any case, an error will be given because of a missing @var tag, so this is probably enough.)

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 92.71772% with 97 lines in your changes are missing coverage. Please review.

Project coverage is 96.10%. Comparing base (02a279e) to head (43f8ce0).

Files	Patch %	Lines
moodle/Sniffs/Commenting/PHPDocTypesSniff.php	89.97%	73 Missing :warning:
moodle/Util/PHPDocTypeParser.php	96.02%	24 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #123 +/- ## ============================================ - Coverage 97.90% 96.10% -1.80% - Complexity 850 1420 +570 ============================================ Files 37 39 +2 Lines 2524 3856 +1332 ============================================ + Hits 2471 3706 +1235 - Misses 53 150 +97 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[james-cnz]

@andrewnicols

Continuing from #121

Regarding comment tags, I'm being pedantic, and checking that the tags start on a new line. Technically, I think param, return, and var tags within a line don't count, so I'm not sure the list would help me. But the issue is with multi-line types like this:

Sorry, what is this in relation to? I'm not sure of the context of your reply.

It was in relation to:

The phpcs parser already does much of this work for us and if you find the T_DOC_COMMENT_OPEN_TAG tag, you'll find that phpcs includes in its structure an array of tags named comment_tags whose value is the ptr for each of the @ tags in the docblock.

I don't think this would help me much, because I'm not sure it would tell me which tags start on a new line.

The general approach to this would be: identify the start and end of the type store these, along with the concatenated type replace the first type part with the new type replace the rest of them with an empty string

Oh, I guess I could do that. It would mean the fix would create one very long line, losing any formatting they had, and causing a complaint about the line length though. (If I understand the suggestion right.) But if that's acceptable I'll give it a try.

That said, we want to strongly discourage complex objects unless they are a class in their own right. They may be syntactically valid, but they're not a good pattern.

It was a separate class originally, but I converted it to an object because the style checker told me I should only have one class per file. That said, part of it's relevant to the type parser, and part of it isn't, so it wasn't really perfect as a class either.

In fact, the above is not recognised by phpDocumentor in any case. See the attached screenshot.

I've aimed to put in everything that's supported by both PHPStan and Psalm. If there are specific tools that are relevant to Moodle, though, I could remove anything those tools don't support.

[james-cnz]

Hi @andrewnicols,

Thanks for your feedback. I've made several changes.

Done:

• Refactored, so some stuff could be passed between functions, rather than stored in variables.
• Changed the check for multiple calls on the same file, so it won't break autofixing.
• Warn about types that don't conform to Moodle style (short type keywords, and lower case), and support autofixing.
• Made use of token lists.
• Check class property, property-read, and property-write tags.
• Allow PHP attributes in between the PHPDoc comment and the thing it's annotating.
• Check against Moodle master, and required fixes for that.
• Removed checks to be performed in other sniffs.

Still to do:

• Remove support for types that aren't supported by phpDocumentor? (I'll have to have a play with it, I haven't used it before.)
• Increase test coverage.

[james-cnz]

I've had a bit of a look at phpDocumentor, and it looks like it will parse almost everything accepted by the checks, although it doesn't handle everything correctly. So the checks as they are are already doing something useful--they go a long way towards ensuring that phpDocumentor will run. But some things would have to be taken out to ensure phpDocumentor could handle everything correctly.

[james-cnz]

It looks like phpDocumentor has just added support for object shapes: https://github.com/phpDocumentor/TypeResolver/commit/c92bb852fd57093641c99cf7705ba83ea00d3a38

[andrewnicols]

As it happens, I've been starting to look at bringing phpdocumentor/reflector in as part of the existing type hint side of things but I have to put this on the backburner for a week.

So far the issue I've encountered is that the type is normalised during the parsing by reflector, which makes it useless for our purposes.

[james-cnz]

Sorry, not sure I understand. Is this related to #126, or something different?

[andrewnicols]

No, it’s a follow up to that series of issues.

On Tue, 26 Mar 2024 at 16:56, James C @.***> wrote:

Sorry, not sure I understand. Is this related to #126 https://github.com/moodlehq/moodle-cs/pull/126, or something different?

— Reply to this email directly, view it on GitHub https://github.com/moodlehq/moodle-cs/pull/123#issuecomment-2019858230, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC2K76F33SU4MEGQNX5MGDY2EZZ7AVCNFSM6AAAAABE2CJHNOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJZHA2TQMRTGA . You are receiving this because you were mentioned.Message ID: @.***>

[james-cnz]

Sorry, I meant in the same vein. Is it about simplifying types? If so, is this still needed? phpDocumentor seems to be adding more complex types for its next release. Are there other tools that Moodle developers depend on that don't support the complex types?

[andrewnicols]

Is what still needed?

[james-cnz]

Is simplifying the types still needed, now that phpDocumentor is adding support for complex types?

[andrewnicols]

At the moment, yes. That may change in future, but right now we are focused on trying to migrate remaining functionality from moodle-local_moodlecheck here.

Once we have the sniffs migrated, we can start to add more support, and I am looking at using Reflection for that, but it's not something I have time to focus on right now.

[andrewnicols]

See also https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/387

[james-cnz]

Changes:

• Added error recovery
• Less parsing, relying more on phpcs
• Added check for misplaced property-*, param, return, template tags (I didn't do this originally, because the old checker didn't, but I guess there's no reason to stick to what the old checker did?)
• More test coverage
• Checked against phpDocumentor--it seems to have high quality PHPDocs, so good for checking there aren't too many false positives, and fixed some issues found there
• Removed the check for multiple var tags. phpDocumentor has multiple var tags for variables separated by commas, even though there's only one type, so I guess this is OK.
• Added BSD licence, since phpcs uses this, I think it's probably a good thing to do.
• And some tidying

I had a look back over comments, to see if I'd missed anything, and found this:

Aside from everything else... since when are those generics compliant with PHPDoc? [with a link to PHP-FIG's PHPDoc standard.] https://github.com/moodlehq/moodle-cs/pull/121#issuecomment-2007155062

So is compliance with PHP-FIG's standard a requirement? Is there any chance of re-evaluating this, now that phpDocumentor supports more complex types? If not, would it be OK to still check more complex types, but give a warning if they're used? Or leave the code in, but set a constant that disables it? There's quite a bit in the Sniff that's not in the PHP-FIG standard, and I think it would be good to keep it in case it's supported in the future.

[james-cnz]

I'll go ahead and add a warning. That'll provide the most informative messages.

[james-cnz]

I think I've addressed all functionality issues, and I've made an attempt at style issues.

The sniff still does some parsing, to determine how classes and functions are nested, so that relative class types (self, parent, static, $this) can be checked correctly, and so it knows which templates are in scope. It passes over the file, calling functions to deal with classes and functions as they are encountered, storing relevant data locally, and exiting, thereby disposing of that data, when it encounters the ends of the classes and functions. I think this approach is relatively simple, and sufficient for the purpose.

I've added a check that (most) relevant tags don't appear in inappropriate places. Perhaps this doesn't ultimately belong in this sniff, but it's easy to do here, since a consequence of having to know when tags are expected, is that we also know when they're not, and I'm not sure this check is currently performed elsewhere?

The var tag isn't checked to see if it appears in inappropriate places, though. Apparently, besides variables defined in the usual way, and in "define" statements, var tags can also annotate variables defined as the iterator in foreach statements, and those defined by being returned as a pass by reference parameter that wasn't defined before a function call. Checking this properly would add complexity for diminishing returns, I think, so for var tags that don't precede class variables, I've just checked the type is well formed, and left it at that.

[andrewnicols]

Hi @james-cnz ,

I'm not going to have time to review this change in the next few weeks as we're prepping for Moodle 4.4's release.

I will again reiterate that our current focus on this project is to migrate all existing sniffs in their current form from the moodlecheck project to here, fixing any existing bugs along the way. In some cases we may add some small improvements as we do so, but such improvements are not the focus.

I would strongly suggest waiting until we have done this as some of these changes simplify the sniffs, and we are considering whether to make the PHP Documentor Reflector a dependency of this project, which would likely alleviate the need for some of your changes.

[james-cnz]

Hi @andrewnicols,

OK. I guess there's nothing really Moodle specific about this sniff. I wonder if phpcs would be interested in it. If part of the issue is time constraints, would you be more likely to consider it if it was available an an off-the-shelf sniff?