-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: proof-of-concept for feature-tracking and perl sub signatures (see #273) #280
base: master
Are you sure you want to change the base?
Conversation
tempted to just copy in feature.pm from each latest perl release. maybe automatable via dzil? |
@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 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. |
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.
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.
I've added that to the TODO in the first post. :)
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.) |
4fedb89
to
082b961
Compare
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. |
|
e6967be
to
89798db
Compare
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 |
89798db
to
40619b1
Compare
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? |
381ec2f
to
fdf7ac0
Compare
Implemented various ways to enable parsing features. Please see the checklist in OP post as well as the Changes file. |
fdf7ac0
to
1997ddb
Compare
1137ad4
to
33bd9de
Compare
I'm running into an interesting question. What should this parse into?
|
Conversation to the above question continued here: Perl-Critic/Perl-Critic#591 (comment) |
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. |
It's not a hard question, just one of naming. Also my question was slightly erroneous. However 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. |
test_document | ||
<<'END_PERL', | ||
use feature "try"; | ||
try{}catch{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use 5.36.0;
use feature "try";
no warnings 'experimental::try';
try { } catch { }
syntax error at try.pl line 4, near "catch {"
try.pl had compilation errors.
It compiles when the signature is included.
try { } catch ($e) { }
33bd9de
to
6adfe06
Compare
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 Perl-Critic/Perl-Critic#591 for requests from the user-side.
TODO: