Created a new Policy to prohibit an unrestricted ## no critic.

[gknop/Perl-Critic.git] / TODO.pod

# best viewed via "perldoc TODO.pod"

=pod

=for stopwords LHS RHS REFACTORINGS FH SVN stopwords

=head1 NAME

Perl::Critic::TODO - Things for Perl::Critic developers to do

=head1 SOURCE

    #######################################################################
    #      $URL$
    #     $Date$
    #   $Author$
    # $Revision$
    #######################################################################


=head1 SEE ALSO

Perl-Critic-More is a separate distribution for less-widely-accepted
policies.  It contains its own TODO.pod.


=head1 NEW FEATURES

=over

=item * Report Safari sections in addition to book page numbers.


=item * Add --files-with-violations/-l and --files-without-violations/-L options to F<perlcritic>.

Just print out file names.  I could have used this at work when combined with
C<--single-policy>.

    gvim `perlcritic --single-policy QuotedWordLists -l`


=item * Add a file Behavior.


=item * Allow values of (at least) string-list Parameters to be specified in a file.

For the benefit of PodSpelling, etc.


=item * Enhance string-list Behavior to allow specification of delimiters.

For things like RequirePodSections.


=item * Add queries to --list option to F<perlcritic>.

List Policies based upon severity, theme, and (what I want this second)
applies_to.

=item * Add formatting of --list output.

Support Jeff Bisbee's use case (he dumps all the policies in severity order
with full descriptions and other metadata).

=item * Support for C<#line 123 "filename"> directives.

For code generators and template languages that allow inline Perl code.

Yes, somebody has an in-house templating system where they've written a custom
test module that extracts the perl code from a template and critiques it.

Actually, this would be useful for programs: Module::Build "fixes" shebang
lines so that there's the bit about invoking perl if the program is attempted
to be run by a Bourne shell, which throws the line numbers off when using
Test::P::C on the contents of a C<blib> directory.

This actually requires support from PPI.


=item * Enhance statistics.

- Blank line count

- POD line count

- Comment line count

- Data section count


=item * Detect 5.10 source and enable stuff for that.

For example, treat C<say> as equivalent to C<print>.


=item * Support a means of failing if a Policy isn't installed.

For example, the self compliance test now depends upon a Policy in the More
distribution.

Something like using a "+" sign in front of the Policy name in its
configuration block, analogous to the "-" sign used for disabling a policy,
e.g. "C<[+Example::Policy]>".


=item * Add support in .run files for regexes for violation descriptions.


=item * Add an "inverse mode": complain about "## no critic" that doesn't eliminate any violations.

L<Alias on P::C|http://use.perl.org/article.pl?sid=08/09/24/1957256>.

I<For example, Perl::Critic helps both helps educate developers and reduce the
time cost of bugs and weird uses of Perl, but at the expense of having to
maintain nocritic entries permanently, which themselves have education and
time costs.>

I<And the goal of the Perl Critic team is that the education and time cost of
using Perl::Critic is significantly less than the education and time costs of
NOT using Perl::Critic.>

In order to allow people to get rid of "## no critic" markers, create a mode
that tells them which markers don't actually hide any violations.


=back


=head1 BUGS/LIMITATIONS

=over

=item * ControlStructures::ProhibitPostfixControls

Look for the do {} while case and change the explanation to point to page 123
when it is found.  RT #37905.


=item * NamingConventions::ProhibitAmbiguousNames

Don't allow compound names with forbidden words, like "last_record".
Allow forbidden words in RHS of variable declarations

Also, we should make it easeir to add (or delete) words from the
forbbiden list.


=item * Subroutines::ProtectPrivateSubs

Doesn't forbid C<< $pkg->_foo() >> because it can't tell the
difference between that and C<< $self->_foo() >>


=item * ErrorHandling::RequireCarping

This should not complain about using C<warn> or C<die> if it's not in a
function, or if it's in main::.

Also, should allow C<die> when it is obvious that the "message" is a
reference.


=item * RegularExpressions::ProhibitCaptureWithoutTest

Allow this construct:

    for ( ... ) {
        next unless /(....)/;
        if ( $1 ) {
            ....
        }
    }

