Login
1f9432b970b32910975b4a7da39837d74d52ec19
[gknop/Perl-Critic.git] / TODO.pod
1 # best viewed via "perldoc TODO.pod"
2
3 =pod
4
5 =for stopwords LHS RHS REFACTORINGS FH SVN stopwords
6
7 =head1 NAME
8
9 Perl::Critic::TODO - Things for Perl::Critic developers to do
10
11
12 =head1 SOURCE
13
14     #######################################################################
15     #      $URL$
16     #     $Date$
17     #   $Author$
18     # $Revision$
19     #######################################################################
20
21
22 =head1 SEE ALSO
23
24 Perl-Critic-More is a separate distribution for less-widely-accepted
25 policies.  It contains its own TODO.pod.
26
27
28 =head1 NEW FEATURES
29
30 =over
31
32 =item * Report Safari sections in addition to book page numbers.
33
34
35 =item * Add --files-with-violations/-l and --files-without-violations/-L options to F<perlcritic>.
36
37 Just print out file names.  I could have used this at work when combined with
38 C<--single-policy>.
39
40     gvim `perlcritic --single-policy QuotedWordLists -l`
41
42
43 =item * Add a file Behavior.
44
45
46 =item * Allow values of (at least) string-list Parameters to be specified in a file.
47
48 For the benefit of PodSpelling, etc.
49
50
51 =item * Enhance string-list Behavior to allow specification of delimiters.
52
53 For things like RequirePodSections.
54
55
56 =item * Add queries to --list option to F<perlcritic>.
57
58 List Policies based upon severity, theme, and (what I want this second)
59 applies_to.
60
61 =item * Add formatting of --list output.
62
63 Support Jeff Bisbee's use case (he dumps all the policies in severity order
64 with full descriptions and other metadata).
65
66 =item * Add --prohibit-unrestricted-no-critic option to F<perlcritic>.
67
68 Requires C<## no critic> to take an argument:
69
70   ## no critic (SomePolicyPattern)     # ok
71   ## no critic                         # not ok
72
73 Can't be done as a regular Policy because any line that violated it would
74 disable it.
75
76
77 =item * Support for C<#line 123 "filename"> directives.
78
79 For code generators and template languages that allow inline Perl code.
80
81 Yes, somebody has an in-house templating system where they've written a custom
82 test module that extracts the perl code from a template and critiques it.
83
84 Actually, this would be useful for programs: Module::Build "fixes" shebang
85 lines so that there's the bit about invoking perl if the program is attempted
86 to be run by a Bourne shell, which throws the line numbers off when using
87 Test::P::C on the contents of a C<blib> directory.
88
89
90 =item * Enhance statistics.
91
92 - Blank line count
93
94 - POD line count
95
96 - Comment line count
97
98 - Data section count
99
100
101 =item * Detect 5.10 source and enable stuff for that.
102
103 For example, treat C<say> as equivalent to C<print>.
104
105
106 =item * Support a means of failing if a Policy isn't installed.
107
108 For example, the self compliance test now depends upon a Policy in the More
109 distribution.
110
111 Something like using a "+" sign in front of the Policy name in its
112 configuration block, analogous to the "-" sign used for disabling a policy,
113 e.g. "C<[+Example::Policy]>".
114
115
116 =back
117
118
119 =head1 BUGS/LIMITATIONS
120
121 =over
122
123 =item * ControlStructures::ProhibitPostfixControls
124
125 Look for the do {} while case and change the explanation to point to page 123
126 when it is found.  RT #37905.
127
128
129 =item * NamingConventions::ProhibitAmbiguousNames
130
131 Don't allow compound names with forbidden words, like "last_record".
132 Allow forbidden words in RHS of variable declarations
133
134 Also, we should make it easeir to add (or delete) words from the
135 forbbiden list.
136
137
138 =item * Subroutines::ProtectPrivateSubs
139
140 Doesn't forbid C<< $pkg->_foo() >> because it can't tell the
141 difference between that and C<< $self->_foo() >>
142
143
144 =item * ErrorHandling::RequireCarping
145
146 This should not complain about using C<warn> or C<die> if it's not in a
147 function, or if it's in main::.
148
149 Also, should allow C<die> when it is obvious that the "message" is a
150 reference.
151
152
153 =item * RegularExpressions::ProhibitCaptureWithoutTest
154
155 Allow this construct:
156
157     for ( ... ) {
158         next unless /(....)/;
159         if ( $1 ) {
160             ....
161         }
162     }
163
164 Right now, P::C thinks that the C<$1> isn't legal to use because it's
165 "outside" of the match.  The thing is, we can only get to the C<if>
166 if the regex matched.
167
168     while ( $str =~ /(expression)/ )
169
170
171 =item * CodeLayout::ProhibitParensWithBuiltins
172
173 Some builtin functions (particularly those that take a variable number of
174 scalar arguments) should probably get parentheses.  This policy should be
175 enhanced to allow the user to specify a list of builtins that are expempt
176 from the policy.
177
178
179 =item * ValuesAndExpressions::ProhibitCommaSeparatedStatements
180
181 Needs to check for C<scalar( something, something )>.
182
183
184 =item * Variables::ProhibitPunctuationVars
185
186 Needs to look inside strings.  RT #35970.
187
188
189 =item * TestingAndDebugging::RequireUseWarnings
190
191 Check for -w on the shbang line.
192
193
194 =item * Change formatting in Violation to eliminate double periods.
195
196
197 =back
198
199
200 =head1 OTHER PBP POLICIES THAT SEEM FEASIBLE TO IMPLEMENT
201
202 =over
203
204 =item * Modules::RequireUseVersion [405-406]
205
206 =item * Modules::RequireThreePartVersion [405-406]
207
208 =back
209
210
211 =head1 NON-PBP POLICIES WANTED
212
213 =over
214
215 =item * NamingConventions::ProhibitMisspelledSymbolNames
216
217 The idea behind this policy is to encourage better names for variables
218 and subroutines by enforcing correct spelling and prohibiting the use of
219 home-grown abbreviations.  Assumng that the author uses underscores or
220 camel-case, it should be possible to split symbols into words, and then look
221 them up in a dictionary (see PodSpelling).  This policy should probably have
222 a similar stopwords feature as well.
223
224 =item * Documentation::RequireModuleAbstract
225
226 Require a C<=head1 NAME> POD section with content that matches
227 C<\A \s* [\w:]+ \s+ - \s+ \S>.  The single hyphen is the important bit.  Also,
228 must be a single line.
229
230 =item * Expressions::RequireFatCommasInHashConstructors
231
232 =item * ErrorHandling::RequireLocalizingEvalErrorInDESTROY
233
234 Prevent C<$@> from being cleared unexpectedly by DESTROY methods.
235
236     package Foo;
237
238     sub DESTROY {
239         die "Died in Foo::DESTROY()";
240     }
241
242     package main;
243
244     eval {
245         my $foo = Foo->new();
246
247         die "Died in eval."
248     }
249     print $@;   # "Died in Foo::DESTROY()", not "Died in eval.".
250
251 See L<http://use.perl.org/~Ovid/journal/36767>.
252
253 =item * Expressions::ProhibitDecimalWithBitwiseOperator
254
255 =item * Expressions::ProhibitStringsWithBitwiseOperator
256
257
258 =item * InputOutput::ProhibitMagicDiamond
259
260 Steal the idea from L<B::Lint|B::Lint>.
261
262
263 =item * TBD::AllProgramsNeedShebangs
264
265 Anything that is a program should have a shebang line.  This includes .t
266 files.
267
268
269 =item * BuiltInFunctions::RequireConstantSprintfFormat
270
271
272 =item * BuiltInFunctions::RequireConstantUnpackFormat
273
274 L<http://diotalevi.isa-geek.net/~josh/yapc-lint/slides/slide5.html>
275
276
277 =item * Miscellanea::ProhibitObnoxiousComments
278
279 Forbid excessive hash marks e.g. "#### This is a loud comment ####".
280 Make the obnoxious pattern configurable
281
282
283 =item * ValuesAndExpressions::RequireNotOperator
284
285 Require the use of "not" instead of "!", except when this would contradict
286 ProhibitMixedBooleanOperators.  This may be better suited for
287 Perl::Critic::More.
288
289
290 =item * Modules::RequireExplicitImporting
291
292 Require every C<use> statement to have an explicit import list.  You could
293 still get around this by calling C<import> directly.
294
295
296 =item * Modules::ForbidImporting
297
298 Require every C<use> to have an explicitly empty import list.  This is for
299 folks who like to see fully-qualified function names.  Should probably provide
300 a list of exempt modules (like FindBin);
301
302
303 =item * ControlStructures::ProhibitIncludeViaDo
304
305 Forbid C<do "foo.pl">.  Not sure about this policy name.
306
307
308 =item * Variables::ProhibitUseVars
309
310 Disallow C<use vars qw(...)> and require C<our $foo> instead.  This
311 contradicts Miscellanea::Prohibit5006isms.  Maybe verify C<use 5.6> before
312 applying this policy.  Low severity.
313
314
315 =item * VariablesAndExpressions::ProhibitQuotedHashKeys
316
317 Forbid quotes around hash keys, unless they are really needed.  This is
318 against what Damian says.  Suggested by Adam Kennedy.  Low severity.
319
320
321 =item * CodeLayout::ProhibitFunctionalNew
322
323 Good: C<< Foo::Bar->new >>, Bad: C<< new Foo::Bar >>
324
325
326 =item * RegularExpressions::ProhibitSWSWSW
327
328 Require C<split> instead of C<m/\s*\w*\s*\w*\s*/>.  From MJD's Red Flags.
329
330
331 =item * VariablesAndExpressions::RequireConstantVersion (low severity)
332
333
334 =item * VariablesAndExpressions::ProhibitComplexVersion (medium severity)
335
336 L<http://rt.cpan.org/Ticket/Display.html?id=20439>
337
338
339 =item * Documentation::RequireSynopsis
340
341
342 =item * Documentation::RequireLicense
343
344 These are simplified versions of Documentation::RequirePodSections.
345
346
347 =item * Documentation::RequireValidSynopsis
348
349 The Synopsis section must be all indented and must be syntactically valid Perl
350 (as validated by PPI).
351
352
353 =item * Documentation::ProhibitEmptySections
354
355 Any C<=headN> and C<=over> sections must not be empty.  This helps catch
356 boilerplate (althought Test::Pod should catch empty C<=over> blocks).
357
358 On the other hand, C<=item ...> sections can be empty, since the item label is
359 content.
360
361
362 =item * Miscellaneous::ProhibitBoilerplate
363
364 Complain about copy-and-paste code or docs from h2xs, Module::Starter::*, etc.
365
366 Here's a non-PPI implementation:
367 L<http://search.cpan.org/src/JJORE/Carp-Clan-5.8/t/04boilerplate.t>
368
369
370 =item * BuiltinFunctions::ProhibitExtraneousScalarCall
371
372 Recommend that C<if (scalar @array)> be rewritten as C<if (@array)>.
373
374
375 =item * RegularExpressions::ProhibitMixedDelimiters
376
377 Ban s{foo}(bar)
378
379
380 =item * RegularExpressions::ProhibitScalarAsRegexp
381
382 Ban naked srtings as regexps, like:
383
384     print 1 if $str =~ $regexp;
385
386 Instead, it should be:
387
388     print 1 if $str =~ m/$regexp/;
389
390 or
391
392     print 1 if $str =~ m/$regexp/xms;
393
394
395 =item * ValuesAndExpressions::RequireInterpolatedStringyEval
396
397 Ensure that the argument to a stringy eval is not a constant string.  That's
398 just wasteful.  Real world examples include:
399
400   eval 'use Optional::Module';
401
402 which is better written as
403
404   eval { require Optional::Module; Optional::Module->import };
405
406 for performance gains and compile-time syntax checking.
407
408
409 =item * RegularExpressions::ProhibitUnnecessaryEscapes
410
411 Complain if user puts a backslash escape in front of non-special characters.
412 For example:
413
414    m/\!/;
415
416 Make exceptions for C<\">, C<\'> and C<\`> since those are often inserted to
417 workaround bugs in syntax highlighting.
418
419 Note that this is different inside character classes, where only C<^>, C<]>
420 and C<-> need to be escaped, I think.  Caret only needs to be escaped at the
421 beginning, and dash does NOT need to be escaped at the beginning and end.  See
422 L<perlreref|perlreref>.
423
424
425 =item * Steal ideas from L<Dunce::Files|Dunce::Files>.
426
427 Can someone expand this entry, please?
428
429 =item * ControlStructures::ProhibitAssigmentInConditional
430
431 =item * ValuesAndExpressions::RequireConstantBeforeEquals
432
433 =item * ValuesAndExpressions::RequireConstantBeforeOperator
434
435 L<http://use.perl.org/~stu42j/journal/36412>
436
437 Just about everyone has been bitten by C<if ($x = 10) { ... }> when they meant
438 to use C<==>.  A safer style is C<10 == $x> because omitting the second C<=>
439 yields a noisy compile-time failure instead of silent runtime error.
440
441 ProhibitAssigmentInConditional complains if the condition of a while, until,
442 if or unless is solely an assignment.  If it's anything more complex (like
443 C<if (($x=10)){}> or C<while ($x=$y=$z){}>), there is no warning.
444
445 RequireConstantBeforeEquals complains if the left side of an C<==> is a
446 variable while the right side is a constant.
447
448 RequireConstantBeforeOperator complains if the left side of any comparison
449 operator (C<==>, C<eq>, C<&lt;>, etc) is a variable while the right side is a
450 constant.
451
452
453 =item * InputOutput::ProhibitUTF8IOLayer
454
455 http://www.perlfoundation.org/perl5/index.cgi?the_utf8_perlio_layer
456
457 =item * BuiltinFunctions::ProhibitExit(?:InModules)?
458
459 Forbid C<exit()> in files that lack a shebang.  Inspired by
460 L<http://use.perl.org/~Ovid/journal/36746> and an analgous checker in
461 FindBugs.
462
463 =item * Modules::ProhibitRedundantLoading
464
465 Don't allow a package to "use" the same module more than once, unless
466 there is a "no <module>" between them.
467
468 See https://rt.cpan.org/Ticket/Display.html?id=38074.
469
470 =back
471
472
473 =head1 REFACTORINGS and ENHANCEMENTS
474
475 =over
476
477 =item * Create constants for the PPI location array elements.
478
479
480 =item * MOVE THE LINE-DISABLING INTO P::C::Document
481
482 All the code that deals with finding all the '##no critic' comments and noting
483 which policies are disabled at each line seems like it would be better placed
484 in Perl::Critic::Document.  P::C::Document could then provide methods to
485 indicate if a policy is disabled at a particular line.  So the basic algorithm
486 in Perl::Critic might look something like this:
487
488   foreach $element (@PPI_ELEMENTS) {
489      foreach $policy (@POLICIES) {
490         $line = $element->location->[0];
491         next if $doc->policy_is_disabled_at_line( $policy, $line );
492         push @violations, $policy->violates( $elem, $doc );
493      }
494   }
495
496
497 =item * Some means of detecting "runnaway" C<##no critic>
498
499 Elliot was talking to a couple of users at ETech and one of their major
500 concerns was that they were using C<##no critic> and forgetting to do a
501 C<##use critic> after the problematic section.  Perhaps an option to
502 F<perlcritic> to scan for such things is in order.
503
504
505 =item * Change API to use named parameters
506
507 Most of the methods on the public classes use named parameters for passing
508 arguments.  I'd like to extend that pattern to include all object-methods.
509 Static methods can still use positional parameters.
510
511
512 =item * Enhance P::C::critique() to accept files, directories, or code strings
513
514 Just like F<bin/perlcritic> does now.
515
516
517 =item * Add C<-cache> flag to F<bin/perlcritic>
518
519 If enabled, this turns on L<PPI::Cache|PPI::Cache>:
520
521     require PPI::Cache;
522     my $cache_path = "/tmp/test-perl-critic-cache-$ENV{USER}";
523     mkdir $cache_path, oct 700 if (! -d $cache_path);
524     PPI::Cache->import(path => $cache_path);
525
526 This cachedir should perhaps include the PPI version number!  At least until
527 PPI incorporates its own version number in the cache.
528
529 (see F<t/40_criticize.t> for a more robust implementation)
530
531
532 =item * Use hash-lookup instead of C<List::MoreUtils::any> function.
533
534 In several places, Perl::Critic uses C<List::MoreUtils::any> to see if a
535 string is a member of a list.  Instead, I suggest using a named subroutine
536 that does a hash-lookup like this:
537
538     my %logical_ops = hashify( qw( ! || && ||= &&= and or not ) );
539     sub is_logical_op { return exists $logical_ops{ $_[0] }; }
540
541 Question: Why?
542
543 Answer: Readability, mostly.  Performance, maybe.
544
545 =item Refactor guts of perlcritic into a module (Perl::Critic::App ??)
546
547 Because it is getting unwieldy in there.
548
549 =back
550
551 =head1 PPI BUGS
552
553 We're waiting on the following bugs to get fixed in a CPAN release of PPI:
554
555
556 =over
557
558 =item PPI::Token::descendant_of()
559
560 Exists in svn.  Replace _descendant_of() in RequireCheckingReturnValueOfEval
561 with that, once it is released, because it's faster and native.
562
563 =item Newlines
564
565 PPI does not preserve newlines.  That makes
566 CodeLayout::RequireConsistentNewlines impossible to implement under PPI.  For
567 now, it's implemented by pulling the source out of the file and skipping PPI.
568
569 It's unlikely that PPI will support mixde newlines anytime soon.
570
571
572 =item Operators
573
574 ValuesAndExpressions::ProhibitMismatchedOperators has two workarounds for PPI
575 bugs with parsing operators.  Many of these bugs have been fixed in PPI, so it
576 would be good to check if those workarounds are still needed.
577
578
579 =item Regexp methods
580
581 Not strictly a bug -- the PPI Regexp classes have a dearth of accessor methods
582 as of v1.118, meaning that we have to do messy digging into internals.  I
583 wrote Perl::Critic:Utils::PPIRegexp to encapsulate this messiness, but it
584 would be nicer to have an official interface in PPI.
585
586
587 =item QuoteLike::Words in the place of a ForLoop
588
589 PPI mis-parses C<<for qw<blah> {}>>.
590
591
592 =back
593
594 =cut
595
596 ##############################################################################
597 # Local Variables:
598 #   mode: cperl
599 #   cperl-indent-level: 4
600 #   fill-column: 78
601 #   indent-tabs-mode: nil
602 #   c-indentation-style: bsd
603 # End:
604 # ex: set ts=8 sts=4 sw=4 tw=78 ft=pod expandtab shiftround :