Skip to content

Typecheck else expression of let-else to ensure it diverges #3469

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion gcc/rust/ast/rust-ast-builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
// <http://www.gnu.org/licenses/>.

#include "rust-ast-builder.h"
#include "optional.h"
#include "rust-ast-builder-type.h"
#include "rust-ast.h"
#include "rust-common.h"
Expand Down Expand Up @@ -352,7 +353,7 @@ Builder::let (std::unique_ptr<Pattern> &&pattern, std::unique_ptr<Type> &&type,
{
return std::unique_ptr<Stmt> (new LetStmt (std::move (pattern),
std::move (init), std::move (type),
{}, loc));
tl::nullopt, {}, loc));
}

std::unique_ptr<Expr>
Expand Down
8 changes: 8 additions & 0 deletions gcc/rust/ast/rust-ast-collector.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "rust-expr.h"
#include "rust-item.h"
#include "rust-keyword-values.h"
#include "rust-location.h"
#include "rust-path.h"
#include "rust-system.h"
#include "rust-token.h"
Expand Down Expand Up @@ -2587,6 +2588,13 @@ TokenCollector::visit (LetStmt &stmt)
push (Rust::Token::make (EQUAL, UNDEF_LOCATION));
visit (stmt.get_init_expr ());
}

if (stmt.has_else_expr ())
{
push (Rust::Token::make (ELSE, UNDEF_LOCATION));
visit (stmt.get_else_expr ());
}

push (Rust::Token::make (SEMICOLON, UNDEF_LOCATION));
}

Expand Down
29 changes: 28 additions & 1 deletion gcc/rust/ast/rust-stmt.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#ifndef RUST_AST_STATEMENT_H
#define RUST_AST_STATEMENT_H

#include "optional.h"
#include "rust-ast.h"
#include "rust-path.h"
#include "rust-expr.h"
Expand Down Expand Up @@ -72,6 +73,8 @@ class LetStmt : public Stmt
// bool has_init_expr;
std::unique_ptr<Expr> init_expr;

tl::optional<std::unique_ptr<Expr>> else_expr;

location_t locus;

public:
Expand All @@ -85,15 +88,18 @@ class LetStmt : public Stmt

// Returns whether let statement has an initialisation expression.
bool has_init_expr () const { return init_expr != nullptr; }
bool has_else_expr () const { return else_expr.has_value (); }

std::string as_string () const override;

LetStmt (std::unique_ptr<Pattern> variables_pattern,
std::unique_ptr<Expr> init_expr, std::unique_ptr<Type> type,
tl::optional<std::unique_ptr<Expr>> else_expr,
std::vector<Attribute> outer_attrs, location_t locus)
: outer_attrs (std::move (outer_attrs)),
variables_pattern (std::move (variables_pattern)),
type (std::move (type)), init_expr (std::move (init_expr)), locus (locus)
type (std::move (type)), init_expr (std::move (init_expr)),
else_expr (std::move (else_expr)), locus (locus)
{}

// Copy constructor with clone
Expand All @@ -107,6 +113,9 @@ class LetStmt : public Stmt
// guard to prevent null dereference (always required)
if (other.init_expr != nullptr)
init_expr = other.init_expr->clone_expr ();
if (other.else_expr.has_value ())
else_expr = other.else_expr.value ()->clone_expr ();

if (other.type != nullptr)
type = other.type->clone_type ();
}
Expand All @@ -128,6 +137,12 @@ class LetStmt : public Stmt
init_expr = other.init_expr->clone_expr ();
else
init_expr = nullptr;

if (other.else_expr != nullptr)
else_expr = other.else_expr.value ()->clone_expr ();
else
else_expr = tl::nullopt;

if (other.type != nullptr)
type = other.type->clone_type ();
else
Expand Down Expand Up @@ -162,12 +177,24 @@ class LetStmt : public Stmt
return *init_expr;
}

Expr &get_else_expr ()
{
rust_assert (has_else_expr ());
return *else_expr.value ();
}

std::unique_ptr<Expr> &get_init_expr_ptr ()
{
rust_assert (has_init_expr ());
return init_expr;
}

std::unique_ptr<Expr> &get_else_expr_ptr ()
{
rust_assert (has_else_expr ());
return else_expr.value ();
}

