Error message for require "./base.pm" is wrong and confusing

p5pRT commented 7 years ago

Migrated from rt.perl.org#131098 (status was 'resolved')

Searchable as RT131098$

p5pRT commented 7 years ago

From @haarg

This is a bug report for perl from haarg@haarg.org\, generated with the help of perlbug 1.40 running under perl 5.24.1.

The error message generated by a require like "require './base.pm';" is confusing and incorrect in a number of ways.

$ perl -e'require "./base.pm";' Can't locate ./base.pm in @INC (you may need to install the .::base module) (@INC contains: /Users/gknop/perl5/perls/v5.24.1/lib/site_perl/5.24.1/darwin-2level /Users/gknop/perl5/perls/v5.24.1/lib/site_perl/5.24.1 /Users/gknop/perl5/perls/v5.24.1/lib/5.24.1/darwin-2level /Users/gknop/perl5/perls/v5.24.1/lib/5.24.1 .) at -e line 1.

First\, it points you towards the nonsensical module ".::base". Second\, it indicates that perl tried to search @INC\, when for files starting with ./ or ../ perl opens them directly without any search. This is a hard coded special case. Third\, if perl had actually searched @INC\, ./base.pm would have been found.

This still applies to 5.25.11 and blead.

Flags: category=core severity=low

Site configuration information for perl 5.24.1:

Configured by gknop at Mon Apr 3 13:20:23 CEST 2017.

Summary of my perl5 (revision 5 version 24 subversion 1) configuration: Derived from: 443bd156a6baaf7a8fe6b6b05fcf6c4178140ed2 Platform: osname=darwin\, osvers=16.4.0\, archname=darwin-2level uname='darwin cuneus 16.4.0 darwin kernel version 16.4.0: thu dec 22 22:53:21 pst 2016; root:xnu-3789.41.3~3release_x86_64 x86_64 ' config_args='-des -Dusedevel -Uversiononly -Dprefix=/Users/gknop/perl5/perls/v5.24.1 -Uman1dir -Uman3dir' hint=recommended\, useposix=true\, d_sigaction=define useithreads=undef\, usemultiplicity=undef use64bitint=define\, use64bitall=define\, uselongdouble=undef usemymalloc=n\, bincompat5005=undef Compiler: cc='cc'\, ccflags ='-fno-common -DPERL_DARWIN -mmacosx-version-min=10.12 -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -DPERL_USE_SAFE_PUTENV'\, optimize='-O3'\, cppflags='-fno-common -DPERL_DARWIN -mmacosx-version-min=10.12 -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include' ccversion=''\, gccversion='4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.38)'\, gccosandvers='' intsize=4\, longsize=8\, ptrsize=8\, doublesize=8\, byteorder=12345678\, doublekind=3 d_longlong=define\, longlongsize=8\, d_longdbl=define\, longdblsize=16\, longdblkind=3 ivtype='long'\, ivsize=8\, nvtype='double'\, nvsize=8\, Off_t='off_t'\, lseeksize=8 alignbytes=8\, prototype=define Linker and Libraries: ld='cc'\, ldflags =' -mmacosx-version-min=10.12 -fstack-protector-strong -L/usr/local/lib' libpth=/usr/local/lib /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/clang/8.1.0/lib /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib /usr/lib libs=-lpthread -lgdbm -ldbm -ldl -lm -lutil -lc perllibs=-lpthread -ldl -lm -lutil -lc libc=\, so=dylib\, useshrplib=false\, libperl=libperl.a gnulibc_version='' Dynamic Linking: dlsrc=dl_dlopen.xs\, dlext=bundle\, d_dlsymun=undef\, ccdlflags=' ' cccdlflags=' '\, lddlflags=' -mmacosx-version-min=10.12 -bundle -undefined dynamic_lookup -L/usr/local/lib -fstack-protector-strong'

Locally applied patches:

