Skip to content

deprecate the ':all' feature bundle - #22817

Open
book wants to merge 1 commit into
bleadfrom
book/deprecate-all-feature-bundle
Open

deprecate the ':all' feature bundle#22817
book wants to merge 1 commit into
bleadfrom
book/deprecate-all-feature-bundle

Conversation

@book

@book book commented Dec 4, 2024

Copy link
Copy Markdown
Contributor

As discussed in Perl/PPCs#60, :all is very unlikely to be useful.

Since it will enable all features a future Perl might support, it will also enable some that might be incompatible with the code as written (see bitwise, for example, which changes the meaning of the existing bitwise operators).

With the addition of "negative" features in v5.32, :all became even less useful, since it would re-enable features deemed undesirable in modern Perl.

As this point in time, :all is effectively a footgun.

If the keyword all (from PPC0027) is added as a feature, there's an extra risk of confusion between use feature 'all' and use feature ':all'.

This patch makes :all warn. We should consider removing that bundle entirely in the future.


  • This set of changes requires a perldelta entry, and it is included.

As discussed in Perl/PPCs#60, `:all` is very unlikely to be useful.

Since it will enable all features a future Perl might support, it will
also enable some that might be incompatible with the code as written
(see `bitwise`, for example, which changes the meaning of the existing
bitwise operators).

With the addition of "negative" features in v5.32, `:all` became even
less useful, since it would re-enable features deemed undesirable in
modern Perl.

As this point in time, `:all` is effectively a footgun.

If the keyword `all` (from PPC0027) is added as a feature, there's an
extra risk of confusion between `use feature 'all'` and `use feature ':all'`.

This patch makes `:all` warn. We should consider removing that bundle
entirely in the future.
@Grinnz

Grinnz commented Dec 4, 2024

Copy link
Copy Markdown
Contributor

I believe this should be added as a deprecated:: category warning and listed in perldeprecation. But I am obviously in favor.

@Grinnz

Grinnz commented Dec 4, 2024

Copy link
Copy Markdown
Contributor

The feature docs should also mention the deprecation.

Comment thread regen/feature.pl
my $name = shift;
if (substr($name, 0, 1) eq ":") {
my $v = substr($name, 1);
carp('Feature bundle ":all" is deprecated and should not be used')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use

warnings::warnif(deprecated => "...");

so it's conditional on the category

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used carp because the rest of the module used croak for errors. Is there the equivalent for carp? Or should I just not bother?

Comment thread regen/feature.pl
Comment on lines +1220 to +1224
sub carp {
require Carp;
Carp::carp(@_);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed by using warnings::warnif

@tonycoz

tonycoz commented Dec 5, 2024

Copy link
Copy Markdown
Contributor

There are modules on CPAN that do use feature ":all";

https://grep.metacpan.org/search?q=use+feature.*%3Aall&qd=&qft=&qifl=

@Grinnz

Grinnz commented Dec 5, 2024

Copy link
Copy Markdown
Contributor

Not many hits (so far?); primarily looks like one author's test file and another author that should really be warned not to use such an unpredictable feature set which also keeps misfeatures around.

I'm curious if anyone has ever used no feature ":all".

@Grinnz

Grinnz commented Dec 5, 2024

Copy link
Copy Markdown
Contributor

https://grep.metacpan.org/search?size=20&_bb=443602641&q=no+feature.*%3Aall&qd=&qft=&qifl= finds one actual hit on CPAN for no feature ":all" in this test file: https://metacpan.org/release/OALDERS/TOML-Tiny-0.18/source/t/toml-test/valid/array/array.t - none of which do anything because they are immediately followed by using a real feature bundle.

@Grinnz

Grinnz commented Dec 5, 2024

Copy link
Copy Markdown
Contributor

And another interesting case is in App::EvalServerAdvanced's deparse handler, which removes such a construct from deparse output, something that might be worth double checking.

@tonycoz

tonycoz commented Dec 5, 2024

Copy link
Copy Markdown
Contributor

none of which do anything because they are immediately followed by using a real feature bundle.

Actually, this is useful.

use feature ":5.36"; leaves on the features that use v5.36; turns off such as indirect and bareword_filehandles. Doing:

no feature ":all";
use feature ":5.36";

ensures only the 5.36 bundle features are enabled, just as use v5.36 does.

@haarg

haarg commented Dec 5, 2024

Copy link
Copy Markdown
Contributor

The code in TOML::Tiny is horrible and should be fixed regardless of what happens with this ticket. But the reason it has no feature ':all' is because that is generated by B::Deparse: https://github.com/Perl/perl5/blob/blead/lib/B/Deparse.pm#L2168-L2170

@leonerd

leonerd commented Dec 5, 2024

Copy link
Copy Markdown
Contributor

Perhaps the warning should only be printed by use and not by no? That would be easy to detect as the boolean value of $import at warning time; just add && $import to the condition.

@Grinnz

Grinnz commented Dec 5, 2024

Copy link
Copy Markdown
Contributor

none of which do anything because they are immediately followed by using a real feature bundle.

Actually, this is useful.

use feature ":5.36"; leaves on the features that use v5.36; turns off such as indirect and bareword_filehandles. Doing:

no feature ":all";
use feature ":5.36";

ensures only the 5.36 bundle features are enabled, just as use v5.36 does.

This seems like an odd inconsistency.

@book

book commented Dec 5, 2024

Copy link
Copy Markdown
Contributor Author

I believe this should be added as a deprecated:: category warning and listed in perldeprecation. But I am obviously in favor.

Something like deprecated::use_feature_all?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants