Using Object::Pad and perlcritic

[hakonhagland]

When using Object::Pad, for example this program:

Notice the blue lines under lines 1 and 7, when I hover the mouse over those lines I get

and

[bscan]

Thanks for the report. These are great issues to solve. In general, the issue is that PPI does not support modern keyword-based extensions like Object::Pad. There is movement in that repo (in Perl-Critic/PPI#273 ), but that is more about signatures and try/catch, so it might be a while before full support for Object::Pad arrives. More specifically, you'll see these errors even if you directly call perlcritic and remove the Navigator from the mix entirely

For the first issue (Code not contained in explicit package), it may help to follow the Object::Pad file layout suggestions shown here, where they also specify a package in addition to the class: https://metacpan.org/pod/Object::Pad#File-Layout . I believe this recommendation is primarily for Perl::Critic. However, I'm also in support of trying to work around these issues.

For example, using an async sub or a method previously resulted in similar issues to the Module does not end with "1". problem. Currently, these are solved by pre-processing the file in https://github.com/bscan/PerlNavigator/blob/main/server/src/perl/criticWrapper.pl . The same script also pre-processes for UTF-8 support (PPI does not support UTF-8). It would be reasonable to add some additional pre-processing to address both of the issues you've mentioned here.

Pull requests more than welcome, or I'll otherwise eventually get around to it. Fixing this before Perl 5.38 would be great, since that's when use feature 'class' will be part of core, and I assume it suffers from the same problems.

[hakonhagland]

These are great issues to solve.

@bscan Thanks for the positive reply, I will take a look.

[bscan]

Thanks for digging in here. I was thinking about the issue a bit more. Pre-processing the source code is one route. Another idea could be dynamically disabling policies. For example, we may find that RequireEndWithOne is currently not possible to get working. So, we could detect that Object::Pad is "use'd", and then remove the policy by adding it to the exclude list. There may be many other policy/features combinations as well that don't work.

[bscan]

Sorry, accidentally closed.

[hakonhagland]

it may help to follow the Object::Pad file layout suggestions shown here .... https://metacpan.org/pod/Object::Pad#File-Layout

Good idea. I tried this: first removing use strict; use warnings; and inserting a package Foo; declaration

this still gives critique RequireExplicitPackage, but exchanging the order of the two first two lines seems to work:

[hakonhagland]

Line 5 ... Critic: Module does not end with "1;" (Modules::RequireEndWithOne)

This can be fixed by placing a semi colon after the ADJUST block:

[hakonhagland]

Added a question on stackoverflow.com to get some more feedback: https://stackoverflow.com/q/75760912/2173773

[bscan]

Awesome, thanks! I'm curious what the response will be.

There was some similar discussion here: Perl-Apollo/Corinna#95 , but this was more related to the design of the Object-Oriented system than a precise style recommendation for dealing with Perl::Critic.

I also looked into the Modules::RequireEndWithOne issue a bit. The key from the Object::Pad documentation is:

Note that an ADJUST block is a named phaser block and not a method;

I suspect PPI is parsing this as calling a sub named ADJUST that takes a codeblock as a parameter, which is a statement that requires a semicolon.

However, PPI considers a phasers as an "implied end", which do not need a semicolon.
https://github.com/Perl-Critic/PPI/blob/93d6cb20accb627bb3d36cfce65d54168a091e71/lib/PPI/Lexer.pm#L741

Seems like there are two possible fixes for this problem. Either we map the Object::Pad phasers (BUILD and ADJUST) to the types of phasers that PPI recognizes (e.g. END), or remove the name entirely since bare blocks also parse correctly.

Removing entirely might be better to ensure the word END doesn't sneak into error messages and confuse people.
Note that the following form is also allowed:

ADJUST :params ( :$var1, :$var2, ... ) {

which does appear to parse correctly in PPI when adjusted to:

( :$var1, :$var2, ... ) {

[bscan]

Hi @hakonhagland , thanks again for pushing this forward. I spent some more time looking at this. Given the conflicting information about proper file layout, and the StackOverflow answers, I think it best if the Navigator tolerated all the different forms, as they'll all be common and not really "wrong". Trying to organize my thoughts and document reasons for changes, I put together a checklist of various changes I think are needed. I'm happy to do them, or I'd gladly accept pull requests if you're looking at any of them already.

• Get Modules::RequireExplicitPackage to allow Object::Pad to start a file. Specifically, https://github.com/bscan/PerlNavigator/blob/main/server/src/perl/defaultCriticProfile should have the following lines added to support use Object::Pad and use feature 'class'

[Modules::RequireExplicitPackage]
allow_import_of = Object::Pad feature

• While we're at it, might as well add this too, in case someone has this policy installed (not installed by default). There might be other policies worth thinking about as well, but this is a good start.

[Subroutines::ProhibitCallsToUndeclaredSubs]
exempt_subs = Object::Pad::class Object::Pad::field Object::Pad::role Object::Pad::has 

• I also noticed that syntax highlighting is incorrect when the use Object::Pad statement occurs prior to the package declaration. This can be seen in your images as well where class and ADJUST are only highlighted in some examples. I believe simply removing the line "end": "(\\b)(?=^\\s*package\\s*[\\w:]+\\s*;)", from perl-scopes.json will cause the Object::Pad highlighting scope to extend until the end of the file instead of resetting with each new package.

• criticWrapper.pl needs to be able to detect usage of Object::Pad, so we can then pre-process files only when it's in use. This regex can probably be re-used:

  PerlNavigator/syntaxes/perl-scopes.json

  Line 61 in 31b6d7b

  "begin": "(?<=use)\\s+(?=(?:Object::Pad|feature\\s.*class.*|experimental\\s.*class.*|Feature::Compat::Class)[\\s;])",

• Preprocess file in criticWrapper.pl to remove phaser titles (ADJUST, ADJUST :params, ADJUSTPARAMS, and BUILD) when Object::Pad or feature class are in use.

• Preprocess file in criticWrapper.pl to modify class Foo -> package Foo. I think this will help for RequireExplicitPackage and RequireFilenameMatchesPackage. If someone only uses "class" and not "package", it's still a valid usage and I think perlcritic should accept it. class also creates a package with the same name, so I believe this transformation is valid.

• Preprocess file to account for lexical subs like method $foo.

Thanks!

[hakonhagland]

I put together a checklist of various changes I think are needed.

@bscan Looks great to me!

I put together a checklist of various changes I think are needed. I'm happy to do them, or I'd gladly accept pull requests if you're looking at any of them already.

Great! Please go ahead, sorry I have a busy schedule so I will probably be very slow if I should do any of these :) But let me know if you want me to have look at any of the items on your list or some other issue with PerlNavigator within a longer time frame (not so urgent issues).

[bscan]

The updates for Perl::Critic and syntax highlighting should are now available in the Marketplace (and on NPM) in version v0.5.5. Normally will automatically update to the latest version, but may require reloading vscode. I also added some code to allow for lexical methods like method $foo.

@hakonhagland, thanks again for your help on this one. I think this helps substantially toward the goal of making vscode with the Perl Navigator the best development environment for modern Perl.