@INC for perl 5.24.1: /Users/gknop/perl5/perls/v5.24.1/lib/site_perl/5.24.1/darwin-2level /Users/gknop/perl5/perls/v5.24.1/lib/site_perl/5.24.1 /Users/gknop/perl5/perls/v5.24.1/lib/5.24.1/darwin-2level /Users/gknop/perl5/perls/v5.24.1/lib/5.24.1

Environment for perl 5.24.1: DYLD_LIBRARY_PATH (unset) HOME=/Users/gknop LANG=en_US.UTF-8 LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/usr/local/sbin:/usr/local/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin PERL_BADLANG (unset) SHELL=/bin/bash

p5pRT commented 7 years ago

From @iabyn

On Tue\, Apr 04\, 2017 at 04:42:50AM -0700\, Graham Knop wrote:

The error message generated by a require like "require './base.pm';" is confusing and incorrect in a number of ways.

$ perl -e'require "./base.pm";' Can't locate ./base.pm in @INC (you may need to install the .::base module) (@INC contains: /Users/gknop/perl5/perls/v5.24.1/lib/site_perl/5.24.1/darwin-2level /Users/gknop/perl5/perls/v5.24.1/lib/site_perl/5.24.1 /Users/gknop/perl5/perls/v5.24.1/lib/5.24.1/darwin-2level /Users/gknop/perl5/perls/v5.24.1/lib/5.24.1 .) at -e line 1.

First\, it points you towards the nonsensical module ".::base". Second\, it indicates that perl tried to search @INC\, when for files starting with ./ or ../ perl opens them directly without any search. This is a hard coded special case. Third\, if perl had actually searched @INC\, ./base.pm would have been found.

This still applies to 5.25.11 and blead.

So there are two issues here. Firstly between 5.6.0 and 5.8.0 with this commit: perl-5.6.0-7584-gbe4b629\, require stopped using two different error messages depending on whether @INC was searched:

$ perl561o -e'@INC=(); require "nosuch"' Can't locate nosuch in @INC (@INC contains:) at -e line 1. $ perl561o -e'@INC=(); require "./nosuch"' Can't locate ./nosuch at -e line 1. $

$ perl580o -e'@INC=(); require "nosuch"' Can't locate nosuch in @INC (@INC contains:) at -e line 1. $ perl580o -e'@INC=(); require "./nosuch"' Can't locate ./nosuch in @INC (@INC contains:) at -e line 1.

Secondly\, 5.18.0 added the helpful "(you may need to install the Foo::Bar module)" hint to the error message if the filename ended in .pm\, but which produces nonsensical module name suggestions if the pathname is prefixed with ./ or ../.

The second issue would probably be fixed by virtue of fixing the first issue (since it will no longer try to print the long-form error message for those types of pathname).

So does everyone agree that the change in 5.8.0 is a bug? Does this need fixing for 5.26.0? We're doing a lot of last-minute fixups for do/@INC \, but my personal feeling is that it can wait.

-- You're only as old as you look.

p5pRT commented 7 years ago

The RT System itself - Status changed from 'new' to 'open'

p5pRT commented 7 years ago

From @haarg

On Wed\, Apr 5\, 2017 at 10:27 AM\, Dave Mitchell \davem@iabyn\.com wrote:

On Tue\, Apr 04\, 2017 at 04:42:50AM -0700\, Graham Knop wrote:

The error message generated by a require like "require './base.pm';" is confusing and incorrect in a number of ways.

$ perl -e'require "./base.pm";' Can't locate ./base.pm in @INC (you may need to install the .::base module) (@INC contains: /Users/gknop/perl5/perls/v5.24.1/lib/site_perl/5.24.1/darwin-2level /Users/gknop/perl5/perls/v5.24.1/lib/site_perl/5.24.1 /Users/gknop/perl5/perls/v5.24.1/lib/5.24.1/darwin-2level /Users/gknop/perl5/perls/v5.24.1/lib/5.24.1 .) at -e line 1.

