RFC: proof-of-concept for feature-tracking and perl sub signatures (see #273)

[wchristian]

For the shortest summary, see Changes file.

---

This is a basic implementation of feature-tracking in the parser, with most of the parts that mainly need to be expanded sketched out.

Please review and comment.

~(This breaks most of the other tests. feature_tracking.t is the only relevant test for this proof-of-concept.)~

See #273 for more details and discussion on missing details (configuration, cpan inclusion, etc.) as well as #194 for the historical ticket and discussion, and https://github.com/Perl-Critic/Perl-Critic/issues/591 for requests from the user-side.

TODO:

• [ ] We also need to support the prototype keyword which is needed when signatures are enabled.
  • [ ] It's already recognized as a generic attribute. What else should we do with it? Can for the moment only think of having https://metacpan.org/pod/PPI::Statement::Sub#prototype recognize it.
  • [ ] What is its history, from which versions on is it active?
• [ ] extend the automatic recognition of perl core enabling of features
  • [ ] we need a list of core features that affect parsing
  • [x] MVP-ready, barring veto
• [ ] implement recognition of common CPAN-enabling of features, with an opt-out configuration
  • [ ] we need a preliminary list of common CPAN modules which enable parsing-relevant features
  • [ ] Syntax::Keyword::Try implement, review requested
  • [ ] implemented in PPI::Statement::Include::feature_mods. review requested
  • [x] MVP-ready, barring veto
• [ ] implement common configuration of home-brew-enabling of features, opt-in
  • [ ] implemented in PPI::Document::new( custom_feature_includes => ... ). review requested
  • [x] MVP-ready, barring veto
• [ ] implement a callback hook in possible features-enabling elements
  • [ ] implemented in PPI::Document::new( custom_feature_include_cb => ... ). review requested
  • [x] MVP-ready, barring veto
• [ ] parse signatures as perl code and create the corresponding tree elements

[wchristian]

tempted to just copy in feature.pm from each latest perl release. maybe automatable via dzil?

[toddr]

@wchristian Kudos for taking the initiative on this!!!

If you're up for it, I'd really like to understand why PPI needs to know that signatures are enabled before parsing a prototype into a signature.

In common code, it's rare that you can confuse one for the other. I think the only thing that's ambiguous would be sub foo ( $ ) ... but nobody is going to write that. They're going to write sub foo ( $bar) ... or maybe sub foo ( $bar, $ ) ... in which case it's obviously an attempt at a signature not a prototype.

I know I'm punting the ball down the road but I think we can get away with ignoring features for now?

We also need to support the prototype keyword which is needed when signatures are enabled.

[wchristian]

I'd really like to understand why PPI needs to know that signatures are enabled before parsing a prototype into a signature.

Regarding signatures, i want PPI to also parse the signatures themselves as perl code, so we can have a plugin that checks for unused variables and such recognize their presence in the signature. Meanwhile prototypes are always just a string, because for the most part that's all they are to the perl parser.

Also, as much as possible, i'd like to be correct. If someone wants to have a perl-critic policy of "disallow prototypes", i want PPI to be useful in helping them actually do that.

I think we can get away with ignoring features for now?

We will need to understand features in any case, because they also affect things like keywords, so this is a framework for solving that matter as a whole. I can understand the motivation of skipping that, but i believe we can actually make this work in a manner that is cheap enough to build to do it right now. Note how the framework is already there in the POC, the nitty gritty is only in teaching it how to recognize the activation of a feature, as well as how to parse that feature.

I'd also prefer not to change the way PPI behaves into one thing, to then later have to (partially) revert that behavior, since that seems unpleasant for users.

We also need to support the prototype keyword which is needed when signatures are enabled.

I've added that to the TODO in the first post. :)

This seems to be the huge TBD. How do we know if signatures are enabled?

I've added the answers to that to the TODO in the first post as well. The short version is for now: Several levels of automated as well as configurable recognition of feature-enablers. (Mercifully limited to use/BEGIN.)

[wchristian]

Implemented pre-setting features at the document level to match -M, environment variablesand other possible related things.

Implemented disabling features via feature.pm.

Found that the $Document object needs to be available at the feature decide point in case there are no previous parented tokens to walk the tree on, which currently resulted in modifying a trillion method calls. ~Probably need to make it an attribute on the tokenizer.~ Done.

[wchristian]

~perl 5.8 has no feature.pm apparently?~ Removed that.

[wchristian]

Replaced the tokenizer passing with an attribute on the tokenizer, and kept a backup of the passing in https://github.com/Perl-Critic/PPI/tree/document_tokenizer_passing

[wchristian]

Did some research regarding prototype attribute:

It's already recognized as a generic attribute. What else should we do with it? Can for the moment only think of having https://metacpan.org/pod/PPI::Statement::Sub#prototype recognize it. What is its history, from which versions on is it active?

[wchristian]

Implemented various ways to enable parsing features. Please see the checklist in OP post as well as the Changes file.

[wchristian]

I'm running into an interesting question. What should this parse into?

use feature 'signatures'; sub meep($) {}

[wchristian]

Conversation to the above question continued here: https://github.com/Perl-Critic/Perl-Critic/issues/591#issuecomment-1367451116

[oalders]

Maybe we could get some thoughts on this PR from @trwyant and @Grinnz?

[Grinnz]

I have given my thoughts in #194 and they have not changed. Particularly because of questions like the above and because there are infinite ways to enable the signature feature in a scope, I think it will always be required to parse a syntax where its nature cannot be programatically determined.

[wchristian]

It's not a hard question, just one of naming.

Also my question was slightly erroneous. ($) is a valid signature and a valid prototype, so it'll parse according to active context as per the PR code.

However (&) and friends are more interesting, as that is not a valid signature, but a valid prototype, which however causes an exception when ran in perl.

That said, have you read the code and got comments on it? A valid question to ask i think as the code didn't exist when i closed #194.