Perl / PPCs

This repository is for Requests For Comments - proposals to change the Perl language.
61 stars 22 forks source link

Amores: add a `module` keyword with two modifiers (attributes). #11

Closed Ovid closed 2 years ago

Ovid commented 2 years ago

This is an RFC to add a module keyword and two related attributes to provide a procedural companion to Corinna.

Pre-RFC discussion is here.

Ovid commented 2 years ago

It should be noted that module keyword and the :version and :export modifiers could all be in separate RFCs. That would be easy enough. But the reason I preferred not to do that is because I am concerned that Perl development is often adding an ad-hoc feature here and there.

Perl doesn't seem to have a grand roadmap for advancing. I would like to change that by adopting a more holistic approach to features.

  1. Design features in groups that are thought out together.
  2. Limit new feature scopes to avoid the issue of simply having use v7 in legacy code break things.
  3. Adopt a more uniform KIM syntax (keyword, identifier, modifier. E.g., module Some::Module :version(42) {...}.
  4. Address a definition of done for feature groups necessary to say "this is Perl 7."

I know this approach doesn't seem to be very popular, but items 1 through 3 were adopted for the Corinna project and they've helped bring a lot of clarity to the project. Similarly, Amores follows this approach and coupled with Corinna, I think could be a release candidate for v7 (obviously not my call, though).

As part of this RFC, I deliberately didn't address the common issue of scripts, though that would have fit nicely. However, handling command line arguments seems to be something for the MAIN sub. This is likely blocked waiting on further work with signatures.

teodesian commented 2 years ago

Package blocks could make it a lot easier to mock things. Example:

# Supposing Quux is already used
local module Quux {
    sub someSub (...) { ... }
}

What to do about redefinition is an interesting one, as MockModule's strict redefine behavior could be useful to support here. Perhaps we could use a keyword other than 'local', say 'redefine' to not warn on redefines, and throw when package/subs/vars redefined don't exist.

Grinnz commented 2 years ago

Copying my comments from the other post.

As in the previous discussion on this, I feel that there are some useful feature requests here, but not a cohesive feature where a new feature flag and keyword (especially such a heavily charged keyword) is warranted.

  1. The :export feature, or some other standardized syntax to mark subs exportable in an intentional design, would be a huge improvement over the Exporter situation. I think this warrants an RFC and discussion on its own. At least as proposed, this should not require a feature flag or cause any incompatibility.

  2. You haven't explained how requiring a block scope is beneficial (other than the note in backwards compatibility, which doesn't seem a useful distinction when the proposal is intended to be feature-gated). Lexical declarations work perfectly fine without being attached to a block, and can be surrounded by a block if you need one, and the option to write a module as the entire contents of a file without an extra scope seems important to preserve, popular, and exactly as featureful.

  3. To wit, all of the features being enabled here are just an unversioned feature bundle (in the use VERSION sense). This seems like something that would quickly become an outdated featureset and interact confusingly with feature bundles that already exist.

  4. Version declarations in package statements are already-existing syntax. I don't feel there's a strong case to add a second slightly different version of it.

Putting this all together as you envision would be a fine job for a CPAN module, but for a core feature it is a tough sell for me, apart from the export mechanism.

eiro commented 2 years ago

Mostly agreed all the things I read. As always : thanks for your amazing job.

just 2 things:

xenu commented 2 years ago

Package blocks could make it a lot easier to mock things. Example:

# Supposing Quux is already used
local module Quux {
    sub someSub (...) { ... }
}

What to do about redefinition is an interesting one, as MockModule's strict redefine behavior could be useful to support here. Perhaps we could use a keyword other than 'local', say 'redefine' to not warn on redefines, and throw when package/subs/vars redefined don't exist.

This doesn't require a new keyword, local package { ... } would work just as well.

xenu commented 2 years ago

3. To wit, all of the features being enabled here are just an unversioned feature bundle (in the use VERSION sense). This seems like something that would quickly become an outdated featureset and interact confusingly with feature bundles that already exist.

Also, it's just not very useful. It doesn't remove the need for the boilerplate because you need it to enable the module keyword:

use v7;
module {
    ...
}
xenu commented 2 years ago

Why not stick with package?

Because I want us to have something clearly analogous to Corinna in terms of structure (KIM syntax) and clearly something new to show movement rather than "here's a new feature we just tossed in." That last bits important because I want a to avoid the issue of us just throwing random features at Perl independently without them being thought out as a whole.

As I said in the pre-RFC thread, I'm not convinced. Everything (apart from the implicit enabling of a feature bundle) this RFC proposed can be accomplished without adding a new keyword, even the :version attribute.

