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 guard the removal of smartmatch #22752

Open
book opened this issue Nov 16, 2024 · 11 comments · May be fixed by #22766
Open

Feature guard the removal of smartmatch #22752

book opened this issue Nov 16, 2024 · 11 comments · May be fixed by #22766

Comments

@book
Copy link
Contributor

book commented Nov 16, 2024

ae78ae4 merged the removal of smartmatch, which was planned for Perl v5.42.

After more discussion, the PSC believes it would be better to make the removal guarded by a feature.
(Similar to what was done with the indirect, multidimensional, bareword_filehandles and apostrophe_as_package_separator features.)

That feature would be included in the :default feature bundle and all bundles up to :5.40. It wouldn't be included in :5.42. The main benefit would be that none of the unmaintained modules on CPAN and elsewhere would break, while newer code (declaring the Perl version it codes against with use VERSION) would not be able to use smartmatch any more.

The proposed name for the feature is simply smartmatch.

Note: This goes against the idea proposed by @iabyn of creating an "air gap" of multiple versions without a smartmatch, that could allow to re-introduce a better smartmatch in the future. It is still possible to take this approach in an undetermined future.

@tonycoz
Copy link
Contributor

tonycoz commented Nov 18, 2024

There's no need for a new feature name - given/when/smartmatch was always historically guarded by the "switch" feature, changing this would break code that specifically enables (or disables, which I've done) the switch feature.

You're also suggesting adding this feature to the default bundle, even though it wasn't included in the default bundle previously. It was included in all other bundles (5.10 onwards).

So as far as I can see what needs to be done is:

  • revert the removal (reinstates "switch" to feature)
  • remove the feature from the 5.42 bundle
  • remove any deprecating messages/documentation

@tonycoz
Copy link
Contributor

tonycoz commented Nov 18, 2024

Actually, switch was removed from versioned bundles starting from v5.35, so it doesn't need to be removed from later bundles.

@Grinnz
Copy link
Contributor

Grinnz commented Nov 18, 2024

Unfortunately, the 'switch' feature only dictates the availability of the given/when keywords, not the smartmatch operator. So in my opinion, it's appropriate to add a corrolary 'smartmatch' feature and remove it from the bundle in the same way; and the 'switch' feature should require the 'smartmatch' feature as necessary for its implementation, and conversely disabling 'smartmatch' must disable 'switch'.

@book
Copy link
Contributor Author

book commented Nov 18, 2024

Yes, the point of no feature 'smartmatch' would be to disable ~~.

I think the two features are actually orthogonal: it's possible to use given/when without ~~, and it's also possible to use ~~ without given/when.

$ perl -E 'say $^V; say "a" ~~ qr/a/'
Smartmatch is deprecated at -e line 1.
v5.38.2
1

@tonycoz
Copy link
Contributor

tonycoz commented Nov 18, 2024

Thanks, I never used smartmatch beyond test code /me removes boot from mouth.

I'll add the smartmatch feature as mentioned.

The feature dependency is something new and I'm not sure how it should behave:

  • no feature "smartmatch"; - also disables the switch feature (as visible via the feature api):
use v5.10; # switch and smartmatch enabled
no feature "smartmatch"; # also force disables switch
use feature "smartmatch"; # but this doesn't enable switch
BEGIN { print feature::feature_enabled("switch"); } # false
  • no feature "smartmatch"; - prevents the switch keywords being recognized even if the "switch" feature is enabled (simplest to implement, I think)
use v5.10;
no feature "smartmatch"; 
BEGIN { print feature::feature_enabled("switch"); } # true
given ($x) { # syntax error
}
  • use feature "switch"; no feature "smartmatch"; - throws an error?

something else?

@Grinnz
Copy link
Contributor

Grinnz commented Nov 18, 2024

Yes, the point of no feature 'smartmatch' would be to disable ~~.

I think the two features are actually orthogonal: it's possible to use given/when without ~~, and it's also possible to use ~~ without given/when.

It is possible to use given/when without ~~, but it is implicitly used depending on the expression being matched, so I'm not sure what should happen if 'switch' is enabled but 'smartmatch' is not in those cases. I suggest doing whatever is simplest since both of these features will be deprecated/not available in modern code.

@book
Copy link
Contributor Author

book commented Nov 18, 2024

I think it's best to keep them independent.

If one does use feature 'switch'; and the implicit smartmatch, it should probably work. However, using an explicit ~~ without the smartmatch feature should fail (i.e. the feature enables the actual parsing of the operator).

@ap
Copy link
Contributor

ap commented Nov 18, 2024

I think it's best to keep them independent.

Concur.

@Leont
Copy link
Contributor

Leont commented Nov 19, 2024

If one does use feature 'switch'; and the implicit smartmatch, it should probably work. However, using an explicit ~~ without the smartmatch feature should fail (i.e. the feature enables the actual parsing of the operator).

That would complicate things, because when really does a ~~ op under the hood in such cases.

@haarg
Copy link
Contributor

haarg commented Nov 19, 2024

If it does a smartmatch under the hood, that's fine. The feature should only control the ~~ syntax, not the underlying op.

@tonycoz
Copy link
Contributor

tonycoz commented Nov 24, 2024

See PR #22766

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

Successfully merging a pull request may close this issue.

7 participants