-
Notifications
You must be signed in to change notification settings - Fork 84
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
C# 6.0 feature: Exception Filters #2
Conversation
d64e5ea
to
8d415d3
Compare
@jskeet This feature PR is ready for committee review. |
Right... we should probably have an alias that we can assign as a reviewer which will ping everyone. Not quite sure how that works though, and I'm signing off for the night... I can look tomorrow though. |
8d415d3
to
64f25ea
Compare
In the diff's I'm seeing changed body text about exception filters and the non-terminal |
I originally created this proposal in the conversion-to-markdown repo as PR107. It had two commits, to update statements.md and lexical-structure.md. Later, @BillWagner force-pushed that work over here, but as best as I can tell, the front part of the statements.md commit got lost, and needs to be put back. (Bill, being a novice GitHub user, rather than trying to figure our how to salvage that missing text from the commit, I'm showing it [the complete/replacement try_statement grammar] here, so Nigel can continue his review.) try_statement
: 'try' block catch_clause+
| 'try' block finally_clause
| 'try' block catch_clause+ finally_clause
;
catch_clause
: 'catch' exception_specifier? exception_filter? block
;
exception_specifier
: '(' type identifier? ')'
;
exception_filter
: 'when' '(' expression ')'
;
finally_clause
: 'finally' block
; @BillWagner - I updated this PR with the above grammar on 2021/07/15 |
Thanks for the proposed grammar Rex, makes it easier :-) An observation on the grammar, I'll decide later whether it is more than that (i.e. this is my note pad!) For comparison this is the current grammar in ANTLR: try_statement
: 'try' block catch_clauses
| 'try' block catch_clauses* finally_clause
;
catch_clauses
: specific_catch_clause+
| specific_catch_clause* general_catch_clause
;
specific_catch_clause
: 'catch' '(' type identifier? ')' block
;
general_catch_clause
: 'catch' block
;
finally_clause
: 'finally' block
; Notes:
Conclusion? None, for now just an observation... |
Lines 1170 - 1184/5 (I couldn't figure out how to comment on a range of lines which ends in an unchanged line, on the other hand starting with an unchanged line is straightforward): In trying to merge the introduction of exception clauses into this built list algorithm the grammar goes wrong (e.g. "The first ... is" -> "If a ... it is" - the it is missing), and clauses are also "considered a match" and then "not a match" or re-"considered a match". Rather than try to address such things piecemeal here is a first attempt, probably too nested, to describe the revised algorithm with bullet points, including dropping some overly (subjective) repetitive bits:
|
standard/statements.md
Outdated
@@ -1103,17 +1103,17 @@ There are three possible forms of `try` statements: | |||
- A `try` block followed by a `finally` block. | |||
- A `try` block followed by one or more `catch` blocks followed by a `finally` block. |
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.
This original para & bullet list are redundant, the forms are not referenced elsewhere and it simply a prose description of the try_statement
production. In the original that production only has two alternatives:
try_statement
: 'try' block catch_clauses
| 'try' block catch_clauses* finally_clause
;
while the proposal expands this to three, maybe just to match this (original) bullet list, though the described syntax is identical.
The descriptions of other statements do not generally start with a prose description of the statement structure, so there is not a pattern being followed here, however the switch statement does.
Option 1: 😄 Remove this para & bullet list
Option 2: 🚀 Follow the model of the switch statement and replaced with a more succinct description which doesn't introduce "forms". Something like:
A try_statement consists of the keyword
try
followed by a block, then zero or more catch_clauses, then an optional finally_clause. There must be at least one catch_clause or a finally_clause.
Option 3: 👀 Leave as is
If Option 1 or 2 is chosen then the grammar for try_statement can revert to the two alternative original version, of option 3 is chosen then there is a small argument for the proposed three alternative version.
A slightly less indented replacement for lines 1170 - 1184/5:
I can't say I like this bulleted prose description of the semantics and none of the versions (original, proposal, my two attempts to fix and clean it up) include what happens if an exception is thrown during the finally block – that is covered in a following para – so they are all incomplete... Incorporating the missing bits would further complicate/nest the bullets, maybe a sequence of short paras would be better, and I say that as someone who generally prefers the economy of bulleted lists to the verbosity of prose! ;-) |
Yes, I think a sequence of short paragraphs would make sense here. I would suggest that we don't merge this PR, but instead create a parallel PR that also addresses #53. We can then compare the two easily and see which we like better. |
(Removed label so that we can reapply it when the parallel PR has been created.) |
@Nigel-Ecma: Currently this is assigned to you - did you volunteer to create the parallel PR mentioned above? |
@jskeet: Not that I recall... I'll have to review the issues and threads to see if I've got content for any parallel PR. |
For discussion tomorrow: If @Nigel-Ecma doesn't have a draft, I can take the notes and content here and create the new PR. |
1b961c3
to
44f4564
Compare
Add "when" as a contextual keyword. Update grammar for exception filters Add description of exception filters.
44f4564
to
512b4d6
Compare
Status update: Most of the comments above have been addressed. The following are still unresolved:
|
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.
Some mall changes suggested.
standard/statements.md
Outdated
|
||
When a `catch` clause specifies both a *class_type* and an *Identifier*, an ***exception variable*** of the given name and type is declared. The exception variable corresponds to a local variable with a scope that extends over the `catch` block. During execution of the `catch` block, the exception variable represents the exception currently being handled. For purposes of definite assignment checking, the exception variable is considered definitely assigned in its entire scope. | ||
When a `catch` clause specifies both a *class_type* and an *Identifier*, an ***exception variable*** of the given name and type is declared. The exception variable corresponds to a local variable with a scope that extends over the `catch` block. During execution of the *exception_filter* and `catch` block, the exception variable represents the exception currently being handled. For purposes of definite assignment checking, the exception variable is considered definitely assigned in its entire scope. |
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 seems odd that the exception is already "being handled" during execution of the exception_filter even though an "exception handler" has not been located yet.
Co-authored-by: Neal Gafter <[email protected]>
Back in the mists of time (Dec 20) I remarked that the revised grammar which adds exception filters ( Being of the view that a reference grammar should be as precise as reasonable (while a compiler may use a more lenient grammar to assist error messages and recovery) here is a version of the grammar which maintains the syntactic restriction: try_statement
: 'try' block catch_clauses
| 'try' block catch_clauses* finally_clause
;
catch_clauses
: specific_catch_clause+
| specific_catch_clause* general_catch_clause
;
specific_catch_clause
: 'catch' exception_specifier exception_filter? block
: 'catch' exception_filter block
;
exception_specifier
: '(' type identifier? ')'
;
exception_filter
: 'when' '(' boolean_expression ')'
;
general_catch_clause
: 'catch' block
;
finally_clause
: 'finally' block
; Notes:
I’ll argue this version of the grammar is better as a reference grammar as it embodies all of the syntactic requirements. (And if case someone asks, sorry I haven't come up with a series of short paras to replace the nested bullets, over to a wordsmith ;-)) |
Recording my vote here for option 2 described above - a succinct description. |
Further actions:
|
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.
Approved after the grammar and "option 2" changes have been applied.
All comments from Neal have been addressed.
Update the grammar and the description of the `try` statement. See #2 (comment)
* feature exception filters Add "when" as a contextual keyword. Update grammar for exception filters Add description of exception filters. * respond to suggestions * restore grammar updates * Apply suggestions from code review Co-authored-by: Neal Gafter <[email protected]> * Update recommended at meeting Update the grammar and the description of the `try` statement. See dotnet/csharpstandard#2 (comment) Co-authored-by: Neal Gafter <[email protected]>
Add "when" as a contextual keyword.
Update grammar for exception filters
Add description of exception filters.