First\, it points you towards the nonsensical module ".::base". Second\, it indicates that perl tried to search @INC\, when for files starting with ./ or ../ perl opens them directly without any search. This is a hard coded special case. Third\, if perl had actually searched @INC\, ./base.pm would have been found.

This still applies to 5.25.11 and blead.

So there are two issues here. Firstly between 5.6.0 and 5.8.0 with this commit: perl-5.6.0-7584-gbe4b629\, require stopped using two different error messages depending on whether @INC was searched:
$ perl561o \-e'@&#8203;INC=; require "nosuch"'
Can't locate nosuch in @&#8203;INC $@&#8203;INC contains&#8203;:$ at \-e line 1\.
$ perl561o \-e'@&#8203;INC=; require "\./nosuch"'
Can't locate \./nosuch at \-e line 1\.
$

$ perl580o \-e'@&#8203;INC=; require "nosuch"'
Can't locate nosuch in @&#8203;INC $@&#8203;INC contains&#8203;:$ at \-e line 1\.
$ perl580o \-e'@&#8203;INC=; require "\./nosuch"'
Can't locate \./nosuch in @&#8203;INC $@&#8203;INC contains&#8203;:$ at \-e line 1\.
Secondly\, 5.18.0 added the helpful "(you may need to install the Foo::Bar module)" hint to the error message if the filename ended in .pm\, but which produces nonsensical module name suggestions if the pathname is prefixed with ./ or ../.

The second issue would probably be fixed by virtue of fixing the first issue (since it will no longer try to print the long-form error message for those types of pathname).

There are other paths that will show this message\, even if they can't be mapped to a proper module name. 'require "Some/Non-Identifier/Module.pm";' for example.

So does everyone agree that the change in 5.8.0 is a bug? Does this need fixing for 5.26.0? We're doing a lot of last-minute fixups for do/@INC \, but my personal feeling is that it can wait.

I do think addressing this is a bit higher priority than it has been in the past. Many of the fixes for removing '.' from @INC involve adding './' to the paths used. Most of those fixes are for uses of do\, but some are for require.

p5pRT commented 7 years ago

From @xsawyerx

On 04/05/2017 10:56 AM\, Graham Knop wrote:

On Wed\, Apr 5\, 2017 at 10:27 AM\, Dave Mitchell \davem@iabyn\.com wrote:
On Tue\, Apr 04\, 2017 at 04:42:50AM -0700\, Graham Knop wrote:

The error message generated by a require like "require './base.pm';" is confusing and incorrect in a number of ways.

$ perl -e'require "./base.pm";' Can't locate ./base.pm in @INC (you may need to install the .::base module) (@INC contains: /Users/gknop/perl5/perls/v5.24.1/lib/site_perl/5.24.1/darwin-2level /Users/gknop/perl5/perls/v5.24.1/lib/site_perl/5.24.1 /Users/gknop/perl5/perls/v5.24.1/lib/5.24.1/darwin-2level /Users/gknop/perl5/perls/v5.24.1/lib/5.24.1 .) at -e line 1.

First\, it points you towards the nonsensical module ".::base". Second\, it indicates that perl tried to search @INC\, when for files starting with ./ or ../ perl opens them directly without any search. This is a hard coded special case. Third\, if perl had actually searched @INC\, ./base.pm would have been found.

This still applies to 5.25.11 and blead. So there are two issues here. Firstly between 5.6.0 and 5.8.0 with this commit: perl-5.6.0-7584-gbe4b629\, require stopped using two different error messages depending on whether @INC was searched:
$ perl561o \-e'@&#8203;INC=; require "nosuch"'
Can't locate nosuch in @&#8203;INC $@&#8203;INC contains&#8203;:$ at \-e line 1\.
$ perl561o \-e'@&#8203;INC=; require "\./nosuch"'
Can't locate \./nosuch at \-e line 1\.
$

