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.

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

Desugar IfLet* expr to match #3064

Merged
merged 1 commit into from
Oct 28, 2024
Merged

Desugar IfLet* expr to match #3064

merged 1 commit into from
Oct 28, 2024

Conversation

dkm
Copy link
Member

@dkm dkm commented Jun 23, 2024

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
@dkm dkm force-pushed the dkm/iflet_desugar branch 3 times, most recently from 54e06f0 to 19a5647 Compare July 2, 2024 21:06
@dkm dkm force-pushed the dkm/iflet_desugar branch 2 times, most recently from f8883da to 70b157e Compare September 5, 2024 20:06
@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)

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 Outdated Show resolved Hide resolved
@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 Oct 6, 2024

@CohenArthur I thought the FAIL that this PR has in nr2 test was caused by my change... but it seems it's caused by my new test case.

crab1: internal compiler error: in visit, at rust/resolve/rust-late-name-resolver-2.0.cc:207
0x259fde4 Rust::Resolver2_0::Late::visit(Rust::AST::PathInExpression&)
        ../../gcc/rust/resolve/rust-late-name-resolver-2.0.cc:207
0x237a889 Rust::AST::DefaultASTVisitor::visit(Rust::AST::TupleStructPattern&)                                                                                                 
        ../../gcc/rust/ast/rust-ast-visitor.cc:1231                                                                                                                           
0x2357099 Rust::AST::TupleStructPattern::accept_vis(Rust::AST::ASTVisitor&)       
        ../../gcc/rust/ast/rust-pattern.cc:487
0x237dc25 void Rust::AST::DefaultASTVisitor::visit<Rust::AST::Pattern>(std::unique_ptr<Rust::AST::Pattern, std::default_delete<Rust::AST::Pattern> >&)
        ../../gcc/rust/ast/rust-ast-visitor.h:409                          
0x2377cc0 Rust::AST::DefaultASTVisitor::visit(Rust::AST::IfLetExpr&)                                                                                                          
        ../../gcc/rust/ast/rust-ast-visitor.cc:623
0x2377d63 Rust::AST::DefaultASTVisitor::visit(Rust::AST::IfLetExprConseqElse&)
        ../../gcc/rust/ast/rust-ast-visitor.cc:631          
0x2277c51 Rust::AST::IfLetExprConseqElse::accept_vis(Rust::AST::ASTVisitor&)
        ../../gcc/rust/ast/rust-ast.cc:4645 
0x237c2d9 void Rust::AST::DefaultASTVisitor::visit<Rust::AST::Expr>(Rust::AST::Expr&)
        ../../gcc/rust/ast/rust-ast-visitor.h:405
0x23773bb Rust::AST::DefaultASTVisitor::visit(Rust::AST::BlockExpr&)
        ../../gcc/rust/ast/rust-ast-visitor.cc:461
0x25803a4 operator()

In :

void
Late::visit (AST::PathInExpression &expr)
{
  // TODO: How do we have a nice error with `can't capture dynamic environment
  // in a function item` error here?
  // do we emit it in `get<Namespace::Labels>`?

  rust_debug ("[ARTHUR]: %s", expr.as_simple_path ().as_string ().c_str ());
  auto value = ctx.values.resolve_path (expr.get_segments ());
  if (!value.has_value ())
    rust_unreachable (); // Should have been resolved earlier

Test case :

enum MyOption {
    Some(i32),
    None,
}

pub fn toto(i : MyOption) -> i32 {
    if let MyOption::Some(v) = i {
        v
    } else {
        23i32
    }
}

Seen in https://rust.godbolt.org/z/1dYfWGY5T

@dkm
Copy link
Member Author

dkm commented Oct 15, 2024

@P-E-P do you know if there's a way to mark a test XFAIL only for a given target (nr2)? I don't think I can debug this issue as I know nearly nothing about the name resolution in gccrs, but I think this change could go in as it's not adding any issue, it's simply testing something that's currently already broken in nr2.

@P-E-P
Copy link
Member

P-E-P commented Oct 15, 2024

@P-E-P do you know if there's a way to mark a test XFAIL only for a given target (nr2)? I don't think I can debug this issue as I know nearly nothing about the name resolution in gccrs, but I think this change could go in as it's not adding any issue, it's simply testing something that's currently already broken in nr2.

There is an exclude file containing a list of test that do not pass with nr2 here https://github.com/Rust-GCC/gccrs/blob/master/gcc%2Ftestsuite%2Frust%2Fcompile%2Fnr2%2Fexclude

You can add your failing test, we'll remove it from the list when nr2 will be more complete.

@dkm
Copy link
Member Author

dkm commented Oct 16, 2024

You can add your failing test, we'll remove it from the list when nr2 will be more complete.

oh nice, thanks!

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!

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.
	* checks/errors/rust-hir-pattern-analysis.cc
	(PatternChecker::visit (IfLetExpr &)): Remove.
	(PatternChecker::visit (IfLetExprConseqElse &)): Remove.
	* checks/errors/rust-hir-pattern-analysis.h (visit(IfLetExpr &)): Remove.
	(visit(IfLetExprConseqElse &)): Remove.

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.
	* rust/compile/nr2/exclude: Add iflet.rs and if_let_expr_simple.rs

Signed-off-by: Marc Poulhiès <[email protected]>
@dkm dkm added this pull request to the merge queue Oct 28, 2024
Merged via the queue into master with commit 5fe9110 Oct 28, 2024
12 checks passed
@dkm dkm deleted the dkm/iflet_desugar branch October 29, 2024 08:25
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.

Compiler error when compiling the IfLet expression.
3 participants