Right now, P::C thinks that the C<$1> isn't legal to use because it's
"outside" of the match.  The thing is, we can only get to the C<if>
if the regex matched.

    while ( $str =~ /(expression)/ )


=item * CodeLayout::ProhibitParensWithBuiltins

Some builtin functions (particularly those that take a variable number of
scalar arguments) should probably get parentheses.  This policy should be
enhanced to allow the user to specify a list of builtins that are expempt
from the policy.


=item * ValuesAndExpressions::ProhibitCommaSeparatedStatements

Needs to check for C<scalar( something, something )>.


=item * Variables::ProhibitPunctuationVars

Needs to look inside strings.  RT #35970.


=item * TestingAndDebugging::RequireUseWarnings

Check for -w on the shbang line.


=item * Change formatting in Violation to eliminate double periods.


=back


=head1 OTHER PBP POLICIES THAT SEEM FEASIBLE TO IMPLEMENT

=over

=item * Modules::RequireUseVersion [405-406]

=item * Modules::RequireThreePartVersion [405-406]

=back


=head1 NON-PBP POLICIES WANTED

=over

=item * NamingConventions::ProhibitMisspelledSymbolNames

The idea behind this policy is to encourage better names for variables
and subroutines by enforcing correct spelling and prohibiting the use of
home-grown abbreviations.  Assumng that the author uses underscores or
camel-case, it should be possible to split symbols into words, and then look
them up in a dictionary (see PodSpelling).  This policy should probably have
a similar stopwords feature as well.

=item * Documentation::RequireModuleAbstract

Require a C<=head1 NAME> POD section with content that matches
C<\A \s* [\w:]+ \s+ - \s+ \S>.  The single hyphen is the important bit.  Also,
must be a single line.

=item * Expressions::RequireFatCommasInHashConstructors

=item * ErrorHandling::RequireLocalizingEvalErrorInDESTROY

Prevent C<$@> from being cleared unexpectedly by DESTROY methods.

    package Foo;

    sub DESTROY {
        die "Died in Foo::DESTROY()";
    }

    package main;

    eval {
        my $foo = Foo->new();

        die "Died in eval."
    }
    print $@;   # "Died in Foo::DESTROY()", not "Died in eval.".

See L<http://use.perl.org/~Ovid/journal/36767>.

=item * Expressions::ProhibitDecimalWithBitwiseOperator

=item * Expressions::ProhibitStringsWithBitwiseOperator


=item * InputOutput::ProhibitMagicDiamond

Steal the idea from L<B::Lint|B::Lint>.


=item * TBD::AllProgramsNeedShebangs

Anything that is a program should have a shebang line.  This includes .t
files.


=item * BuiltInFunctions::RequireConstantSprintfFormat


=item * BuiltInFunctions::RequireConstantUnpackFormat

L<http://diotalevi.isa-geek.net/~josh/yapc-lint/slides/slide5.html>


=item * Miscellanea::ProhibitObnoxiousComments

Forbid excessive hash marks e.g. "#### This is a loud comment ####".
Make the obnoxious pattern configurable


=item * ValuesAndExpressions::RequireNotOperator

Require the use of "not" instead of "!", except when this would contradict
ProhibitMixedBooleanOperators.  This may be better suited for
Perl::Critic::More.


=item * Modules::RequireExplicitImporting

Require every C<use> statement to have an explicit import list.  You could
still get around this by calling C<import> directly.


=item * Modules::ForbidImporting

Require every C<use> to have an explicitly empty import list.  This is for
folks who like to see fully-qualified function names.  Should probably provide
a list of exempt modules (like FindBin);


=item * ControlStructures::ProhibitIncludeViaDo

Forbid C<do "foo.pl">.  Not sure about this policy name.


=item * Variables::ProhibitUseVars

Disallow C<use vars qw(...)> and require C<our $foo> instead.  This
contradicts Miscellanea::Prohibit5006isms.  Maybe verify C<use 5.6> before
applying this policy.  Low severity.


=item * VariablesAndExpressions::ProhibitQuotedHashKeys

Forbid quotes around hash keys, unless they are really needed.  This is
against what Damian says.  Suggested by Adam Kennedy.  Low severity.


=item * CodeLayout::ProhibitFunctionalNew

