Modules::RequireExplicitInclusion trips on structures in constant.pm

[xsawyerx]

If I set a construct with constant.pm, it will trip Modules::RequireExplicitInclusion. For example:

use constant {
    'FOO' => {},
};

use constant 'BAR' => [];

FOO->{'foo'} = 'bar';

BAR->[0] = 'baz';

Has the following failures:

[Modules::RequireExplicitInclusion] Use of "FOO" without including "FOO" at eg.pl line 7, near 'FOO->{'foo'} = 'bar';'
[Modules::RequireExplicitInclusion] Use of "BAR" without including "BAR" at eg.pl line 9, near 'BAR->[0] = 'baz';'

This is hard to detect, but maybe we need a configuration option that only flags methods and fuctions, rather than hash or array calls.

[petdance]

This is a bummer for me, too. I've got a couple of modules where I have to no critic my constants because of this.

[roh-roh]

I'm not familiar with Perl::Critic::Utils, but adding these checks to https://github.com/Perl-Critic/Perl-Critic-StricterSubs/blob/master/lib/Perl/Critic/Policy/Modules/RequireExplicitInclusion.pm#L102 solved it for me

 && $elem->next_token->next_token->isa('PPI::Token::Word')
 && is_method_call( $elem->next_token->next_token ) }

[xsawyerx]

I thought about this further and I'm not sure why it trips.

If it sees FOO (or Foo, for that matter), it doesn't know whether it's a class or a constant. But if it's a class, you wouldn't be able to do FOO->{...}. You would have to call a method on it: FOO::BAR->{...}.

So it seems like these are mutually exclusive. FOO::BAR->{...} means the class was FOO, not FOO::BAR.

Furthermore, if we make it skip variables ($Foo::Bar::Thingie=...), it's even less likely to trip.

Basically:

$Foo::Bar = ...; # Don't trip (or at most, make it configurable)
FOO->{...}; # Don't trip. Definitely a constant and not a method of a class
FOO::BAR->{...}; # Violation! Definitely sub call in class 'FOO', whether constant or not.
FOO::BAR(); # Violation! Definitely sub call.

It's very possible the above suggestion fixes this exact issue.

[petdance]

but adding these checks .... solved it for me

What do those checks do?

[roh-roh]

What do those checks do?

From what I understand reading Perl::Critic::Utils docs, FOO->{...} is considered a class name by design, as it has -> on it's right

is_class_name( $element )
Given a PPI::Token::Word, returns true if the element that immediately follows this element is the dereference operator "->". When a bareword has a "->" on the right side, it usually means that it is the name of the class (from which a method is being called).

I added a check that -> is followed by a word, not sure if is_method_call is needed there as it only checks that element has -> to it's left.
And it probably will skip FOO->$method_name calls, but again, I'm totally unfamiliar with Perl::Critic::Utils :)

[petdance]

Whatever change gets made to handle this case, we definitely need tests that check for the behavior and that it doesn't break other behavior.