Skip to content
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

Desugar IfLet* expr to match #3064

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Desugar IfLet* expr to match #3064

wants to merge 1 commit into from

Conversation

dkm
Copy link
Member

@dkm dkm commented Jun 23, 2024 •
edited
Loading

Replace the "regular" AST->HIR lowering for IfLet* with a desugaring
into a MatchExpr.

Desugar a simple if let:

   if let Some(y) = some_value {
     bar();
   }

into:

 match some_value {
    Some(y) => {bar();},
    _ => ()
  }

Same applies for IfLetExprConseqElse (if let with an else block).

Desugar:

   if let Some(y) = some_value {
     bar();
   } else {
     baz();
   }

into:


   match some_value {
     Some(y) => {bar();},
     _ => {baz();}
   }

Fixes #1177

@dkm dkm self-assigned this Jun 23, 2024
@dkm dkm marked this pull request as draft June 23, 2024 21:58
CohenArthur
CohenArthur reviewed Jun 24, 2024
View reviewed changes
gcc/rust/hir/rust-ast-lower-block.h
@@ -115,7 +115,7 @@ class ASTLoweringIfLetBlock : public ASTLoweringBase
using Rust::HIR::ASTLoweringBase::visit;

public:
static HIR::IfLetExpr *translate (AST::IfLetExpr &expr)
static HIR::MatchExpr *translate (AST::IfLetExpr &expr)
Copy link
Member

Choose a reason for hiding this comment

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

ah, interesting - it'll be nice if we can remove the HIR::IfLetExpr node entirely from the compiler!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and that's a lot more code than I expected. I did the experiment tonight.

@dkm dkm force-pushed the dkm/iflet_desugar branch 3 times, most recently from 54e06f0 to 19a5647 Compare July 2, 2024 21:06
@dkm
rust: Desugar IfLet* into MatchExpr
70b157e 
Replace the "regular" AST->HIR lowering for IfLet* with a desugaring
into a MatchExpr.

Desugar a simple if let:

   if let Some(y) = some_value {
     bar();
   }

into:

   match some_value {
     Some(y) => {bar();},
     _ => ()
   }

Same applies for IfLetExprConseqElse (if let with an else block).

Desugar:

   if let Some(y) = some_value {
     bar();
   } else {
     baz();
   }

into:

   match some_value {
     Some(y) => {bar();},
     _ => {baz();}
   }

Fixes #1177

gcc/rust/ChangeLog:

	* backend/rust-compile-block.h: Adjust after removal of
	HIR::IfLetExpr and HIR::IfLetExprConseqElse.
	* backend/rust-compile-expr.h: Likewise.
	* checks/errors/borrowck/rust-bir-builder-expr-stmt.cc
	(ExprStmtBuilder::visit): Likewise.
	* checks/errors/borrowck/rust-bir-builder-expr-stmt.h: Likewise.
	* checks/errors/borrowck/rust-bir-builder-lazyboolexpr.h:
	Likewise.
	* checks/errors/borrowck/rust-bir-builder-struct.h: Likewise.
	* checks/errors/borrowck/rust-function-collector.h: Likewise.
	* checks/errors/privacy/rust-privacy-reporter.cc
	(PrivacyReporter::visit): Likewise.
	* checks/errors/privacy/rust-privacy-reporter.h: Likewise.
	* checks/errors/rust-const-checker.cc (ConstChecker::visit):
	Likewise.
	* checks/errors/rust-const-checker.h: Likewise.
	* checks/errors/rust-unsafe-checker.cc (UnsafeChecker::visit):
	Likewise.
	* checks/errors/rust-unsafe-checker.h: Likewise.
	* hir/rust-ast-lower-block.h (ASTLoweringIfLetBlock::translate):
	Change return type.
	* hir/rust-ast-lower.cc (ASTLoweringIfLetBlock::desugar_iflet):
	New.
	(ASTLoweringIfLetBlock::visit(AST::IfLetExpr &)): Adjust and use
	desugar_iflet.
	* hir/rust-ast-lower.h: Add comment.
	* hir/rust-hir-dump.cc (Dump::do_ifletexpr): Remove.
	(Dump::visit(IfLetExpr&)): Remove.
	(Dump::visit(IfLetExprConseqElse&)): Remove.
	* hir/rust-hir-dump.h (Dump::do_ifletexpr): Remove.
	(Dump::visit(IfLetExpr&)): Remove.
	(Dump::visit(IfLetExprConseqElse&)): Remove.
	* hir/tree/rust-hir-expr.h (class IfLetExpr): Remove.
	(class IfLetExprConseqElse): Remove.
	* hir/tree/rust-hir-full-decls.h (class IfLetExpr): Remove.
	(class IfLetExprConseqElse): Remove.
	* hir/tree/rust-hir-visitor.h: Adjust after removal of
	HIR::IfLetExpr and HIR::IfLetExprConseqElse.
	* hir/tree/rust-hir.cc (IfLetExpr::as_string): Remove.
	(IfLetExprConseqElse::as_string): Remove.
	(IfLetExpr::accept_vis): Remove.
	(IfLetExprConseqElse::accept_vis): Remove.
	* hir/tree/rust-hir.h: Adjust after removal of HIR::IfLetExpr and
	HIR::IfLetExprConseqElse.
	* typecheck/rust-hir-type-check-expr.cc (TypeCheckExpr::visit):
	Likewise.
	* typecheck/rust-hir-type-check-expr.h: Likewise.