Good: C<< Foo::Bar->new >>, Bad: C<< new Foo::Bar >>


=item * RegularExpressions::ProhibitSWSWSW

Require C<split> instead of C<m/\s*\w*\s*\w*\s*/>.  From MJD's Red Flags.


=item * VariablesAndExpressions::RequireConstantVersion (low severity)


=item * VariablesAndExpressions::ProhibitComplexVersion (medium severity)

L<http://rt.cpan.org/Ticket/Display.html?id=20439>


=item * Documentation::RequireSynopsis


=item * Documentation::RequireLicense

These are simplified versions of Documentation::RequirePodSections.


=item * Documentation::RequireValidSynopsis

The Synopsis section must be all indented and must be syntactically valid Perl
(as validated by PPI).


=item * Documentation::ProhibitEmptySections

Any C<=headN> and C<=over> sections must not be empty.  This helps catch
boilerplate (althought Test::Pod should catch empty C<=over> blocks).

On the other hand, C<=item ...> sections can be empty, since the item label is
content.


=item * Miscellaneous::ProhibitBoilerplate

Complain about copy-and-paste code or docs from h2xs, Module::Starter::*, etc.

Here's a non-PPI implementation:
L<http://search.cpan.org/src/JJORE/Carp-Clan-5.8/t/04boilerplate.t>


=item * BuiltinFunctions::ProhibitExtraneousScalarCall

Recommend that C<if (scalar @array)> be rewritten as C<if (@array)>.


=item * RegularExpressions::ProhibitMixedDelimiters

Ban s{foo}(bar)


=item * RegularExpressions::ProhibitScalarAsRegexp

Ban naked srtings as regexps, like:

    print 1 if $str =~ $regexp;

Instead, it should be:

    print 1 if $str =~ m/$regexp/;

or

    print 1 if $str =~ m/$regexp/xms;


=item * ValuesAndExpressions::RequireInterpolatedStringyEval

Ensure that the argument to a stringy eval is not a constant string.  That's
just wasteful.  Real world examples include:

  eval 'use Optional::Module';

which is better written as

  eval { require Optional::Module; Optional::Module->import };

for performance gains and compile-time syntax checking.


=item * RegularExpressions::ProhibitUnnecessaryEscapes

Complain if user puts a backslash escape in front of non-special characters.
For example:

   m/\!/;

Make exceptions for C<\">, C<\'> and C<\`> since those are often inserted to
workaround bugs in syntax highlighting.

Note that this is different inside character classes, where only C<^>, C<]>
and C<-> need to be escaped, I think.  Caret only needs to be escaped at the
beginning, and dash does NOT need to be escaped at the beginning and end.  See
L<perlreref|perlreref>.


=item * Steal ideas from L<Dunce::Files|Dunce::Files>.

Can someone expand this entry, please?

=item * ControlStructures::ProhibitAssigmentInConditional

=item * ValuesAndExpressions::RequireConstantBeforeEquals

=item * ValuesAndExpressions::RequireConstantBeforeOperator

L<http://use.perl.org/~stu42j/journal/36412>

Just about everyone has been bitten by C<if ($x = 10) { ... }> when they meant
to use C<==>.  A safer style is C<10 == $x> because omitting the second C<=>
yields a noisy compile-time failure instead of silent runtime error.

ProhibitAssigmentInConditional complains if the condition of a while, until,
if or unless is solely an assignment.  If it's anything more complex (like
C<if (($x=10)){}> or C<while ($x=$y=$z){}>), there is no warning.

RequireConstantBeforeEquals complains if the left side of an C<==> is a
variable while the right side is a constant.

RequireConstantBeforeOperator complains if the left side of any comparison
operator (C<==>, C<eq>, C<&lt;>, etc) is a variable while the right side is a
constant.


=item * InputOutput::ProhibitUTF8IOLayer

http://www.perlfoundation.org/perl5/index.cgi?the_utf8_perlio_layer

=item * BuiltinFunctions::ProhibitExit(?:InModules)?

Forbid C<exit()> in files that lack a shebang.  Inspired by
L<http://use.perl.org/~Ovid/journal/36746> and an analgous checker in
FindBugs.

