Login
RT #79289: False Postive in Perl::Critic::Utils::is_in_void_context()
[gknop/Perl-Critic.git] / TODO.pod
1 # best viewed via "perldoc TODO.pod"
2
3 =pod
4
5 =for stopwords API LHS RHS REFACTORINGS FH SVN stopwords
6
7 =head1 NAME
8
9 Perl::Critic::TODO - Things for Perl::Critic developers to do
10
11 =head1 SOURCE
12
13          $URL$
14         $Date$
15       $Author$
16     $Revision$
17
18
19 =head1 SEE ALSO
20
21 Perl-Critic-More is a separate distribution for less-widely-accepted
22 policies.  It contains its own TODO.pod.
23
24
25 =head1 NEW FEATURES
26
27 =over
28
29 =item * Report PBP and Safari sections in addition to PBP page numbers.
30
31 Something like
32
33     Readonly::Scalar my $EXPL => {
34         pbp_pages       => [ 57 ],
35         pbp_section     => '5.2',
36         safari_section  => something,
37     };
38
39 =item * Include PBP references and Safari sections in the documentation.
40
41 Perhaps these could be injected into the POD at build time, based on the data
42 in the code.  But that data is not entirely static, so I'm not sure how it
43 would work for Policies that dynamically determine the PBP references.
44
45 Perhaps it would be good enough to just create a one-off tool that would
46 inject the PBP and/or Safari references into the POD one time, and we could
47 manually deal with Policies that behave oddly.
48
49 Much better would be to put the information in the POD in a structured manner
50 and parse it out in the code, something along the lines of
51
52     =head1 METADATA
53
54     =over
55
56     =item Default Themes
57
58     core bugs pbp
59
60     =item Default Severity
61
62     3
63
64     =item Perl Best Practices Page Numbers
65
66     193, 195
67
68     =back
69
70 and so on.
71
72
73 =item * Add a file Parameter::Behavior.
74
75
76 =item * Allow values of (at least) string-list Parameters to be specified in a file.
77
78 For the benefit of PodSpelling, etc.
79
80
81 =item * Enhance string-list Behavior to allow specification of delimiters.
82
83 For things like RequirePodSections.
84
85
86 =item * Add queries to --list option to F<perlcritic>.
87
88 List Policies based upon severity, theme, and (what I want this second)
89 applies_to.
90
91 =item * Add formatting of --list output.
92
93 Support Jeff Bisbee's use case (he dumps all the policies in severity order
94 with full descriptions and other metadata).
95
96 =item * Support for C<#line 123 "filename"> directives.
97
98 For code generators and template languages that allow inline Perl code.
99
100 Yes, somebody has an in-house templating system where they've written a custom
101 test module that extracts the perl code from a template and critiques it.
102
103 Actually, this would be useful for programs: Module::Build "fixes" shebang
104 lines so that there's the bit about invoking perl if the program is attempted
105 to be run by a Bourne shell, which throws the line numbers off when using
106 Test::P::C on the contents of a C<blib> directory.
107
108 This has been implemented in PPI, but hasn't been released yet.  When it does
109 come out, we need to change the line and file reported by Violations.
110
111
112 =item * Enhance statistics.
113
114 - Blank line count
115
116 - POD line count
117
118 - Comment line count
119
120 - Data section count
121
122 Proposed implementation committed 15-Mar-2007 by wyant, about revision 3240.
123
124
125 =item * Detect 5.10 source and enable stuff for that.
126
127 For example, treat C<say> as equivalent to C<print>.
128
129 =item * Detect 5.12 source and enable stuff for that.
130
131 Yes, this is long-term, and is really a list of stuff from 5.011 to enable if
132 it makes it into 5.12, gleaned from the perl511xdelta files:
133
134 'use 5.011;' implies 'use strict;' and 'use feature qw{ :5.11 };' per
135 perl5110delta.
136
137 'sub foo { ... }' (yes, with the subroutine body being an elipsis a.k.a. the
138 'yada yada' operator) compiles but fails at runtime per perl5110delta. PPI
139 seems to parse this sanely as of 1.206.
140
141 'package Foo 1.23;' is equivalent to 'package Foo; our $VERSION = 1.23;' per
142 perl5111delta. PPI seems to parse this sanely as of 1.206.
143
144 Nothing additional found in perl5112delta, which is the most recent as of the
145 addition of this item.
146
147 =item * Detect 5.14 source and enable stuff for that.
148
149 5.13.7 allows references in many places where arrays or hashes used to
150 be required (e.g. C<push $stack, 'foo'> where C<$stack> is an array
151 ref). Not sure what policies are affected.
152
153 Lexical regular expression modifier defaults via (e.g.)
154 C<use re '/smx'>). This also interacts with
155 C<use feature 'unicode_strings'>. 5.13.7.
156
157 =item * Support a means of failing if a Policy isn't installed.
158
159 For example, the self compliance test now depends upon a Policy in the More
160 distribution.
161
162 Something like using a "+" sign in front of the Policy name in its
163 configuration block, analogous to the "-" sign used for disabling a policy,
164 e.g. "C<[+Example::Policy]>".
165
166
167 =item * Threading
168
169 Pretty obviously, Perl::Critic is readily parallelizable, just do a document per
170 thread.  ("readily" being conceptual, not necessarily practical)  Although
171 there's now C<Policy::prepare_to_scan_document()>, given perl's thread data
172 sharing model, this shouldn't be an issue.
173
174
175 =item * Add support in .run files for regexes for violation descriptions.
176
177 =item * Add support for "## use critic (blah)".
178
179 If I've got:
180
181     ## no critic (SomePolicy)
182
183     ...
184
185     ## no critic (ADifferentPolicy)
186
187     ...
188
189     ## no critic (YetAnotherPolicy)
190
191 If I want to turn C<YetAnotherPolicy> back on but neither C<SomePolicy> nor
192 C<ADifferentPolicy>, I've got to do this:
193
194     ## use critic
195     ## no critic (SomePolicy, ADifferentPolicy)
196
197 Why can't I do this:
198
199     ## use critic (SomeOtherPolicy)
200
201
202 =item * Make color work on Windows.
203
204 Use L<Win32::Console::ANSI> like L<App::Ack>.
205
206
207 =item * Create P::C::Node and make P::C::Document a subclass and make use of PPIx::Utilities::Node::split_ppi_node_by_namespace() to provide per-namespace caching of lookups that are now on P::C::Document.
208
209 This is necessary to get P::C::Moose Policies correct.
210
211
212 =item * Use L<version|version> to declare C<$VERSION> numbers throughout P::C
213
214 PBP recommends using the L<version|version> module.  I chose not to follow that
215 recommendation because L<version|version> didn't work with the Perl v5.6.1 that I had
216 at $work at that time (and I really wanted to use Perl::Critic at work).
217 But now the L<version|version> has been updated and those bugs may have been fixed,
218 or perhaps we just don't care about running on Perl v5.6.1 any more.  So
219 maybe now we can go ahead and use L<version|version>.
220
221 =back
222
223
224 =head1 BUGS/LIMITATIONS
225
226 Document bugs for individual Policies in the Policies themselves.  Users
227 should be aware of limitations.  (And, hey, we might get patches that way.)
228
229
230 =head1 OTHER PBP POLICIES THAT SEEM FEASIBLE TO IMPLEMENT
231
232 =over
233
234 =item * Modules::RequireUseVersion [405-406]
235
236 =item * Modules::RequireThreePartVersion [405-406]
237
238
239 =item * NamingConventions::RequireArrayAndHashReferenceVariablesEndWith_Ref [41-42]
240
241 Check for C<$variable = [...]>, C<$variable = {...}>, C<< $variable->[...] >>, and
242 C<< $variable->{...} >>.
243
244
245 =item * Objects::ProhibitRestrictedHashes [322-323]
246
247 Look for use of the bad methods in Hash::Util.
248
249
250 =item * Objects::ProhibitLValueAccessors [346-349]
251
252 Look for the C<:lvalue> subroutine attribute.
253
254
255 =back
256
257
258 =head1 NON-PBP POLICIES WANTED
259
260 =over
261
262 =item * Subroutines::RequireArgumentValidation
263
264 Enforce the use of Params::Validate or Params::Util or some other
265 argument validation mechanism.  This could be one Policy that
266 can be configured for different validation mechanisms, or we could
267 have a separate Policy for each mechanism, and let the user choose
268 which one they want to use (I think I prefer the later).
269
270
271 =item * NamingConventions::ProhibitMisspelledSymbolNames
272
273 The idea behind this policy is to encourage better names for variables
274 and subroutines by enforcing correct spelling and prohibiting the use of
275 home-grown abbreviations.  Assuming that the author uses underscores or
276 camel-case, it should be possible to split symbols into words, and then look
277 them up in a dictionary (see PodSpelling).  This policy should probably have
278 a similar stopwords feature as well.
279
280
281 =item * Documentation::RequireModuleAbstract
282
283 Require a C<=head1 NAME> POD section with content that matches
284 C<\A \s* [\w:]+ \s+ - \s+ \S>.  The single hyphen is the important bit.  Also,
285 must be a single line.
286
287
288 =item * Expressions::RequireFatCommasInHashConstructors
289
290 =item * ErrorHandling::RequireLocalizingGlobalErrorVariablesInDESTROY
291
292 Prevent C<$.>, C<$@>, C<$!>, C<$^E>, and C<$?> from being cleared unexpectedly
293 by DESTROY methods.
294
295     package Foo;
296
297     sub DESTROY {
298         die "Died in Foo::DESTROY()";
299     }
300
301     package main;
302
303     eval {
304         my $foo = Foo->new();
305
306         die "Died in eval."
307     }
308     print $@;   # "Died in Foo::DESTROY()", not "Died in eval.".
309
310 See L<http://use.perl.org/~Ovid/journal/36767> and
311 L<http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2008-06/msg00542.html>.
312
313
314 =item * Expressions::ProhibitDecimalWithBitwiseOperator
315
316 =item * Expressions::ProhibitStringsWithBitwiseOperator
317
318
319 =item * InputOutput::ProhibitMagicDiamond
320
321 Steal the idea from L<B::Lint|B::Lint>.
322
323
324 =item * NamingConventions::RequireArrayAndHashReferenceVariablesEndWith_Ref
325
326
327 =item * Programs::RequireShebang
328
329 Anything that is a program should have a shebang line.  This includes .t
330 files.
331
332
333 =item * Modules::RequirePackageDeclarationAsFirstStatementInModule
334
335 See L<http://blog.woobling.org/2009/11/scoping-of-current-package.html>.
336 Ouch.
337
338
339 =item * BuiltInFunctions::RequireConstantSprintfFormat
340
341
342 =item * BuiltInFunctions::RequireConstantUnpackFormat
343
344 L<http://diotalevi.isa-geek.net/~josh/yapc-lint/slides/slide5.html>
345
346
347 =item * Miscellanea::ProhibitObnoxiousComments
348
349 Forbid excessive hash marks e.g. "#### This is a loud comment ####".
350 Make the obnoxious pattern configurable
351
352
353 =item * ValuesAndExpressions::RequireNotOperator
354
355 Require the use of "not" instead of "!", except when this would contradict
356 ProhibitMixedBooleanOperators.  This may be better suited for
357 Perl::Critic::More.
358
359
360 =item * ValuesAndExpressions::ProhibitUnusedReadonlyConstants
361
362 We'll only be able to look at lexicals.  For similar reasons, we can't do
363 anything about L<constant>.
364
365
366 =item * Modules::RequireExplicitImporting
367
368 Require every C<use> statement to have an explicit import list.  You could
369 still get around this by calling C<import> directly.
370
371
372 =item * Modules::ForbidImporting
373
374 Require every C<use> to have an explicitly empty import list.  This is for
375 folks who like to see fully-qualified function names.  Should probably provide
376 a list of exempt modules (like FindBin);
377
378
379 =item * ControlStructures::ProhibitIncludeViaDo
380
381 Forbid C<do "foo.pl">.  Not sure about this policy name.
382
383
384 =item * Variables::ProhibitUseVars
385
386 Disallow C<use vars qw(...)> and require C<our $foo> instead.  This
387 contradicts Miscellanea::Prohibit5006isms.  Maybe verify C<use 5.6> before
388 applying this policy.  Low severity.
389
390
391 =item * VariablesAndExpressions::ProhibitQuotedHashKeys
392
393 Forbid quotes around hash keys, unless they are really needed.  This is
394 against what Damian says.  Suggested by Adam Kennedy.  Low severity.
395
396
397 =item * CodeLayout::ProhibitFunctionalNew
398
399 Good: C<< Foo::Bar->new >>, Bad: C<< new Foo::Bar >>
400
401
402 =item * RegularExpressions::ProhibitSWSWSW
403
404 Require C<split> instead of C<m/\s*\w*\s*\w*\s*/>.  From MJD's Red Flags.
405
406
407 =item * Documentation::RequireSynopsis
408
409
410 =item * Documentation::RequireLicense
411
412 These are simplified versions of Documentation::RequirePodSections.
413
414
415 =item * Documentation::RequireValidSynopsis
416
417 The Synopsis section must be all indented and must be syntactically valid Perl
418 (as validated by PPI).
419
420
421 =item * Documentation::ProhibitEmptySections
422
423 Any C<=headN> and C<=over> sections must not be empty.  This helps catch
424 boilerplate (although Test::Pod should catch empty C<=over> blocks).
425
426 On the other hand, C<=item ...> sections can be empty, since the item label is
427 content.
428
429
430 =item * Miscellaneous::ProhibitBoilerplate
431
432 Complain about copy-and-paste code or docs from h2xs, Module::Starter::*, etc.
433
434 Here's a non-PPI implementation:
435 L<http://search.cpan.org/src/JJORE/Carp-Clan-5.8/t/04boilerplate.t>
436
437
438 =item * NamingConventions::ProhibitPackagesSubroutinesAndBarewordFileHandlesWithTheSameNames
439
440 See
441 L<http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2009-01/msg00685.html>.
442
443
444 =item * BuiltinFunctions::ProhibitExtraneousScalarCall
445
446 Recommend that C<if (scalar @array)> be rewritten as C<if (@array)>.
447
448
449 =item * RegularExpressions::ProhibitMixedDelimiters
450
451 Ban s{foo}(bar)
452
453
454 =item * RegularExpressions::ProhibitScalarAsRegexp
455
456 Ban naked strings as regexps, like:
457
458     print 1 if $str =~ $regexp;
459
460 Instead, it should be:
461
462     print 1 if $str =~ m/$regexp/;
463
464 or
465
466     print 1 if $str =~ m/$regexp/xms;
467
468
469 =item * ValuesAndExpressions::RequireInterpolatedStringyEval
470
471 Ensure that the argument to a stringy eval is not a constant string.  That's
472 just wasteful.  Real world examples include:
473
474   eval 'use Optional::Module';
475
476 which is better written as
477
478   eval { require Optional::Module; Optional::Module->import };
479
480 for performance gains and compile-time syntax checking.
481
482 Question: This is very similar to BuiltInFunctions::ProhibitStringyEval. What
483 does the new policy buy us? Could we get the same thing with an option on the
484 latter to forbid un-interpolated includes even if C<allow_includes> is turned
485 on?
486
487
488 =item * RegularExpressions::ProhibitUnnecessaryEscapes
489
490 Complain if user puts a backslash escape in front of non-special characters.
491 For example:
492
493    m/\!/;
494
495 Make exceptions for C<\">, C<\'> and C<\`> since those are often inserted to
496 workaround bugs in syntax highlighting.
497
498 Note that this is different inside character classes, where only C<^>, C<]>
499 and C<-> need to be escaped, I think.  Caret only needs to be escaped at the
500 beginning, and dash does NOT need to be escaped at the beginning and end.  See
501 L<perlreref|perlreref>.
502
503
504 =item * Steal ideas from L<Dunce::Files|Dunce::Files>.
505
506 Can someone expand this entry, please?
507
508 =item * ControlStructures::ProhibitAssigmentInConditional
509
510 =item * ValuesAndExpressions::RequireConstantBeforeEquals
511
512 =item * ValuesAndExpressions::RequireConstantBeforeOperator
513
514 L<http://use.perl.org/~stu42j/journal/36412>
515
516 Just about everyone has been bitten by C<if ($x = 10) { ... }> when they meant
517 to use C<==>.  A safer style is C<10 == $x> because omitting the second C<=>
518 yields a noisy compile-time failure instead of silent runtime error.
519
520 ProhibitAssigmentInConditional complains if the condition of a while, until,
521 if or unless is solely an assignment.  If it's anything more complex (like
522 C<if (($x=10)){}> or C<while ($x=$y=$z){}>), there is no warning.
523
524 RequireConstantBeforeEquals complains if the left side of an C<==> is a
525 variable while the right side is a constant.
526
527 RequireConstantBeforeOperator complains if the left side of any comparison
528 operator (C<==>, C<eq>, C<&lt;>, etc) is a variable while the right side is a
529 constant.
530
531
532 =item * InputOutput::ProhibitUTF8IOLayer
533
534 http://www.perlfoundation.org/perl5/index.cgi?the_utf8_perlio_layer
535
536
537 =item * BuiltinFunctions::ProhibitExit(?:InModules)?
538
539 Forbid C<exit()> in files that lack a shebang.  Inspired by
540 L<http://use.perl.org/~Ovid/journal/36746> and an analogous checker in
541 FindBugs.
542
543
544 =item * Modules::ProhibitRedundantLoading
545
546 Don't allow a package to "use" the same module more than once, unless
547 there is a "no <module>" between them.
548
549 See https://rt.cpan.org/Ticket/Display.html?id=38074.
550
551
552 =item * ErrorHandling::RequireLocalizingEVAL_ERRORInDESTROY
553
554 The opposite side of ErrorHandling::RequireCheckingReturnValueOfEval.
555
556
557 =back
558
559
560 =head1 REFACTORINGS and ENHANCEMENTS
561
562 =over
563
564 =item * Reformat all the POD to use 78 columns instead of 70.
565
566 This thing of having different widths for the documentation and the code is
567 rediculous.  Don't do this until after the next non-dev release.  Elliot is
568 considering doing a special release only including this change so that the
569 search.cpan.org diff tool doesn't entirely break.
570
571
572 =item * Eliminate use of IO::String
573
574 I'm pretty sure that opening references to scalars is in 5.6, so IO::String
575 isn't necessary.
576
577
578 =item * Give L<Perl::Critic::Command> a proper API.
579
580 Now that we've got the guts of L<perlcritic> in there, we should make the
581 it available to users.
582
583
584 =item * Create constants for the PPI location array elements.
585
586 =item * Some means of detecting "runaway" C<##no critic>
587
588 Elliot was talking to a couple of users at ETech and one of their major
589 concerns was that they were using C<##no critic> and forgetting to do a
590 C<##use critic> after the problematic section.  Perhaps an option to
591 F<perlcritic> to scan for such things is in order.
592
593
594 =item * Change API to use named parameters
595
596 Most of the methods on the public classes use named parameters for passing
597 arguments.  I'd like to extend that pattern to include all object-methods.
598 Static methods can still use positional parameters.
599
600
601 =item * Enhance P::C::critique() to accept files, directories, or code strings
602
603 Just like F<bin/perlcritic> does now.
604
605
606 =item * Add C<-cache> flag to F<bin/perlcritic>
607
608 If enabled, this turns on L<PPI::Cache|PPI::Cache>:
609
610     require PPI::Cache;
611     my $cache_path = "/tmp/test-perl-critic-cache-$ENV{USER}";
612     mkdir $cache_path, oct 700 if (! -d $cache_path);
613     PPI::Cache->import(path => $cache_path);
614
615 This cache directory should perhaps include the PPI version number!  At least
616 until PPI incorporates its own version number in the cache.
617
618 (see F<t/40_criticize.t> for a more robust implementation)
619
620
621 =item * Use hash-lookup instead of C<List::MoreUtils::any> function.
622
623 In several places, Perl::Critic uses C<List::MoreUtils::any> to see if a
624 string is a member of a list.  Instead, I suggest using a named subroutine
625 that does a hash-lookup like this:
626
627     my %logical_ops = hashify( qw( ! || && ||= &&= and or not ) );
628     sub is_logical_op { return exists $logical_ops{ $_[0] }; }
629
630 Question: Why?
631
632 Answer: Readability, mostly.  Performance, maybe.
633
634
635 =back
636
637 =head1 PPI BUGS
638
639 We're waiting on the following bugs to get fixed in a CPAN release of PPI:
640
641
642 =over
643
644 =item PPI::Token::descendant_of()
645
646 Exists in svn.  Replace _descendant_of() in RequireCheckingReturnValueOfEval
647 with that, once it is released, because it's faster and native.
648
649
650 =item Newlines
651
652 PPI does not preserve newlines.  That makes
653 CodeLayout::RequireConsistentNewlines impossible to implement under PPI.  For
654 now, it's implemented by pulling the source out of the file and skipping PPI.
655
656 It's unlikely that PPI will support mixed newlines anytime soon.
657
658
659 =item Operators
660
661 ValuesAndExpressions::ProhibitMismatchedOperators has two workarounds for PPI
662 bugs with parsing operators.  Many of these bugs have been fixed in PPI, so it
663 would be good to check if those workarounds are still needed.
664
665
666 =item Regexp methods
667
668 Not strictly a bug -- the PPI Regexp classes have a dearth of accessor methods
669 as of v1.118, meaning that we have to do messy digging into internals.  I
670 wrote Perl::Critic:Utils::PPIRegexp to encapsulate this messiness, but it
671 would be nicer to have an official interface in PPI.
672
673
674 =item QuoteLike::Words in the place of a ForLoop
675
676 PPI incorrectly parses C<<for qw<blah> {}>>.
677
678
679 =back
680
681 =cut
682
683 ##############################################################################
684 # Local Variables:
685 #   mode: cperl
686 #   cperl-indent-level: 4
687 #   fill-column: 78
688 #   indent-tabs-mode: nil
689 #   c-indentation-style: bsd
690 # End:
691 # ex: set ts=8 sts=4 sw=4 tw=78 ft=pod expandtab shiftround :