Pattern &get_pattern ()
{
rust_assert (variables_pattern != nullptr);
Expand Down
14 changes: 10 additions & 4 deletions gcc/rust/hir/rust-ast-lower-stmt.cc
Original file line number Diff line number Diff line change
Expand Up @@ -76,20 +76,26 @@ ASTLoweringStmt::visit (AST::LetStmt &stmt)
type
= std::unique_ptr<Type> (ASTLoweringType::translate (stmt.get_type ()));

tl::optional<std::unique_ptr<HIR::Expr>> init_expression = tl::nullopt;
tl::optional<std::unique_ptr<HIR::Expr>> init_expr = tl::nullopt;
tl::optional<std::unique_ptr<HIR::Expr>> else_expr = tl::nullopt;

if (stmt.has_init_expr ())
init_expression = std::unique_ptr<HIR::Expr> (
init_expr = std::unique_ptr<HIR::Expr> (
ASTLoweringExpr::translate (stmt.get_init_expr ()));

if (stmt.has_else_expr ())
else_expr = std::unique_ptr<HIR::Expr> (
ASTLoweringExpr::translate (stmt.get_else_expr ()));

auto crate_num = mappings.get_current_crate ();
Analysis::NodeMapping mapping (crate_num, stmt.get_node_id (),
mappings.get_next_hir_id (crate_num),
UNKNOWN_LOCAL_DEFID);
translated
= new HIR::LetStmt (mapping, std::unique_ptr<HIR::Pattern> (variables),
std::move (init_expression), std::move (type),
stmt.get_outer_attrs (), stmt.get_locus ());
std::move (init_expr), std::move (else_expr),
std::move (type), stmt.get_outer_attrs (),
stmt.get_locus ());
}

void
Expand Down
12 changes: 11 additions & 1 deletion gcc/rust/hir/tree/rust-hir-stmt.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,13 @@ namespace HIR {
LetStmt::LetStmt (Analysis::NodeMapping mappings,
std::unique_ptr<Pattern> variables_pattern,
tl::optional<std::unique_ptr<Expr>> init_expr,
tl::optional<std::unique_ptr<Expr>> else_expr,
tl::optional<std::unique_ptr<Type>> type,
AST::AttrVec outer_attrs, location_t locus)
: Stmt (std::move (mappings)), outer_attrs (std::move (outer_attrs)),
variables_pattern (std::move (variables_pattern)), type (std::move (type)),
init_expr (std::move (init_expr)), locus (locus)
init_expr (std::move (init_expr)), else_expr (std::move (else_expr)),
locus (locus)
{}

LetStmt::LetStmt (LetStmt const &other)
Expand All @@ -43,6 +45,8 @@ LetStmt::LetStmt (LetStmt const &other)
// guard to prevent null dereference (always required)
if (other.has_init_expr ())
init_expr = other.get_init_expr ().clone_expr ();
if (other.has_else_expr ())
else_expr = other.get_else_expr ().clone_expr ();

if (other.has_type ())
type = other.get_type ().clone_type ();
Expand All @@ -67,6 +71,12 @@ LetStmt::operator= (LetStmt const &other)
init_expr = other.get_init_expr ().clone_expr ();
else
init_expr = nullptr;

if (other.has_else_expr ())
else_expr = other.get_else_expr ().clone_expr ();
else
else_expr = tl::nullopt;

if (other.has_type ())
type = other.get_type ().clone_type ();
else
Expand Down
16 changes: 16 additions & 0 deletions gcc/rust/hir/tree/rust-hir-stmt.h
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ class LetStmt : public Stmt
tl::optional<std::unique_ptr<Type>> type;

tl::optional<std::unique_ptr<Expr>> init_expr;
tl::optional<std::unique_ptr<Expr>> else_expr;

location_t locus;

Expand All @@ -113,12 +114,15 @@ class LetStmt : public Stmt

// Returns whether let statement has an initialisation expression.
bool has_init_expr () const { return init_expr.has_value (); }
// Returns whether let statement has a diverging else expression.
bool has_else_expr () const { return else_expr.has_value (); }

std::string as_string () const override;

LetStmt (Analysis::NodeMapping mappings,
std::unique_ptr<Pattern> variables_pattern,
tl::optional<std::unique_ptr<Expr>> init_expr,
tl::optional<std::unique_ptr<Expr>> else_expr,
tl::optional<std::unique_ptr<Type>> type, AST::AttrVec outer_attrs,
location_t locus);

Expand Down Expand Up @@ -167,6 +171,18 @@ class LetStmt : public Stmt
return *init_expr.value ();
}

HIR::Expr &get_else_expr ()
{
rust_assert (*else_expr);
return *else_expr.value ();
}

const HIR::Expr &get_else_expr () const
{
rust_assert (*else_expr);
return *else_expr.value ();
}

HIR::Pattern &get_pattern () { return *variables_pattern; }