=item * Modules::ProhibitRedundantLoading

Don't allow a package to "use" the same module more than once, unless
there is a "no <module>" between them.

See https://rt.cpan.org/Ticket/Display.html?id=38074.

=back


=head1 REFACTORINGS and ENHANCEMENTS

=over

=item * Create constants for the PPI location array elements.


=item * MOVE THE LINE-DISABLING INTO P::C::Document

All the code that deals with finding all the '##no critic' comments and noting
which policies are disabled at each line seems like it would be better placed
in Perl::Critic::Document.  P::C::Document could then provide methods to
indicate if a policy is disabled at a particular line.  So the basic algorithm
in Perl::Critic might look something like this:

  foreach $element (@PPI_ELEMENTS) {
     foreach $policy (@POLICIES) {
        $line = $element->location->[0];
        next if $doc->policy_is_disabled_at_line( $policy, $line );
        push @violations, $policy->violates( $elem, $doc );
     }
  }


=item * Some means of detecting "runnaway" C<##no critic>

Elliot was talking to a couple of users at ETech and one of their major
concerns was that they were using C<##no critic> and forgetting to do a
C<##use critic> after the problematic section.  Perhaps an option to
F<perlcritic> to scan for such things is in order.


=item * Change API to use named parameters

Most of the methods on the public classes use named parameters for passing
arguments.  I'd like to extend that pattern to include all object-methods.
Static methods can still use positional parameters.


=item * Enhance P::C::critique() to accept files, directories, or code strings

Just like F<bin/perlcritic> does now.


=item * Add C<-cache> flag to F<bin/perlcritic>

If enabled, this turns on L<PPI::Cache|PPI::Cache>:

    require PPI::Cache;
    my $cache_path = "/tmp/test-perl-critic-cache-$ENV{USER}";
    mkdir $cache_path, oct 700 if (! -d $cache_path);
    PPI::Cache->import(path => $cache_path);

This cachedir should perhaps include the PPI version number!  At least until
PPI incorporates its own version number in the cache.

(see F<t/40_criticize.t> for a more robust implementation)


=item * Use hash-lookup instead of C<List::MoreUtils::any> function.

In several places, Perl::Critic uses C<List::MoreUtils::any> to see if a
string is a member of a list.  Instead, I suggest using a named subroutine
that does a hash-lookup like this:

    my %logical_ops = hashify( qw( ! || && ||= &&= and or not ) );
    sub is_logical_op { return exists $logical_ops{ $_[0] }; }

Question: Why?

Answer: Readability, mostly.  Performance, maybe.

=item * Refactor guts of perlcritic into a module (Perl::Critic::App ??)

Because it is getting unwieldy in there.

=back

=head1 PPI BUGS

We're waiting on the following bugs to get fixed in a CPAN release of PPI:


=over

=item PPI::Token::descendant_of()

Exists in svn.  Replace _descendant_of() in RequireCheckingReturnValueOfEval
with that, once it is released, because it's faster and native.

=item Newlines

PPI does not preserve newlines.  That makes
CodeLayout::RequireConsistentNewlines impossible to implement under PPI.  For
now, it's implemented by pulling the source out of the file and skipping PPI.

It's unlikely that PPI will support mixde newlines anytime soon.


=item Operators

ValuesAndExpressions::ProhibitMismatchedOperators has two workarounds for PPI
bugs with parsing operators.  Many of these bugs have been fixed in PPI, so it
would be good to check if those workarounds are still needed.


=item Regexp methods

Not strictly a bug -- the PPI Regexp classes have a dearth of accessor methods
as of v1.118, meaning that we have to do messy digging into internals.  I
wrote Perl::Critic:Utils::PPIRegexp to encapsulate this messiness, but it
would be nicer to have an official interface in PPI.


=item QuoteLike::Words in the place of a ForLoop

PPI mis-parses C<<for qw<blah> {}>>.


=back

=cut

##############################################################################
# Local Variables:
#   mode: cperl
#   cperl-indent-level: 4
#   fill-column: 78
#   indent-tabs-mode: nil
#   c-indentation-style: bsd
# End:
# ex: set ts=8 sts=4 sw=4 tw=78 ft=pod expandtab shiftround :