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