$ perl580o \-e'@&#8203;INC=; require "nosuch"'
Can't locate nosuch in @&#8203;INC $@&#8203;INC contains&#8203;:$ at \-e line 1\.
$ perl580o \-e'@&#8203;INC=; require "\./nosuch"'
Can't locate \./nosuch in @&#8203;INC $@&#8203;INC contains&#8203;:$ at \-e line 1\.
Secondly\, 5.18.0 added the helpful "(you may need to install the Foo::Bar module)" hint to the error message if the filename ended in .pm\, but which produces nonsensical module name suggestions if the pathname is prefixed with ./ or ../.

The second issue would probably be fixed by virtue of fixing the first issue (since it will no longer try to print the long-form error message for those types of pathname). There are other paths that will show this message\, even if they can't be mapped to a proper module name. 'require "Some/Non-Identifier/Module.pm";' for example.

I think it makes sense to show @INC only when @INC is being searched. I wouldn't object if someone thinks it's *considerably* useful to have it regardless\, but at the very least\, we shouldn't suggest installing a module when we're attempting to load a file\, not a module. Having @INC noted there can throw you off to thinking this is the reason (but you can argue it's additional details)\, but giving a suggestion that is simply notes a definitely wrong "solution\," we should fix it.

(In theory.)

So does everyone agree that the change in 5.8.0 is a bug? Does this need fixing for 5.26.0? We're doing a lot of last-minute fixups for do/@INC \, but my personal feeling is that it can wait. I do think addressing this is a bit higher priority than it has been in the past. Many of the fixes for removing '.' from @INC involve adding './' to the paths used. Most of those fixes are for uses of do\, but some are for require.

The question is to which extent. Would we now bikeshed through the usability (or possible compat breakage) for removing @INC indication on non-@INC-search loading? Are we discussing simply removing the suggestion when it isn't searched?

In any case\, I think 5.26.1 might be a better place for this than 5.26.0\, to be honest. ("Think\," not "decisive.")

p5pRT commented 7 years ago

From @iabyn

On Tue\, Apr 11\, 2017 at 12:08:17PM +0200\, Sawyer X wrote:

The question is to which extent. Would we now bikeshed through the usability (or possible compat breakage) for removing @INC indication on non-@INC-search loading? Are we discussing simply removing the suggestion when it isn't searched?

In any case\, I think 5.26.1 might be a better place for this than 5.26.0\, to be honest. ("Think\," not "decisive.")

If we're going to change it\, we either need to change it for 5.26.0\, or leave it for 5.28.0. Changing it in 5.26.1 would be the worst option - if it breaks anything\, we don't want to do that in a maint release.

I'm keen to work up a patch to fix this issue\, which will also help me understand its scope. If its scope turns out to manageable\, then I'll recommend it goes in 5.26.0.

-- You're only as old as you look.

p5pRT commented 7 years ago

From @iabyn

On Thu\, Apr 13\, 2017 at 11:00:40AM +0100\, Dave Mitchell wrote:

If we're going to change it\, we either need to change it for 5.26.0\, or leave it for 5.28.0. Changing it in 5.26.1 would be the worst option - if it breaks anything\, we don't want to do that in a maint release.

I'm keen to work up a patch to fix this issue\, which will also help me understand its scope. If its scope turns out to manageable\, then I'll recommend it goes in 5.26.0.

I've just pushed the branch davem/require_fixups containing four commits.

The first two commits just add more code comments around the require() code and tidy up slightly.

The third fixes the 5.8.0 regression: the require error message no longer mentions @INC if @INC wasn't searched.

The fourth causes the "(you may need to install the Foo::Bar module)" hint part of require's error message to be included in fewer cases. Specifically\, it used to be emitted for any pathname ending in .pm; now it only does so for C\<require Bare::Word>:

previously:
require Foo::Bar # (you may need to install the Foo::Bar module) require "Foo/Bar.pm" # (you may need to install the Foo::Bar module) require "./foo.pm" # (you may need to install the .::foo module)
now:
require Foo::Bar # (you may need to install the Foo::Bar module) require "Foo/Bar.pm" # no hint require "./foo.pm" # no hint

The first two commits have no functional changes so are not a risk for 5.26.0. The third one didn't break any core tests\, although it could in theory break CPAN tests which expect a particular format of error message.

The fourth is the same as the third\, except that it also broke

cpan/parent/t/parent.t

which was testing for a particular error message in

eval q{use parent 'reallyReAlLyNotexists'};

Since parent.pm does

require "$_.pm"

Its not a bareword and the hint no longer appears.

I think the first 3 commits should go into 5.26.0; I'm not so certain about the fourth.

-- Fire extinguisher (n) a device for holding open fire doors.

p5pRT commented 7 years ago

From zefram@fysh.org

Dave Mitchell wrote:

The fourth causes the "(you may need to install the Foo::Bar module)" hint part of require's error message to be included in fewer cases. Specifically\, it used to be emitted for any pathname ending in .pm; now it only does so for C\<require Bare::Word>:

That sounds like a bad idea. Previously a require on the .pm filename has been equivalent to a bareword require\, and the equivalence is useful. With respect to utility of the error message\, advice to install a specified module is useful whenever the module file is being searched for in @INC\, regardless of which way that was invoked.

Since parent.pm does

require "$_.pm"

Its not a bareword and the hint no longer appears.

This is an example of how the equivalence between bareword require and filename require is useful.

I think the installation advice should appear in the message when\, and only when\, the search was in @INC for a filename that would result from a syntactically well-formed module name. Thus "Foo/Bar.pm" should yield a message advising about Foo::Bar (as it presently does); "Foo/Bar.pl" should not\, because it's not of the right pattern to arise from a module name; "Foo-Bar.pm" should not\, because it too can't arise from a well-formed module name; and "./Foo.pm" should not\, because it doesn't search in @INC.

-zefram

p5pRT commented 7 years ago

From @kentfredric

On 14 April 2017 at 00:24\, Dave Mitchell \davem@iabyn\.com wrote:

    require "Foo/Bar\.pm"   \# no hint

As with what Zefram said\, this not being seen the same as

require Foo::Bar

In error handling/user space is just weird and surprising.

Especially since such a transformation is *prolific* in Module loaders

https://metacpan.org/source/BINGOS/Module-Load-0.32/lib/Module/Load.pm#L103

https://metacpan.org/source/BINGOS/Module-Load-0.32/lib/Module/Load.pm#L76-77

-- Kent

KENTNL - https://metacpan.org/author/KENTNL

p5pRT commented 7 years ago

From @iabyn

On Fri\, Apr 14\, 2017 at 10:41:19AM +1200\, Kent Fredric wrote:

On 14 April 2017 at 00:24\, Dave Mitchell \davem@iabyn\.com wrote:
    require "Foo/Bar\.pm"   \# no hint
As with what Zefram said\, this not being seen the same as

require Foo::Bar

In error handling/user space is just weird and surprising.

Especially since such a transformation is *prolific* in Module loaders

https://metacpan.org/source/BINGOS/Module-Load-0.32/lib/Module/Load.pm#L103

https://metacpan.org/source/BINGOS/Module-Load-0.32/lib/Module/Load.pm#L76-77

I've now pushed a revised branch: davem/require_fixups2

which has the same set of commits as before\, except for the final one\, which has been replaced with the following. I'll merge in a day of two if no-one objects.

commit 0c25175c3e81ecb7a371d88eafea390328681258 Author: David Mitchell \davem@iabyn\.com AuthorDate: Sun Apr 16 09:50:04 2017 +0100 Commit: David Mitchell \davem@iabyn\.com CommitDate: Sun Apr 16 10:54:09 2017 +0100

emit require module name err hint only when valid
RT #131098
The helpful "you may need to install" hint which 'require' sometimes includes in its error message these days (split across multiple lines for clarity):
$ perl -e'require Foo::Bar' Can't locate Foo/Bar.pm in @INC (you may need to install the Foo::Bar module) (@INC contains: ... ) at ...
is a bit over-enthusiastic when the pathname hasn't actually been derived from a module name:
$ perl -e'require "Foo.+/%#Bar.pm"' Can't locate Foo.+%#Bar.pm in @INC (you may need to install the Foo.+::%#Bar module) (@INC contains: ... ) at ...
This commit changes things so that the hint message is only emitted if the reverse-mapped module name is legal as a bareword:
$ perl -e'require "Foo.+/%#Bar.pm"' Can't locate Foo.+%#Bar.pm in @INC (@INC contains: ... ) at ...

Affected files ... M pp_ctl.c M t/op/require_errors.t

-- The Enterprise's efficient long-range scanners detect a temporal vortex distortion in good time\, allowing it to be safely avoided via a minor course correction. -- Things That Never Happen in "Star Trek" #21

p5pRT commented 7 years ago

From @xsawyerx

On 04/13/2017 12:00 PM\, Dave Mitchell wrote:

On Tue\, Apr 11\, 2017 at 12:08:17PM +0200\, Sawyer X wrote:

The question is to which extent. Would we now bikeshed through the usability (or possible compat breakage) for removing @INC indication on non-@INC-search loading? Are we discussing simply removing the suggestion when it isn't searched?

In any case\, I think 5.26.1 might be a better place for this than 5.26.0\, to be honest. ("Think\," not "decisive.") If we're going to change it\, we either need to change it for 5.26.0\, or leave it for 5.28.0. Changing it in 5.26.1 would be the worst option - if it breaks anything\, we don't want to do that in a maint release.

D'oh! Yes\, of course. This cannot be in a maint release.

I'm keen to work up a patch to fix this issue\, which will also help me understand its scope. If its scope turns out to manageable\, then I'll recommend it goes in 5.26.0.

I think this alone dictates we should have 5.25.12.

While I have earlier expressed a recognition of the focusing value of urgency\, I fear such urgency will yield more concern and less total benefit. In other words\, stressing over 5.26.0 coming out with no further delay likely won't help much.

p5pRT commented 7 years ago

From @iabyn

On Sun\, Apr 16\, 2017 at 11:04:38AM +0100\, Dave Mitchell wrote:

On Fri\, Apr 14\, 2017 at 10:41:19AM +1200\, Kent Fredric wrote:
On 14 April 2017 at 00:24\, Dave Mitchell \davem@iabyn\.com wrote:
    require "Foo/Bar\.pm"   \# no hint
As with what Zefram said\, this not being seen the same as

require Foo::Bar

In error handling/user space is just weird and surprising.

Especially since such a transformation is *prolific* in Module loaders

https://metacpan.org/source/BINGOS/Module-Load-0.32/lib/Module/Load.pm#L103

https://metacpan.org/source/BINGOS/Module-Load-0.32/lib/Module/Load.pm#L76-77
I've now pushed a revised branch: davem/require_fixups2

which has the same set of commits as before\, except for the final one\, which has been replaced with the following. I'll merge in a day of two if no-one objects.

Now merged:

305ff1d [MERGE] fix require's croak message d31614f emit require module name err hint only when valid 4b62894 require die msg: only mention @INC if used 13e8e86 S_require_file() : simplify an else if block f0dea69 better comment require() source.

-- The Enterprise successfully ferries an alien VIP from one place to another without serious incident. -- Things That Never Happen in "Star Trek" #7

p5pRT commented 7 years ago

@iabyn - Status changed from 'open' to 'resolved'

Perl / perl5