Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feature] Perl 7: don't require modules to return true #17921

Open
dnmfarrell opened this issue Jul 1, 2020 · 12 comments
Open

[feature] Perl 7: don't require modules to return true #17921

dnmfarrell opened this issue Jul 1, 2020 · 12 comments

Comments

@dnmfarrell
Copy link

dnmfarrell commented Jul 1, 2020

Can we change Perl 7 to stop requiring modules to return true?

I wrote about this previously:

I don’t find this feature useful: if a module fails to initialize, it could call die with a meaningful error message, instead of returning false and Perl croaking with a generic message. I would wager that the majority of the time this exception is encountered, it’s because the programmer forgot to append a true value to their module code. If one ethos of Perl is optimizing for the common case, croaking on require returning false doesn’t seem to fit.

Module authors are free to continue appending 1; to their modules, and they will continue to work; Perl won't just demand it anymore :)

For the rare cases where authors depend on that behavior, it could be implemented as a feature which could be enabled by default in Perl 7. That is how I did it in the article.

If we agree this is an appropriate change, I'm happy to rebase and submit a PR but wanted to ask first.

Thanks

@Grinnz
Copy link
Contributor

Grinnz commented Jul 1, 2020

My disagreement with the idea of default enabling features notwithstanding, this would be a very useful feature to add, and I am not sure it even needs a feature flag.

@Corion
Copy link

Corion commented Jul 1, 2020

Even better IMO would be if the return value of a use (or require) statement could be used in the parent:

my $ua_class = use LWP::UserAgent;
my $ua = $ua_class->new();

This would mean that Perl (7) would have to cache the return value to make it available to all locations.

@Grinnz
Copy link
Contributor

Grinnz commented Jul 1, 2020

That is possible for require, but use is a compile time statement which physically cannot be used in a runtime expression (except via string eval which is definitely not something we should be encouraging).

@Corion
Copy link

Corion commented Jul 1, 2020

Yeah - I think use doesn't make much sense, but require would be good to have, for not having to hardcode the class name in a using module other than in the require line.

@dnmfarrell
Copy link
Author

Even better IMO would be if the return value of a use (or require) statement could be used in the parent

That's a nice idea but aren't you conflating modules and packages? Require can say "the eval of this module was successful". It doesn't know what packages in the module were eval'd.

@Corion
Copy link

Corion commented Jul 1, 2020

Yes, but what is returned is still up to the module. Maybe it returns a hash mapping functionality to class names, or whatever. But for the simple use case of a single classname that is exported from a module file, this would already be useful.

@dnmfarrell
Copy link
Author

Ah I think I see, you're proposing that require returns the value of the last statement in the module? It already does that today

package Foo;

{ packages => ['Foo'] }
perl -I. -MData::Dumper -wE 'print Dumper(require Foo);';
$VAR1 = {
          'packages' => [
                          'Foo'
                        ]
        };

@tobyink
Copy link
Contributor

tobyink commented Jul 2, 2020

Ah I think I see, you're proposing that require returns the value of the last statement in the module? It already does that today

Not quite. It only returns the last value the first time a file is required.

perl -I. -MData::Dumper -wE 'print Dumper(require Foo) for 1..2' ;
$VAR1 = {
          'packages' => [
                          'Foo'
                        ]
        };
$VAR1 = 1;

So you can't really rely on require to return anything useful, other than its return value being almost certainly true. (With some careful use of overloading, it might be possible to return an object that require itself thinks is success, but overloads boolification to be false.)

@scottchiefbaker
Copy link
Contributor

I just came to add my +1 to this idea. I think it's a great modernization.

@dnmfarrell
Copy link
Author

I can think of 3 ways it could be done:

  1. Introduce as a feature, enabled by default; this would make it compatible with the current proposal to have Perl 7 enable certain features/pragmas by default. It would also give users that still wanted to use it the ability to revert to the original behavior.
  2. Deprecation cycle, e.g. amend the exception message to say something like "relying on this exception is discouraged as it will be removed from a future version of Perl". Then remove it in the subsequent release
  3. Remove it straight away

What do you think?

Would compiling a custom 5.32 Perl with this feature removed and running a cpantesters server, comparing the results to 5.32 cpantester results be a good way to gauge the impact?

@bulk88
Copy link
Contributor

bulk88 commented Aug 1, 2020

A module at require time might want that a certain subclass/package/PM of a abstract class/package/virtual class/PM be already loaded, lets simplify the jargon to a "backend" must have been already loaded. If no backend is in the perl process, the PM returns 0 to require(). User can then test either for frontend package to be defined or retval of require() for T/F, if F, require("The::Backend") then require("The::Frontend") again. The backend may require so much configuration that there is no way for frontend to use/require any default backend without user custom code/custom config data. Hence the "return 0" and ability to load a PM a 2nd time needs to be preserved. PM files can also be silently truncated through STDIO or truncated on disk. by installers/archivers/TCP sockets/VM/kernel process limit exceeded. If the truncation happens at root PM scope in whitespace, its NOT a syntax/compile error. the return 1 protects against a file truncated on disk/over the network.

@dnmfarrell
Copy link
Author

A module at require time might want that a certain subclass/package/PM of a abstract class/package/virtual class/PM be already loaded, lets simplify the jargon to a "backend" must have been already loaded. If no backend is in the perl process, the PM returns 0 to require(). User can then test either for frontend package to be defined or retval of require() for T/F, if F, require("The::Backend") then require("The::Frontend") again.

In this scenario the user must have knowledge that the module has this quirk; otherwise it wouldn't know to retry when it fails to load. It must also be calling require in an eval block to catch the exception. In that case, why can't the module die instead? That's more powerful as the module can set the exception message which the user can parse and potentially take different actions on.

package Foo;

sub bar { print "Foobar\n" }

die "reload me\n" if time % 2;
1;
use strict;
use warnings;
use Time::HiRes 'sleep';

do {
  delete $INC{'Foo.pm'};
  eval { require Foo };
} while ($@ =~ /reload/);

Foo->bar;

PM files can also be silently truncated through STDIO or truncated on disk. by installers/archivers/TCP sockets/VM/kernel process limit exceeded. If the truncation happens at root PM scope in whitespace, its NOT a syntax/compile error. the return 1 protects against a file truncated on disk/over the network.

A better way to be sure you have loaded a file over a network (or another unreliable interface) would be to use a checksum. Relying on require returning true doesn't protect against this as a partially loaded file can also return a true value.

Even though I think this is misfeature, I agree that some users' code may depend on this behavior. I've proposed 2 ways we can mitigate that (via deprecation, via a feature). But I don't think it is common. As there is already a more powerful way to handle conditional load without relying on require returning true, I think we should optimize for the common case here and stop littering our modules with extra boilerplate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants