Skip to content

Commit

Permalink
Cleanup parsed AST (#5464)
Browse files Browse the repository at this point in the history
## Description

This cleans up the parsed AST by doing the following:

[Remove unused
ReturnStatement.](c058e5f)

[Refactor Declaration::ImplicitReturnExpression as
Expression::ImplicitReturn](1276671)

During the work on debugging some issues around this refactoring, an
issue with the way that implicit/explicit return type-checking is
currently realized was found, which is reported in
#5518.

Currently this was "worked-around" by special casing the type-checking
of implicit returns, just like it is currently done, but we should look
into unifying this behaviour in a later PR (I've been looking into it,
but all hell breaks loose after an initial try).


## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [x] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [x] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.
  • Loading branch information
tritao authored Feb 2, 2024
1 parent 1fee545 commit 71e9ea9
Show file tree
Hide file tree
Showing 24 changed files with 245 additions and 223 deletions.
5 changes: 4 additions & 1 deletion sway-core/src/control_flow_analysis/analyze_return_paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,10 @@ fn connect_node<'eng: 'cfg, 'cfg>(
expression: ty::TyExpressionVariant::Return(..),
..
})
| ty::TyAstNodeContent::ImplicitReturnExpression(_) => {
| ty::TyAstNodeContent::Expression(ty::TyExpression {
expression: ty::TyExpressionVariant::ImplicitReturn(..),
..
}) => {
let this_index = graph.add_node(ControlFlowGraphNode::from_node(node));
for leaf_ix in leaves {
graph.add_edge(*leaf_ix, this_index, "".into());
Expand Down
61 changes: 18 additions & 43 deletions sway-core/src/control_flow_analysis/dead_code_analysis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -353,34 +353,6 @@ fn connect_node<'eng: 'cfg, 'cfg>(
// let mut graph = graph.clone();
let span = node.span.clone();
Ok(match &node.content {
ty::TyAstNodeContent::ImplicitReturnExpression(expr) => {
let this_index = graph.add_node(ControlFlowGraphNode::from_node_with_parent(
node,
options.parent_node,
));
for leaf_ix in leaves {
graph.add_edge(*leaf_ix, this_index, "".into());
}
// evaluate the expression

let return_contents = connect_expression(
engines,
&expr.expression,
graph,
&[this_index],
exit_node,
"",
tree_type,
expr.span.clone(),
options,
)?;

// connect return to the exit node
if let Some(exit_node) = exit_node {
graph.add_edge(this_index, exit_node, "return".into());
}
(return_contents, None)
}
ty::TyAstNodeContent::Expression(ty::TyExpression {
expression: expr_variant,
span,
Expand Down Expand Up @@ -408,7 +380,10 @@ fn connect_node<'eng: 'cfg, 'cfg>(
span.clone(),
options,
)?,
exit_node,
match expr_variant {
ty::TyExpressionVariant::ImplicitReturn(_) => None,
_ => exit_node,
},
)
}
ty::TyAstNodeContent::SideEffect(_) => (leaves.to_vec(), exit_node),
Expand Down Expand Up @@ -1832,7 +1807,7 @@ fn connect_expression<'eng: 'cfg, 'cfg>(
options,
)
}
Return(exp) => {
ImplicitReturn(exp) | Return(exp) => {
let this_index = graph.add_node("return entry".into());
for leaf in leaves {
graph.add_edge(*leaf, this_index, "".into());
Expand All @@ -1848,15 +1823,19 @@ fn connect_expression<'eng: 'cfg, 'cfg>(
exp.span.clone(),
options,
)?;
// TODO: is this right? Shouldn't we connect the return_contents leaves to the exit
// node?
for leaf in return_contents {
graph.add_edge(this_index, leaf, "".into());
}
if let Some(exit_node) = exit_node {
graph.add_edge(this_index, exit_node, "return".into());
if let Return(_) = expr_variant {
// TODO: is this right? Shouldn't we connect the return_contents leaves to the exit
// node?
for leaf in return_contents {
graph.add_edge(this_index, leaf, "".into());
}
if let Some(exit_node) = exit_node {
graph.add_edge(this_index, exit_node, "return".into());
}
Ok(vec![])
} else {
Ok(return_contents)
}
Ok(vec![])
}
Ref(exp) | Deref(exp) => connect_expression(
engines,
Expand Down Expand Up @@ -2130,10 +2109,7 @@ fn construct_dead_code_warning_from_node(
// Otherwise, this is unreachable.
ty::TyAstNode {
span,
content:
ty::TyAstNodeContent::ImplicitReturnExpression(_)
| ty::TyAstNodeContent::Expression(_)
| ty::TyAstNodeContent::SideEffect(_),
content: ty::TyAstNodeContent::Expression(_) | ty::TyAstNodeContent::SideEffect(_),
} => CompileWarning {
span: span.clone(),
warning_content: Warning::UnreachableCode,
Expand Down Expand Up @@ -2304,7 +2280,6 @@ fn allow_dead_code_ast_node(decl_engine: &DeclEngine, node: &ty::TyAstNode) -> b
ty::TyDecl::StorageDecl { .. } => false,
},
ty::TyAstNodeContent::Expression(_) => false,
ty::TyAstNodeContent::ImplicitReturnExpression(_) => false,
ty::TyAstNodeContent::SideEffect(_) => false,
ty::TyAstNodeContent::Error(_, _) => false,
}
Expand Down
50 changes: 32 additions & 18 deletions sway-core/src/ir_generation/const_eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -565,6 +565,17 @@ fn const_eval_typed_expr(
})
}
},
ty::TyExpressionVariant::ImplicitReturn(e) => {
if let Ok(Some(constant)) =
const_eval_typed_expr(lookup, known_consts, e, allow_configurables)
{
Some(constant)
} else {
return Err(ConstEvalError::CannotBeEvaluatedToConst {
span: e.span.clone(),
});
}
}
// we could allow non-local control flow in pure functions, but it would
// require some more work and at this point it's not clear if it is too useful
// for constant initializers -- the user can always refactor their pure functions
Expand Down Expand Up @@ -719,26 +730,29 @@ fn const_eval_codeblock(
}
}
ty::TyAstNodeContent::Declaration(_) => Ok(None),
ty::TyAstNodeContent::Expression(e) => {
if const_eval_typed_expr(lookup, known_consts, e, allow_configurables).is_err() {
Err(ConstEvalError::CannotBeEvaluatedToConst {
span: e.span.clone(),
})
} else {
Ok(None)
ty::TyAstNodeContent::Expression(e) => match e.expression {
ty::TyExpressionVariant::ImplicitReturn(_) => {
if let Ok(Some(constant)) =
const_eval_typed_expr(lookup, known_consts, e, allow_configurables)
{
Ok(Some(constant))
} else {
Err(ConstEvalError::CannotBeEvaluatedToConst {
span: e.span.clone(),
})
}
}
}
ty::TyAstNodeContent::ImplicitReturnExpression(e) => {
if let Ok(Some(constant)) =
const_eval_typed_expr(lookup, known_consts, e, allow_configurables)
{
Ok(Some(constant))
} else {
Err(ConstEvalError::CannotBeEvaluatedToConst {
span: e.span.clone(),
})
_ => {
if const_eval_typed_expr(lookup, known_consts, e, allow_configurables).is_err()
{
Err(ConstEvalError::CannotBeEvaluatedToConst {
span: e.span.clone(),
})
} else {
Ok(None)
}
}
}
},
ty::TyAstNodeContent::SideEffect(_) => Err(ConstEvalError::CannotBeEvaluatedToConst {
span: ast_node.span.clone(),
}),
Expand Down
28 changes: 18 additions & 10 deletions sway-core/src/ir_generation/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,18 +240,22 @@ impl<'eng> FnCompiler<'eng> {
ty::TyDecl::TraitTypeDecl { .. } => unexpected_decl("trait type"),
},
ty::TyAstNodeContent::Expression(te) => {
// An expression with an ignored return value... I assume.
let value = self.compile_expression_to_value(context, md_mgr, te)?;
// Terminating values should end the compilation of the block
if value.is_terminator {
Ok(Some(value))
} else {
Ok(None)
match &te.expression {
TyExpressionVariant::ImplicitReturn(exp) => self
.compile_expression_to_value(context, md_mgr, exp)
.map(Some),
_ => {
// An expression with an ignored return value... I assume.
let value = self.compile_expression_to_value(context, md_mgr, te)?;
// Terminating values should end the compilation of the block
if value.is_terminator {
Ok(Some(value))
} else {
Ok(None)
}
}
}
}
ty::TyAstNodeContent::ImplicitReturnExpression(te) => self
.compile_expression_to_value(context, md_mgr, te)
.map(Some),
// a side effect can be () because it just impacts the type system/namespacing.
// There should be no new IR generated.
ty::TyAstNodeContent::SideEffect(_) => Ok(None),
Expand Down Expand Up @@ -618,6 +622,10 @@ impl<'eng> FnCompiler<'eng> {
ty::TyExpressionVariant::Reassignment(reassignment) => {
self.compile_reassignment(context, md_mgr, reassignment, span_md_idx)
}
ty::TyExpressionVariant::ImplicitReturn(_exp) => {
// This is currently handled at the top-level handler, `compile_ast_node`.
unreachable!();
}
ty::TyExpressionVariant::Return(exp) => {
self.compile_return(context, md_mgr, exp, span_md_idx)
}
Expand Down
6 changes: 6 additions & 0 deletions sway-core/src/language/parsed/expression/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,12 @@ pub enum ExpressionKind {
Break,
Continue,
Reassignment(ReassignmentExpression),
/// An implicit return expression is different from a [Expression::Return] because
/// it is not a control flow item. Therefore it is a different variant.
///
/// An implicit return expression is an [Expression] at the end of a code block which has no
/// semicolon, denoting that it is the [Expression] to be returned from that block.
ImplicitReturn(Box<Expression>),
Return(Box<Expression>),
Ref(Box<Expression>),
Deref(Box<Expression>),
Expand Down
8 changes: 0 additions & 8 deletions sway-core/src/language/parsed/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ mod expression;
mod include_statement;
mod module;
mod program;
mod return_statement;
mod use_statement;

pub use code_block::*;
Expand All @@ -14,7 +13,6 @@ pub use expression::*;
pub use include_statement::IncludeStatement;
pub use module::{ParseModule, ParseSubmodule};
pub use program::{ParseProgram, TreeType};
pub use return_statement::*;
use sway_error::handler::ErrorEmitted;
pub use use_statement::{ImportType, UseStatement};

Expand Down Expand Up @@ -54,12 +52,6 @@ pub enum AstNodeContent {
Declaration(Declaration),
/// Any type of expression, of which there are quite a few. See [Expression] for more details.
Expression(Expression),
/// An implicit return expression is different from a [AstNodeContent::ReturnStatement] because
/// it is not a control flow item. Therefore it is a different variant.
///
/// An implicit return expression is an [Expression] at the end of a code block which has no
/// semicolon, denoting that it is the [Expression] to be returned from that block.
ImplicitReturnExpression(Expression),
/// A statement of the form `mod foo::bar;` which imports/includes another source file.
IncludeStatement(IncludeStatement),
/// A malformed statement.
Expand Down
6 changes: 0 additions & 6 deletions sway-core/src/language/parsed/return_statement.rs

This file was deleted.

Loading

0 comments on commit 71e9ea9

Please sign in to comment.