I do see some value in the :version syntax. It's extensible, it allows us to add new attributes in the future, and it's consistent with the syntax of the future keyword class (assuming it will be implemented as it's currently specified in the Corinna RFC). But it doesn't need the module keyword!

Grinnz commented 2 years ago
  • I think the signature should stay close to the name as those 2 info are the important ones in most cases.

This is how subroutine signatures work - the order of attributes and signatures have been changed twice already, and the current order is because the :lvalue attribute can affect the parsing of the signature so it needs to come first. This RFC doesn't have any effect on this so it would be something to discuss separately.

Grinnz commented 2 years ago

It should be noted that module keyword and the :version and :export modifiers could all be in separate RFCs. That would be easy enough. But the reason I preferred not to do that is because I am concerned that Perl development is often adding an ad-hoc feature here and there.

Perl doesn't seem to have a grand roadmap for advancing. I would like to change that by adopting a more holistic approach to features.

I appreciate the intent, but I don't agree. Adding ad-hoc features as appropriate is the most useful way to improve a language that already has decades of precedent in syntax and features. Corinna is a more exceptional case but I still would not want it to introduce an unrelated feature, if it could be added to the language more modularly.

  1. Limit new feature scopes to avoid the issue of simply having use v7 in legacy code break things.

I don't understand this point. If someone wants to use a new feature, they need to apply the new feature to the code where it will be used and reckon with incompatibility they may introduce - forced scoping doesn't buy anything there.

leonerd commented 2 years ago

Largely in agreement with what @Grinnz said; to summarize:

In addition: I don't see a need for the :version attribute (though I don't see a need for one in Corinna either) - perl has supported package Name v1.23; syntax ever since 5.14 (or was it 5.12? Either way; a very long time). CPAN, PAUSE, other bits of toolchain already cope with that just fine. There's no need to add another incompatible way to do the same thing.

I don't see why your original example can't be written:

use v7;
package Some::Utilities v3.14;

sub make_slug :export(strings) ( $name ) { ... }

sub constrain :export(numbers) ( $min, $num, $max = undef ) { ... }

sub weighted_pick :export(numbers) ( $weight_for ) { ... }

sub _binary_range ( $elem, $list ) { ... }

# note the lack of `1;`

Benefits:

On those latter two points: I could imagine that use v7 enables some features to get those. Lets call them for sake of argument

use feature qw( export_attr yield_true )
teodesian commented 2 years ago

This doesn't require a new keyword, local package { ... } would work just as well.

Supposing you run under 'no warnings' (or don't care if warnings are emitted), and don't really care if the underlying thing exists or not this is true, yes. I doubt many people operate under those assumptions. The whole point of a new keyword would be to bring in the useful redefinition mode from MockModule, but that is beside the point I am making about mocking.

Block scoped packages beats doing what we do now, where you have to say local for every symbol you want to redefine, and disable warnings temporarily, or call a MockModule sub. Saves lines and strokes without sacrificing the ability to reason about the code.

oodler577 commented 2 years ago

This very much reminds me of Fortrran's attempt at OOP. I am also very confused about how this is to provide some symmetry wrt Corinna - as I have asked in my email during the preliminary discussion, what's the "play" here? In other words, what's the roadmap regarding this "non-oop" or "non-corinna" entry in the procedural realm and where it may meet up with what corinna is trying to provide? I must strongly echo the sentiments expressed by some that the module keyword is going to be extremely confusing. I recommend you either call it something else entirely or determine if there are some backcompat changes or extensions to what can be done now - either by way of package enhancements or some idioms that might get "close" but could be made complete with some scoped and consistent enhancements. Ultimately, this seems like what you're wanting is to create a new idiom - which is perfectly well and good - but if a new keyword (especially such an overloaded term) can be avoided, then that's the right path for Perl 5. (more) I also think it is prudent to really think through to how this will impact the current idiomic landscape, and however unintentional, may begin to produce perl "dialects" that eventually lead to a tower of bable effect. We already see this when it comes to the different tongues already existing: babby perl, modern perl, pbp perl,. Moo/Moose perl, etc. On interpreter, many tongues - doesn't seem like a think that should be encouraged. (more) e.g., what improvements to use, require, do, package, local, and bless (among others) could we all benefit from? There's gotta be something there.

xenu commented 2 years ago

Supposing you run under 'no warnings' (or don't care if warnings are emitted), and don't really care if the underlying thing exists or not this is true, yes. I doubt many people operate under those assumptions. The whole point of a new keyword would be to bring in the useful redefinition mode from MockModule, but that is beside the point I am making about mocking.

@teodesian: I don't understand what adding support for local to package has to do with warnings.

teodesian commented 2 years ago

@teodesian: I don't understand what adding support for local to package has to do with warnings.

Example of one standard approach to mock functions:

no warnings qw{redefine once};
local *Package::some_subroutine = sub {...}
use warnings

If we do a block scoped package redefine, I would assume the same concerns about warnings would apply.

My point about MockModule / redefine:

use Test::Mockmodule qw{strict};
my $mm = Test::MockModule->new('Module::To::Mock');
$mm->redefine('some_subroutine', sub {...});

It's a lot less typing and things to think about by just having a 'redefine' keyword handle all that for you.

xenu commented 2 years ago

Oh, so your point was specifically about local, not about module vs package. Then this discussion is unrelated to this RFC.

(BTW, that redefine warning is IMO a bug, so I've made a PR to fix it: perl/perl5#19371)

leonerd commented 2 years ago

@Ovid I think the :export attribute could stand alone well in its own RFC. Do you want to write one up, or should I?

A couple of further thoughts on it:

neilb commented 2 years ago

As per response on p5p, we are rejecting this in the current state.

We'd like to see something in this area (making it easy to create modules that are libraries of functions, dealing with importing and exporting), but would like to see something with a broader, more unifying scope.