gcc/testsuite/ChangeLog:

	* rust/compile/if_let_expr.rs: Adjust.
	* rust/compile/if_let_expr_simple.rs: New test.
	* rust/compile/iflet.rs: New test.
	* rust/execute/torture/iflet.rs: New test.

Signed-off-by: Marc Poulhiès <[email protected]>
@dkm dkm marked this pull request as ready for review September 5, 2024 20:08
@dkm
Copy link
Member Author

dkm commented Sep 5, 2024

@CohenArthur I think this is now ready for review (but of course, no hurry).

@dkm
Copy link
Member Author

dkm commented Sep 6, 2024

Looks like the alpine builder had a network issue: [7] Could not connect to server (Failed to connect to static.crates.io port 443 after 0 ms: Could not connect to server)

P-E-P
P-E-P approved these changes Sep 10, 2024
View reviewed changes
Copy link
Member

@P-E-P P-E-P left a comment

Choose a reason for hiding this comment

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

Looks great

gcc/rust/hir/rust-ast-lower.cc
*branch_value = ASTLoweringExpr::translate (expr.get_value_expr ());
kase_expr = ASTLoweringExpr::translate (expr.get_if_block ());

// FIXME: if let only accepts a single pattern. Why do we have a vector of
Copy link
Member

@P-E-P P-E-P Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Probably because of if let chains ? Given their implementation is not stable and they have been postponed multiple time we probably shouldn't care that much about them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add a comment in the AST class then, so that we don't have to ask again :)

@dkm
Copy link
Member Author

dkm commented Sep 10, 2024

Also, I'm not really sure how this impacts error reporting... What if we report an error after the HIR lowering. Do we already have a mechanism to make sure we're not reporting something confusing (e.g. a diagnostic mentioning a match error pointing to the "if let").

@P-E-P
Copy link
Member

P-E-P commented Sep 11, 2024

Also, I'm not really sure how this impacts error reporting... What if we report an error after the HIR lowering. Do we already have a mechanism to make sure we're not reporting something confusing (e.g. a diagnostic mentioning a match error pointing to the "if let").

Mmh good point, I think there is no mention of if let at this stage in the compile process, but the location should still point to the initial if let statement, if an error arise it should be correctly annotated anyway.

@dkm
Copy link
Member Author

dkm commented Sep 11, 2024

Also, I'm not really sure how this impacts error reporting... What if we report an error after the HIR lowering. Do we already have a mechanism to make sure we're not reporting something confusing (e.g. a diagnostic mentioning a match error pointing to the "if let").

Mmh good point, I think there is no mention of if let at this stage in the compile process, but the location should still point to the initial if let statement, if an error arise it should be correctly annotated anyway.

Sure, but I'm afraid we may produce nonsensical diagnostic at some point... I'll try to push the compiler to emit errors around this... But maybe it's not possible by construction... at least for the moment. But we probably need something to identify synthetic construction from the ones that come from the source.

@dkm
Copy link
Member Author

dkm commented Sep 12, 2024

Something like:

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

@CohenArthur CohenArthur CohenArthur left review comments

@P-E-P P-E-P P-E-P approved these changes

Assignees

@dkm dkm

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Compiler error when compiling the IfLet expression.
3 participants
@dkm @P-E-P @CohenArthur