Skip to content

rfc 0015, part 1 - warn on use of ' as :: in identifiers #20593

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 6, 2023
Merged

rfc 0015, part 1 - warn on use of ' as :: in identifiers #20593

merged 2 commits into from
Feb 6, 2023

Conversation

@tonycoz
Copy link
Contributor

@tonycoz tonycoz commented Dec 8, 2022

This implements the first part of RFC 0015, which states we should warn on the use of ' instead of :: in identifiers.

@rjbs
Copy link
Member

rjbs commented Dec 9, 2022

Hey, nice. I am not qualified to code review this, but am qualified to say "thanks!"

@nicomen
Copy link
Contributor

nicomen commented Dec 9, 2022

Nice, I never saw a reply on how to submit my work in the RFC itself Perl/PPCs#18, but my changes related to this are here if you want to take a look blead...nicomen:perl5:nicomen/warn_on_old_package_separator. I've been struggling with how to get a proper warning (suggesting the correct syntax) for sub names as the token contains a space.

@tonycoz
Copy link
Contributor Author

tonycoz commented Dec 10, 2022

Nice, I never saw a reply on how to submit my work in the RFC itself

Sorry, I didn't see that. I think the best way to get a code review is to make a PR, if you don't think it's ready to merge, make it draft.

abraxxa
abraxxa reviewed Dec 11, 2022
View reviewed changes
@tonycoz
Copy link
Contributor Author

tonycoz commented Dec 13, 2022

Some updates:

  • changed the removal version to 5.40
  • added a removal notice to the ' separator in quoted string warning
  • suppressed the warnings in two more core tests

A few bundled cpan modules also warn:

autodie:

Old package separator "'" deprecated at t/exception_class.t line 46.
Old package separator "'" deprecated at /home/tony/dev/perl/git/perl6/cpan/autodie/t/lib/pujHa/ghach.pm line 1.
Old package separator "'" deprecated at /home/tony/dev/perl/git/perl6/cpan/autodie/t/lib/pujHa/ghach/Dotlh.pm line 1.

../cpan/Scalar-List-Utils/t/blessed.t ................................ ok
../cpan/Scalar-List-Utils/t/dualvar.t ................................ ok
Old package separator "'" deprecated at (eval 285) line 1.
../cpan/Scalar-List-Utils/t/exotic_names.t ........................... ok
../cpan/Scalar-List-Utils/t/first.t .................................. ok
../cpan/Scalar-List-Utils/t/getmagic-once.t .......................... ok


io/socket.t .......................................................... ok
Old package separator "'" deprecated at test.pl line 124.
../cpan/Term-Cap/test.pl ............................................. ok

Old package separator "'" deprecated at t/parent-classfromclassfile.t line 21.
../cpan/parent/t/parent-classfromclassfile.t ......................... ok

I'll make PRs (or tickets) against upstreams once (if) this is merged.

@nicomen
Copy link
Contributor

nicomen commented Dec 13, 2022

Please look at the RFC for some already reported upstream bugs/pull requests that may need a nudge.

leonerd
leonerd reviewed Dec 16, 2022
View reviewed changes
Copy link
Contributor

@leonerd leonerd left a comment

Choose a reason for hiding this comment

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

Overall seems good, give or take some minor formatting comments in the tests.

As this is labelled "Part 1" I imagine it's not complete for the RFC yet. What further parts are required?

t/comp/package.t
print $xyz eq 'bar:main:new:xyz:ABC' ? "ok 1\n" : "not ok 1 '$xyz'\n";
} else {
print $xyz eq 'ABC:bar:main:new:xyz' ? "ok 1\n" : "not ok 1 '$xyz'\n";
print $xyz eq 'ABC:BEGIN:bar:main:new:xyz' ? "ok 1\n" : "not ok 1 '$xyz'\n";
Copy link
Contributor

@leonerd leonerd Dec 16, 2022

Choose a reason for hiding this comment

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

FYI: This behaviour of BEGIN blocks really annoys me. I have vague plans to fix it one day...

@tonycoz
Copy link
Contributor Author

tonycoz commented Dec 18, 2022

Overall seems good, give or take some minor formatting comments in the tests.

As this is labelled "Part 1" I imagine it's not complete for the RFC yet. What further parts are required?

The RFC calls them "steps":

https://github.com/Perl/RFCs/blob/main/rfcs/rfc0015.md#specification

and they belong in separate releases.

The first step is this, effectively deprecating ' in identifiers.

The second step is turning them into a syntax error, though it's unclear whether that should be a "directly produce a syntax error where ' is now accepted" or "don't accept ' in identifiers, the ' is part of the next token, which may produce a syntax error".

@tonycoz tonycoz force-pushed the warn-quote-ids branch 2 times, most recently from 025457f to 41aaec5 Compare December 18, 2022 23:01
@demerphq
Copy link
Collaborator

demerphq commented Feb 1, 2023

Is there anything needed on this still? It looks good to me.

@tonycoz
Enable the current "Old package separator used in string" by default
8d011e8 
If we're going to be removing this entirely, the behavior of
existing code is going to change.  Make sure everyone sees it.
@tonycoz
toke.c: deprecation warning for ' as a package separator
7cc9c41 
First stage of RFC 0015.

This also changes the warning for ' as package separator in
quoted strings to also be a deprecation warning.
leonerd
leonerd approved these changes Feb 5, 2023
View reviewed changes
Copy link
Contributor

@leonerd leonerd left a comment

Choose a reason for hiding this comment

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

LGTM.

Can we get this in in time for 5.37.9?

@tonycoz tonycoz merged commit 7e2d91e into Perl:blead Feb 6, 2023
@demerphq
Copy link
Collaborator

demerphq commented Feb 7, 2023

@tonycoz for the win. Thanks!

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

Reviewers

@demerphq demerphq demerphq approved these changes

@leonerd leonerd leonerd approved these changes

+1 more reviewer

@abraxxa abraxxa abraxxa left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tonycoz @rjbs @nicomen @demerphq @abraxxa @leonerd