Skip to content

An initial attempt at try/catch syntax #18505

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

Merged
merged 2 commits into from
Feb 4, 2021
Merged

An initial attempt at try/catch syntax #18505

merged 2 commits into from
Feb 4, 2021

Conversation

leonerd
Copy link
Contributor

@leonerd leonerd commented Jan 23, 2021

This PR adds try/catch notation, of the form specified by #18504; namely

use feature 'try';

try {
    a_function();
}
catch ($e) {
    warn "An error occurred: $e";
}

When merged this is sufficient to pass the unit tests in the Feature::Compat::Try module at v0.02.

In addition, at some point once this feature is merged it would be good to have a big trawl through much of the documentation (e.g. starting with eval in perlfunc) to look for opportunities to draw attention to the new syntax, as well as fixing up examples currently written on eval (several of which I expect will contain bugs because of it).

@leonerd
Copy link
Contributor Author

leonerd commented Jan 23, 2021

Some comments to reviewers:

  • I decided to rename the existing CXp_TRYBLOCK and CxTRYBLOCK macros to EVALBLOCK instead, as they do not relate to the true try blocks introduced here. Some back-compat macros exist to avoid breaking existing code, but the new forms should be preferred henceforth. They have identical semantics as before, they were just renamed.
  • I added a CXp_TRY bit flag to the existing CXt_EVAL structure in the context stack, in favour of introducing a whole new type of perhaps CXt_TRY. I may still need to do the latter in order to fix the bug/performance issues mentioned in the above PR message, though that would lead to more code in more places being disturbed, so this path was chosen as the minimum viable to get something committed sooner.
  • The current implementation does not need to add any new opcodes, but I feel that for performance I will later need to add an OP_CATCH, or perhaps for supporting a typed dispatch feature, a few of them.
  • I have not at all attempted to fix B::Deparse yet. The new syntax does not correctly deparse. While B::Deparse does create some output that looks plausible, it is not in fact correct because it hasn't noticed the OPf_SPECIAL flag on the OP_ENTERTRY. I intend to fix this in a followup commit.

@leonerd leonerd force-pushed the leonerd/feature-try branch 2 times, most recently from 4500339 to 89208a4 Compare January 23, 2021 23:31
@leonerd leonerd requested review from tonycoz and iabyn January 23, 2021 23:46
@tonycoz
Copy link
Contributor

tonycoz commented Jan 25, 2021

It looks reasonable to me, though I think the false exception object issue needs to be fixed before it is merged.

@leonerd
Copy link
Contributor Author

leonerd commented Jan 26, 2021

PTAL. Many updates:

  • Have now added an OP_CATCH which can distinguish the "exception object is false" case, and added a unit test for it.
  • pp_catch now also handles the my VAR = $@ logic, making it faster and less noisy in the optree
  • This new opcode is also a handy signal to the still-as-yet unfixed B::Deparse module
  • pp_catch now clears the $@ variable, to ensure users don't accidentally end up relying on it. They must use the declared var instead

Will also edit the main PR description to match.

@leonerd
Copy link
Contributor Author

leonerd commented Jan 26, 2021

PS I shall definitely squash most (or all?) of the commits together before it gets merged.. most of the commit messages shouldn't really stay in the merged copy.

@tonycoz
Copy link
Contributor

tonycoz commented Jan 26, 2021

I wonder, should using try/catch (ideally) be changing $@ at all? I mean like:

my $x = eval "..."; # some other eval
try {
  # do something with $@
}
catch ($y) {
  # do something with $@
}

That said, I don't really expect this to work

@Grinnz
Copy link
Contributor

Grinnz commented Jan 26, 2021

This was discussed in the p5p thread, I believe the answer was "ideally yes but it's way more difficult for the moment"

@Grinnz
Copy link
Contributor

Grinnz commented Jan 26, 2021

Rather, in #18504

@leonerd
Copy link
Contributor Author

leonerd commented Jan 27, 2021

@tonycoz

I wonder, should using try/catch (ideally) be changing $@ at all?

That's certainly a potential end goal, yes. As @Grinnz points out currently the related handling of die etc.. is somewhat too entangled with ERRSV ($@) to easily extract. Eventually I could imagine a CXt_TRY that stored an SV of its (lexical) exception variable, for die to write there instead. But currently that is too much adjustment of other code.

@leonerd leonerd force-pushed the leonerd/feature-try branch 2 times, most recently from a8569bf to 98a092a Compare January 27, 2021 22:31
@leonerd
Copy link
Contributor Author

leonerd commented Jan 27, 2021

Squashed into fewer commits with better messages; rebased onto current blead.

@leonerd leonerd force-pushed the leonerd/feature-try branch from 98a092a to 830d235 Compare January 28, 2021 18:36
@leonerd
Copy link
Contributor Author

leonerd commented Feb 2, 2021

