-
Notifications
You must be signed in to change notification settings - Fork 597
Fatalization of goto LABEL construct #23782
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
base: blead
Are you sure you want to change the base?
Conversation
t/lib/croak/pp_ctl
Outdated
no warnings 'deprecated'; | ||
goto f; | ||
foreach(1){f:} | ||
EXPECT | ||
Can't "goto" into the middle of a foreach loop at - line 3. | ||
Use of goto to jump into a construct is no longer permitted at - line 1. | ||
######## | ||
# NAME goto into given | ||
no warnings 'deprecated'; | ||
goto f; | ||
CORE::given(1){f:} | ||
EXPECT | ||
Can't "goto" into a "given" block at - line 3. | ||
Use of goto to jump into a construct is no longer permitted at - line 1. | ||
######## | ||
# NAME goto from given topic expression | ||
no warnings 'deprecated'; | ||
CORE::given(goto f){f:} | ||
EXPECT | ||
Can't "goto" into a "given" block at - line 2. | ||
Use of goto to jump into a construct is no longer permitted at - line 1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if we could keep the more detailed messages here, though that would take some figuring out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In blead, those other exception messages are defined in pp_ctl.c in the S_check_op_type
function:
3229 S_check_op_type(pTHX_ OP * const o)
3230 {
3231 /* Eventually we may want to stack the needed arguments
3232 * for each op. For now, we punt on the hard ones. */
3233 /* XXX This comment seems to me like wishful thinking. --sprout */
3234 if (o == UNENTERABLE)
3235 croak(
3236 "Can't \"goto\" into a binary or list expression");
3237 if (o->op_type == OP_ENTERITER)
3238 croak(
3239 "Can't \"goto\" into the middle of a foreach loop");
3240 if (o->op_type == OP_ENTERGIVEN)
3241 croak(
3242 "Can't \"goto\" into a \"given\" block");
3243 }
3244
Note the inline comments and misgivings in the above -- all from b537774 by Father C back in 2017.
The warning about impending fatalization occurs later on in the same file, in PP(pp_goto)
:
3247 PP(pp_goto)
3248 {
...
3649 if (*enterops && enterops[1]) {
3650 I32 i = enterops[1] != UNENTERABLE
3651 && enterops[1]->op_type == OP_ENTER && in_block
3652 ? 2
3653 : 1;
3654 if (enterops[i])
3655 deprecate_fatal_in(WARN_DEPRECATED__GOTO_CONSTRUCT,
3656 "5.42",
3657 "Use of \"goto\" to jump into a construct");
3658 }
...
3705 }
In blead, we see these messages here:
$ ack 'into the middle of a foreach loop|into a binary or list expression|into a.+?given.+?block' *.c t/
pp_ctl.c
3236: "Can't \"goto\" into a binary or list expression");
3239: "Can't \"goto\" into the middle of a foreach loop");
3242: "Can't \"goto\" into a \"given\" block");
t/lib/croak/pp_ctl
7:Can't "goto" into the middle of a foreach loop at - line 3.
14:Can't "goto" into a "given" block at - line 3.
20:Can't "goto" into a "given" block at - line 2.
29:Can't "goto" into a binary or list expression at - line 2.
30:Can't "goto" into a binary or list expression at - line 3.
31:Can't "goto" into a binary or list expression at - line 4.
32:Can't "goto" into a binary or list expression at - line 5.
t/op/goto.t
286: like($@, qr/Can't "goto" into the middle of a foreach loop/,
t/op/eval.t
228: like($@, qr/Can't "goto" into the middle of a foreach loop/,
Now I acknowledge that I don't understand why, in blead, the deprecation warning is sometimes encountered first, whereas other times 1 of the 3 croak
messages is encountered first.
In the branch, we see them here:
$ ack 'into the middle of a foreach loop|into a binary or list expression|into a.+?given.+?block' *.c t/
pp_ctl.c
3236: "Can't \"goto\" into a binary or list expression");
3239: "Can't \"goto\" into the middle of a foreach loop");
3242: "Can't \"goto\" into a \"given\" block");
t/lib/croak/pp_ctl
26:Can't "goto" into a binary or list expression at - line 2.
27:Can't "goto" into a binary or list expression at - line 3.
28:Can't "goto" into a binary or list expression at - line 4.
29:Can't "goto" into a binary or list expression at - line 5.
t/op/eval.t
228: like($@, qr/Can't "goto" into the middle of a foreach loop/,
The 3 messages are still defined. 2 of the 3 are still exercised in either t/lib/croak.t
or t/op/eval.t
. The message about a given
block is no longer exercised. If the point of this fatalization is to have an exception throw whenever we jump to a LABEL that is located inside a "construct" (a poorly defined term, IMO), then I don't see why we need the 3 more specific messages at all anymore. If you want to retain them and test for them, you should make an argument for that (though that would constitute mission creep, IMO).
But, as I acknowledged in earlier posts, I've never used goto
myself, so I could be missing something.
Note: I welcome suggestions as to how we handle the entry in
Further note: Once we implement this fatalization, do we still need these others?
|
As I said earlier, I think the more detailed messages are more useful - "construct" is vague, "into the middle of a foreach loop" is specific and gives the user a clue of exactly what they're doing that is forbidden. |
ae4d500
to
aa8cd16
Compare
This pull request supersedes #23677. (I am filing a new pull request in part because in my own GH repo I changed the name of the branch I wish to merge into blead; network search suggests that filing a new p.r. is the easiest way to proceed.) This commit message incorporates aspects of the earlier p.r.'s commit message.
I've been working on this project over the past month-and-a-half. I am now ready to propose a plan to implement the fatalization of the behavior currently guarded by warnings category
deprecated::goto_construct
. This pull request would resolve #23618.This project is, in effect, a feature removal process, so we have to move carefully. If we remove a feature from core, we also have to remove code testing or otherwise relying on that feature. We have to remove documentation about that feature except insofar as we discuss the removal or suggest other ways to approach the business problem the feature was previously intended to solve. We then have to assess how the feature removal would affect Perl code out in the wild -- which may, in turn, require developing new programs (outside of the core distribution) to facilitate that assessment. I have done all that, so this pull request is being submitted in non-draft status with the hope that we can get it into blead well before the Feb 20 2026 (v5.43-8) "contentious changes" deadline for our next production release in May 2026.
On September 1 we completed one stage in this project. In #23659 (c00b151), we moved many of the code blocks in
t/op/goto.t
which we anticipate will not be affected by this fatalization to new filet/op/goto-sub.t
.The remaining stages then became:
Preliminary code changes in the Perl guts, including removal of the existing deprecation warning and its replacement with a new exception. To support the new exception we'll probably have to update documentation in
pod/perldeprecation.pod
andperldoc -f goto
. We should getmake test_porting
to PASS. We should identify tests in the test suite other thant/op/goto*.t
which need to test for an exception rather than a warning. All of this should, IMO, be done before we tacklet/op/goto.t
head-on. With the exception of needing more detail in the entry for the fatalization inpod/perldiag.pod
, all of this has been done in this pull request.Getting the main test files to PASS. That file is
t/op/goto.t
, which now passes largely because I have simply stripped out all the code that pertained to the now fatal feature. The extent to which the code remaining in that file exercises ("covers") the remaininggoto
features needs investigation.Identifying the scope of Blead Breaks CPAN (BBC) problems. Since CPANtesters is set up on the premise that we're only testing CPAN distros against perl production/maintenance releases or commits to
blead
, we'll have to figure out a way to test against a branch without polluting the CPANtesters matrix. I have done that by revising one of my previous CPAN distributions and releasing it as Test-Against-Commit. I'll include details of my research using that library in a separate post.Please review. Thank you very much.