bool is_item () const override final { return false; }
Expand Down
6 changes: 5 additions & 1 deletion gcc/rust/parse/rust-parse-impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -6163,6 +6163,10 @@ Parser<ManagedTokenSource>::parse_let_stmt (AST::AttrVec outer_attrs,
}
}

tl::optional<std::unique_ptr<AST::Expr>> else_expr = tl::nullopt;
if (maybe_skip_token (ELSE))
else_expr = parse_block_expr ();

if (restrictions.consume_semi)
{
// `stmt` macro variables are parsed without a semicolon, but should be
Expand All @@ -6177,7 +6181,7 @@ Parser<ManagedTokenSource>::parse_let_stmt (AST::AttrVec outer_attrs,

return std::unique_ptr<AST::LetStmt> (
new AST::LetStmt (std::move (pattern), std::move (expr), std::move (type),
std::move (outer_attrs), locus));
std::move (else_expr), std::move (outer_attrs), locus));
}

// Parses a type path.
Expand Down
7 changes: 4 additions & 3 deletions gcc/rust/resolve/rust-ast-resolve-stmt.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,10 @@ class ResolveStmt : public ResolverBase
void visit (AST::LetStmt &stmt) override
{
if (stmt.has_init_expr ())
{
ResolveExpr::go (stmt.get_init_expr (), prefix, canonical_prefix);
}
ResolveExpr::go (stmt.get_init_expr (), prefix, canonical_prefix);

if (stmt.has_else_expr ())
ResolveExpr::go (stmt.get_else_expr (), prefix, canonical_prefix);

PatternDeclaration::go (stmt.get_pattern (), Rib::ItemType::Var);
if (stmt.has_type ())
Expand Down
3 changes: 3 additions & 0 deletions gcc/rust/resolve/rust-late-name-resolver-2.0.cc
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,9 @@ Late::visit (AST::LetStmt &let)
visit (let.get_init_expr ());
visit (let.get_pattern ());

if (let.has_else_expr ())
visit (let.get_init_expr ());

// how do we deal with the fact that `let a = blipbloup` should look for a
// label and cannot go through function ribs, but `let a = blipbloup()` can?

Expand Down
19 changes: 19 additions & 0 deletions gcc/rust/typecheck/rust-hir-type-check-stmt.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,14 @@
// <http://www.gnu.org/licenses/>.

#include "rust-hir-type-check-stmt.h"
#include "rich-location.h"
#include "rust-hir-type-check-type.h"
#include "rust-hir-type-check-expr.h"
#include "rust-hir-type-check-implitem.h"
#include "rust-hir-type-check-item.h"
#include "rust-hir-type-check-pattern.h"
#include "rust-type-util.h"
#include "rust-tyty.h"

namespace Rust {
namespace Resolver {
Expand Down Expand Up @@ -78,6 +80,7 @@ TypeCheckStmt::visit (HIR::LetStmt &stmt)
auto &stmt_pattern = stmt.get_pattern ();
TyTy::BaseType *init_expr_ty = nullptr;
location_t init_expr_locus = UNKNOWN_LOCATION;

if (stmt.has_init_expr ())
{
init_expr_locus = stmt.get_init_expr ().get_locus ();
Expand All @@ -97,6 +100,22 @@ TypeCheckStmt::visit (HIR::LetStmt &stmt)
specified_ty_locus = stmt.get_type ().get_locus ();
}

if (stmt.has_else_expr ())
{
auto &else_expr = stmt.get_else_expr ();
auto else_expr_ty = TypeCheckExpr::Resolve (else_expr);

if (else_expr_ty->get_kind () != TyTy::TypeKind::NEVER)
{
rust_error_at (else_expr.get_locus (),
"%<else%> clause of let-else does not diverge");
return;
}

// FIXME: Is that enough? Do we need to do something like
// `append_reference` here as well?
Comment on lines +115 to +116
Copy link
Member Author

Choose a reason for hiding this comment

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

@philberty I'm not sure if there's anything else we need to do here. I was trying to use something like coercion_site(...) but obviously it doesn't work because any type is coercible to !, which is not the behavior we actually want here

Copy link
Collaborator

Choose a reason for hiding this comment

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

! should be coercible to any other type, but not the other way around

}

// let x:i32 = 123;
if (specified_ty != nullptr && init_expr_ty != nullptr)
{
Expand Down
8 changes: 8 additions & 0 deletions gcc/testsuite/rust/compile/let-else-invalid-type.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
enum FakeOption {
Some(i32),
None,
}

fn main() {
let FakeOption::Some(a) = FakeOption::None else { FakeOption::Some(15) }; // { dg-error "does not diverge" }
}
Loading