Trying to find the right balance between forward velocity, and cautiousness on new features. This has been reviewed by a few people now. I have sent it to @iabyn to review but so far there's no word at all from him - not even to acknlowledge the request
I think therefore I'll wait for any other comments from folks for a couple more days, but maybe aim that if nothing else comes up I'll hit the Merge button on Thursday evening

It'd be good to get it tried out in a few 5.33.x point releases ahead of 5.34

CxTRYBLOCK would be confusing when we add a real CxTRY for try/catch

Also renames the associated CXp_TRYBLOCK flag to CXp_EVALBLOCK
@leonerd leonerd force-pushed the leonerd/feature-try branch from 830d235 to 52ee75a Compare February 4, 2021 14:10
 * Add feature, experimental warning, keyword
 * Basic parsing
 * Basic implementation as optree fragment

See also
  #18504
@leonerd leonerd force-pushed the leonerd/feature-try branch from 52ee75a to a1325b9 Compare February 4, 2021 14:22
@leonerd leonerd merged commit a1325b9 into blead Feb 4, 2021
@leonerd leonerd deleted the leonerd/feature-try branch February 8, 2021 22:49
@xsawyerx
Copy link
Member

xsawyerx commented Mar 5, 2021

I don't think this should have been merged. The Perl Steering Committee is still discussing Perl 7 and waited for that discussion to be resolved before addressing new features like this one.

@leonerd
Copy link
Contributor Author

leonerd commented Mar 5, 2021

I see. Hmm - well a little late for that now.

I did announce my intentions loudly and clearly on each of of this issue in github, the perl5-porters@ mailing list, and the #p5p IRC channel on irc.perl.org. I gave well over a week of notice and got many OKs from folks all around, so I judged it to be fine.

Was either the form or duration of my notice period somehow inadequate?

@wchristian
Copy link
Contributor

I'd like to have an explanation of how this feature could possibly be more contentious or dangerous than anything previously scoped to Perl 7?

(That is the only reason i could imagine for comparing this with Perl 7.)

As an additional question, is that a comment intended to represent PSC or a private opinion?

@shadowcat-mst
Copy link
Contributor

@xsawyerx this feature was planned before you announced your original perl7 plan and in fact had had significantly wider public discussion within the community before implementation was started than the perl7 plan did.

This has broad community support and there's no reason to oppose its merging here - if anything you might consider the process that led to this merge to be a model of how do such things successfully by building broad consensus out in the open first.

@xsawyerx
Copy link
Member

xsawyerx commented Mar 8, 2021

I don't think this feature is contentious or problematic. I don't think the manner in which it was discussed is a problem. I realize this was planned and worked on prior to the Perl 7 announcement. Thank you all for "informing" me of this, but none of these are my issues with it.

The problem is a strategic one relating to a possible release of a Perl 7. If we release Perl 7, we leave Perl 5 with an experimental feature that will not have a stable release approving it. This might be okay and we're discussing it in the Council, but this merge forces us in this direction.

@Grinnz
Copy link
Contributor

Grinnz commented Mar 8, 2021

There are several features currently experimental. If a plan for Perl 7 can't account for that then it will have to be improved.

@Leont
Copy link
Contributor

Leont commented Mar 8, 2021

I don't think this feature is contentious or problematic. I don't think the manner in which it was discussed is a problem. I realize this was planned and worked on prior to the Perl 7 announcement. Thank you all for "informing" me of this, but none of these are my issues with it.

I suspect you were saying what you were saying made sense in the context of a PSC discussion that none of us are part of. I think you should expect to keep being misunderstood as long as the rest of us aren't provided the full context of what you're saying.

The problem is a strategic one relating to a possible release of a Perl 7. If we release Perl 7, we leave Perl 5 with an experimental feature that will not have a stable release approving it. This might be okay and we're discussing it in the Council, but this merge forces us in this direction.

That could easily be fixed by a sunsetting release (python learned the hard way just how important those are), I don't see how this would be a hard problem to solve.

@wchristian
Copy link
Contributor

wchristian commented Mar 8, 2021

There are several features currently experimental. If a plan for Perl 7 can't account for that then it will have to be improved.

Can you make a list of those?

you should expect to keep being misunderstood as long as the rest of us aren't provided the full context of what you're saying

I wouldn't necessarily go that far, but i agree that when making a claim without a fully grounded justification, some confusion is likely.

And agreed as well, if experimentality of things is an issue, then it seems there should be a sunsetting release with no threads unresolved.

@Grinnz
Copy link
Contributor

Grinnz commented Mar 8, 2021

There are several features currently experimental. If a plan for Perl 7 can't account for that then it will have to be improved.

Can you make a list of those?

They are listed in perlexperiment (of which I note, try is missing).

@xsawyerx
Copy link
Member

xsawyerx commented Mar 8, 2021

Good points. I guess I was hasty in my comment on this. I apologize.

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 this pull request may close these issues.

8 participants