Repository navigation
Add support for except* handling and starred subscripts
#6522
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
3dea814
c37fc4f
62bb015
fbcf0b8
9b94189
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -471,7 +471,16 @@ impl Compiler { | |||||
| } | ||||||
| } else { | ||||||
| // VISIT(c, expr, e->v.Subscript.slice) | ||||||
| self.compile_expression(slice)?; | ||||||
| match slice { | ||||||
| Expr::Starred(starred) => { | ||||||
| self.starunpack_helper( | ||||||
| &[Expr::Starred(starred.clone())], | ||||||
| 0, | ||||||
| CollectionType::Tuple, | ||||||
| )?; | ||||||
| } | ||||||
| other => self.compile_expression(other)?, | ||||||
| } | ||||||
|
|
||||||
| // Emit appropriate instruction based on context | ||||||
| match ctx { | ||||||
|
|
@@ -2121,12 +2130,150 @@ impl Compiler { | |||||
|
|
||||||
| fn compile_try_star_statement( | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| &mut self, | ||||||
| _body: &[Stmt], | ||||||
| _handlers: &[ExceptHandler], | ||||||
| _orelse: &[Stmt], | ||||||
| _finalbody: &[Stmt], | ||||||
| body: &[Stmt], | ||||||
| handlers: &[ExceptHandler], | ||||||
| orelse: &[Stmt], | ||||||
| finalbody: &[Stmt], | ||||||
| ) -> CompileResult<()> { | ||||||
| Err(self.error(CodegenErrorType::NotImplementedYet)) | ||||||
| let handler_block = self.new_block(); | ||||||
| let finally_block = self.new_block(); | ||||||
|
|
||||||
| if !finalbody.is_empty() { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather have it split like CPython does it This makes it easier to maintain when looking for the diff between our impl and theirs |
||||||
| emit!( | ||||||
| self, | ||||||
| Instruction::SetupFinally { | ||||||
| handler: finally_block, | ||||||
| } | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| let else_block = self.new_block(); | ||||||
|
|
||||||
| emit!( | ||||||
| self, | ||||||
| Instruction::SetupExcept { | ||||||
| handler: handler_block, | ||||||
| } | ||||||
| ); | ||||||
| self.compile_statements(body)?; | ||||||
| emit!(self, Instruction::PopBlock); | ||||||
| emit!(self, Instruction::Jump { target: else_block }); | ||||||
|
|
||||||
| self.switch_to_block(handler_block); | ||||||
| // incoming stack: [exc] | ||||||
| emit!( | ||||||
| self, | ||||||
| Instruction::CallIntrinsic1 { | ||||||
| func: bytecode::IntrinsicFunction1::EnsureExceptionGroup | ||||||
| } | ||||||
| ); | ||||||
| // stack: [remainder] | ||||||
|
|
||||||
| for handler in handlers { | ||||||
| let ExceptHandler::ExceptHandler(ExceptHandlerExceptHandler { | ||||||
| type_, name, body, .. | ||||||
| }) = handler; | ||||||
|
|
||||||
| let skip_block = self.new_block(); | ||||||
| let next_block = self.new_block(); | ||||||
|
|
||||||
| // Split current remainder for this handler | ||||||
| if let Some(exc_type) = type_ { | ||||||
| self.compile_expression(exc_type)?; | ||||||
| } else { | ||||||
| return Err(self.error(CodegenErrorType::SyntaxError( | ||||||
| "except* must specify an exception type".to_owned(), | ||||||
| ))); | ||||||
| } | ||||||
|
|
||||||
| emit!( | ||||||
| self, | ||||||
| Instruction::CallIntrinsic2 { | ||||||
| func: bytecode::IntrinsicFunction2::ExceptStarMatch | ||||||
| } | ||||||
| ); | ||||||
| emit!(self, Instruction::UnpackSequence { size: 2 }); // stack: [rest, match] | ||||||
| emit!(self, Instruction::CopyItem { index: 1_u32 }); // duplicate match for truthiness test | ||||||
| emit!(self, Instruction::ToBool); | ||||||
| emit!(self, Instruction::PopJumpIfFalse { target: skip_block }); | ||||||
|
|
||||||
| // Handler selected, stack: [rest, match] | ||||||
| if let Some(alias) = name { | ||||||
| self.store_name(alias.as_str())?; | ||||||
| } else { | ||||||
| emit!(self, Instruction::Pop); | ||||||
| } | ||||||
|
|
||||||
| self.compile_statements(body)?; | ||||||
|
|
||||||
| if let Some(alias) = name { | ||||||
| self.emit_load_const(ConstantData::None); | ||||||
| self.store_name(alias.as_str())?; | ||||||
| self.compile_name(alias.as_str(), NameUsage::Delete)?; | ||||||
| } | ||||||
|
|
||||||
| emit!(self, Instruction::Jump { target: next_block }); | ||||||
|
|
||||||
| // Skip handler when no match (stack currently [rest, match]) | ||||||
| self.switch_to_block(skip_block); | ||||||
| emit!(self, Instruction::Pop); // drop match | ||||||
| emit!(self, Instruction::Jump { target: next_block }); | ||||||
|
|
||||||
| // Continue with remaining exceptions | ||||||
| self.switch_to_block(next_block); | ||||||
| } | ||||||
|
|
||||||
| let handled_block = self.new_block(); | ||||||
|
|
||||||
| // If remainder is truthy, re-raise it | ||||||
| emit!(self, Instruction::CopyItem { index: 1_u32 }); | ||||||
| emit!(self, Instruction::ToBool); | ||||||
| emit!( | ||||||
| self, | ||||||
| Instruction::PopJumpIfFalse { | ||||||
| target: handled_block | ||||||
| } | ||||||
| ); | ||||||
| emit!( | ||||||
| self, | ||||||
| Instruction::Raise { | ||||||
| kind: bytecode::RaiseKind::Raise | ||||||
| } | ||||||
| ); | ||||||
|
|
||||||
| // All exceptions handled | ||||||
| self.switch_to_block(handled_block); | ||||||
| emit!(self, Instruction::Pop); // drop remainder (None) | ||||||
| emit!(self, Instruction::PopException); | ||||||
|
|
||||||
| if !finalbody.is_empty() { | ||||||
| emit!(self, Instruction::PopBlock); | ||||||
| emit!(self, Instruction::EnterFinally); | ||||||
| } | ||||||
|
|
||||||
| emit!( | ||||||
| self, | ||||||
| Instruction::Jump { | ||||||
| target: finally_block, | ||||||
| } | ||||||
| ); | ||||||
|
|
||||||
| // try-else path | ||||||
| self.switch_to_block(else_block); | ||||||
| self.compile_statements(orelse)?; | ||||||
|
|
||||||
| if !finalbody.is_empty() { | ||||||
| emit!(self, Instruction::PopBlock); | ||||||
| emit!(self, Instruction::EnterFinally); | ||||||
| } | ||||||
|
|
||||||
| self.switch_to_block(finally_block); | ||||||
| if !finalbody.is_empty() { | ||||||
| self.compile_statements(finalbody)?; | ||||||
| emit!(self, Instruction::EndFinally); | ||||||
| } | ||||||
|
|
||||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
| fn is_forbidden_arg_name(name: &str) -> bool { | ||||||
|
|
||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests are good. Move it to append to |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| def test_except_star_with_plain_exception(): | ||
| try: | ||
| raise ValueError("x") | ||
| except* ValueError as err: | ||
| assert isinstance(err, ExceptionGroup) | ||
| assert err.exceptions == (ValueError("x"),) | ||
| else: | ||
| raise AssertionError("except* handler did not run") | ||
|
|
||
|
|
||
| def test_starred_index_builds_tuple(): | ||
| > | ||
| target[*"ab"] = 1 | ||
| assert list(target.items()) == [(("a", "b"), 1)] | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| test_except_star_with_plain_exception() | ||
| test_starred_index_builds_tuple() |
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.
I don't see this logic in CPython:
https://github.com/python/cpython/blob/627894459a84be3488a1789919679c997056a03c/Python/compile.c#L6669-L6678
Assuming that this works, I'd like to have a comment explaining why we are deviating from the CPython logic