Skip to content

Commit

Permalink
Refactor TyDecl Enum and Separate Structs for Improved Code Organiz…
Browse files Browse the repository at this point in the history
…ation (FuelLabs#4457)

## Description
This PR refactors the `TyDecl` enum by separating its variants into
individual structs and simplifying the enum itself, leading to better
code organization and maintainability.

This refactoring enhances modularity making it easier to implement a
Parse trait on these types in the language server when matching on the
variants during AST traversal, which is required to unblock FuelLabs#3799. I had
attempted to do this in FuelLabs#4247 but it was reverted by @emilyaherbert.
Hopefully, this new approach is acceptable.

The main change is

```rust
#[derive(Clone, Debug)]
pub enum TyDecl {
    ConstantDecl {
        name: Ident,
        decl_id: DeclId<TyConstantDecl>,
        decl_span: Span,
    },
    FunctionDecl {
        name: Ident,
        decl_id: DeclId<TyFunctionDecl>,
        subst_list: Template<SubstList>,
        decl_span: Span,
    },
    ....
}
```

Has been changed to 

```rust
#[derive(Clone, Debug)]
pub enum TyDecl {
    ConstantDecl(ConstantDecl),
    FunctionDecl(FunctionDecl),
    ....
}

#[derive(Clone, Debug)]
pub struct ConstantDecl {
    pub name: Ident,
    pub decl_id: DeclId<TyConstantDecl>,
    pub decl_span: Span,
}

#[derive(Clone, Debug)]
pub struct FunctionDecl {
    pub name: Ident,
    pub decl_id: DeclId<TyFunctionDecl>,
    pub subst_list: Template<SubstList>,
    pub decl_span: Span,
}
```

## Checklist

- [x] I have linked to any relevant issues.
- [ ] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [ ] I have added tests that prove my fix is effective or that my
feature works.
- [ ] 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
JoshuaBatty authored Apr 18, 2023
1 parent 5e7ad6e commit 7011edd
Show file tree
Hide file tree
Showing 37 changed files with 594 additions and 502 deletions.
22 changes: 10 additions & 12 deletions forc-plugins/forc-doc/src/doc/descriptor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::{
use anyhow::Result;
use sway_core::{
decl_engine::*,
language::ty::{TyDecl, TyTraitFn, TyTraitInterfaceItem},
language::ty::{self, TyTraitFn, TyTraitInterfaceItem},
};

trait RequiredMethods {
Expand All @@ -33,16 +33,14 @@ impl Descriptor {
/// Decides whether a [TyDecl] is [Descriptor::Documentable].
pub(crate) fn from_typed_decl(
decl_engine: &DeclEngine,
ty_decl: &TyDecl,
ty_decl: &ty::TyDecl,
module_info: ModuleInfo,
document_private_items: bool,
) -> Result<Self> {
const CONTRACT_STORAGE: &str = "Contract Storage";

use swayfmt::parse;
use TyDecl::*;
const CONTRACT_STORAGE: &str = "Contract Storage";
match ty_decl {
StructDecl { decl_id, .. } => {
ty::TyDecl::StructDecl(ty::StructDecl { decl_id, .. }) => {
let struct_decl = decl_engine.get_struct(decl_id);
if !document_private_items && struct_decl.visibility.is_private() {
Ok(Descriptor::NonDocumentable)
Expand Down Expand Up @@ -78,7 +76,7 @@ impl Descriptor {
}))
}
}
EnumDecl { decl_id, .. } => {
ty::TyDecl::EnumDecl(ty::EnumDecl { decl_id, .. }) => {
let enum_decl = decl_engine.get_enum(decl_id);
if !document_private_items && enum_decl.visibility.is_private() {
Ok(Descriptor::NonDocumentable)
Expand Down Expand Up @@ -114,7 +112,7 @@ impl Descriptor {
}))
}
}
TraitDecl { decl_id, .. } => {
ty::TyDecl::TraitDecl(ty::TraitDecl { decl_id, .. }) => {
let trait_decl = decl_engine.get_trait(decl_id);
if !document_private_items && trait_decl.visibility.is_private() {
Ok(Descriptor::NonDocumentable)
Expand Down Expand Up @@ -161,7 +159,7 @@ impl Descriptor {
}))
}
}
AbiDecl { decl_id, .. } => {
ty::TyDecl::AbiDecl(ty::AbiDecl { decl_id, .. }) => {
let abi_decl = decl_engine.get_abi(decl_id);
let item_name = abi_decl.name;
let attrs_opt =
Expand Down Expand Up @@ -201,7 +199,7 @@ impl Descriptor {
raw_attributes: attrs_opt,
}))
}
StorageDecl { decl_id, .. } => {
ty::TyDecl::StorageDecl(ty::StorageDecl { decl_id, .. }) => {
let storage_decl = decl_engine.get_storage(decl_id);
let item_name = sway_types::BaseIdent::new_no_trim(
sway_types::span::Span::from_string(CONTRACT_STORAGE.to_string()),
Expand Down Expand Up @@ -263,7 +261,7 @@ impl Descriptor {
// raw_attributes: None,
// }))
// }
FunctionDecl { decl_id, .. } => {
ty::TyDecl::FunctionDecl(ty::FunctionDecl { decl_id, .. }) => {
let fn_decl = decl_engine.get_function(decl_id);
if !document_private_items && fn_decl.visibility.is_private() {
Ok(Descriptor::NonDocumentable)
Expand Down Expand Up @@ -293,7 +291,7 @@ impl Descriptor {
}))
}
}
ConstantDecl { decl_id, .. } => {
ty::TyDecl::ConstantDecl(ty::ConstantDecl { decl_id, .. }) => {
let const_decl = decl_engine.get_constant(decl_id);
if !document_private_items && const_decl.visibility.is_private() {
Ok(Descriptor::NonDocumentable)
Expand Down
25 changes: 12 additions & 13 deletions sway-core/src/control_flow_analysis/analyze_return_paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,25 +179,24 @@ fn connect_declaration<'eng: 'cfg, 'cfg>(
graph: &mut ControlFlowGraph<'cfg>,
leaves: &[NodeIndex],
) -> Result<Vec<NodeIndex>, CompileError> {
use ty::TyDecl::*;
let decl_engine = engines.de();
match decl {
TraitDecl { .. }
| AbiDecl { .. }
| StructDecl { .. }
| EnumDecl { .. }
| EnumVariantDecl { .. }
| StorageDecl { .. }
| TypeAliasDecl { .. }
| GenericTypeForFunctionScope { .. } => Ok(leaves.to_vec()),
VariableDecl(_) | ConstantDecl { .. } => {
ty::TyDecl::TraitDecl(_)
| ty::TyDecl::AbiDecl(_)
| ty::TyDecl::StructDecl(_)
| ty::TyDecl::EnumDecl(_)
| ty::TyDecl::EnumVariantDecl(_)
| ty::TyDecl::StorageDecl(_)
| ty::TyDecl::TypeAliasDecl(_)
| ty::TyDecl::GenericTypeForFunctionScope(_) => Ok(leaves.to_vec()),
ty::TyDecl::VariableDecl(_) | ty::TyDecl::ConstantDecl(_) => {
let entry_node = graph.add_node(ControlFlowGraphNode::from_node(node));
for leaf in leaves {
graph.add_edge(*leaf, entry_node, "".into());
}
Ok(vec![entry_node])
}
FunctionDecl { decl_id, .. } => {
ty::TyDecl::FunctionDecl(ty::FunctionDecl { decl_id, .. }) => {
let fn_decl = decl_engine.get_function(decl_id);
let entry_node = graph.add_node(ControlFlowGraphNode::from_node(node));
for leaf in leaves {
Expand All @@ -206,7 +205,7 @@ fn connect_declaration<'eng: 'cfg, 'cfg>(
connect_typed_fn_decl(engines, &fn_decl, graph, entry_node)?;
Ok(leaves.to_vec())
}
ImplTrait { decl_id, .. } => {
ty::TyDecl::ImplTrait(ty::ImplTrait { decl_id, .. }) => {
let ty::TyImplTrait {
trait_name, items, ..
} = decl_engine.get_impl_trait(decl_id);
Expand All @@ -218,7 +217,7 @@ fn connect_declaration<'eng: 'cfg, 'cfg>(
connect_impl_trait(engines, &trait_name, graph, &items, entry_node)?;
Ok(leaves.to_vec())
}
ErrorRecovery(_) => Ok(leaves.to_vec()),
ty::TyDecl::ErrorRecovery(_) => Ok(leaves.to_vec()),
}
}

Expand Down
81 changes: 49 additions & 32 deletions sway-core/src/control_flow_analysis/dead_code_analysis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -438,10 +438,9 @@ fn connect_declaration<'eng: 'cfg, 'cfg>(
leaves: &[NodeIndex],
options: NodeConnectionOptions,
) -> Result<Vec<NodeIndex>, CompileError> {
use ty::TyDecl::*;
let decl_engine = engines.de();
match decl {
VariableDecl(var_decl) => {
ty::TyDecl::VariableDecl(var_decl) => {
let ty::TyVariableDecl { body, name, .. } = &**var_decl;
let result = connect_expression(
engines,
Expand All @@ -467,7 +466,7 @@ fn connect_declaration<'eng: 'cfg, 'cfg>(
);
result
}
ConstantDecl { decl_id, .. } => {
ty::TyDecl::ConstantDecl(ty::ConstantDecl { decl_id, .. }) => {
let ty::TyConstantDecl {
call_path, value, ..
} = decl_engine.get_constant(decl_id);
Expand All @@ -490,39 +489,39 @@ fn connect_declaration<'eng: 'cfg, 'cfg>(
Ok(leaves.to_vec())
}
}
FunctionDecl { decl_id, .. } => {
ty::TyDecl::FunctionDecl(ty::FunctionDecl { decl_id, .. }) => {
let fn_decl = decl_engine.get_function(decl_id);
connect_typed_fn_decl(
engines, &fn_decl, graph, entry_node, span, exit_node, tree_type, options,
)?;
Ok(leaves.to_vec())
}
TraitDecl { decl_id, .. } => {
ty::TyDecl::TraitDecl(ty::TraitDecl { decl_id, .. }) => {
let trait_decl = decl_engine.get_trait(decl_id);
connect_trait_declaration(&trait_decl, graph, entry_node, tree_type);
Ok(leaves.to_vec())
}
AbiDecl { decl_id, .. } => {
ty::TyDecl::AbiDecl(ty::AbiDecl { decl_id, .. }) => {
let abi_decl = decl_engine.get_abi(decl_id);
connect_abi_declaration(engines, &abi_decl, graph, entry_node, tree_type)?;
Ok(leaves.to_vec())
}
StructDecl { decl_id, .. } => {
ty::TyDecl::StructDecl(ty::StructDecl { decl_id, .. }) => {
let struct_decl = decl_engine.get_struct(decl_id);
connect_struct_declaration(&struct_decl, *decl_id, graph, entry_node, tree_type);
Ok(leaves.to_vec())
}
EnumDecl { decl_id, .. } => {
ty::TyDecl::EnumDecl(ty::EnumDecl { decl_id, .. }) => {
let enum_decl = decl_engine.get_enum(decl_id);
connect_enum_declaration(&enum_decl, *decl_id, graph, entry_node);
Ok(leaves.to_vec())
}
EnumVariantDecl { enum_ref, .. } => {
ty::TyDecl::EnumVariantDecl(ty::EnumVariantDecl { enum_ref, .. }) => {
let enum_decl = decl_engine.get_enum(enum_ref.id());
connect_enum_declaration(&enum_decl, *enum_ref.id(), graph, entry_node);
Ok(leaves.to_vec())
}
ImplTrait { decl_id, .. } => {
ty::TyDecl::ImplTrait(ty::ImplTrait { decl_id, .. }) => {
let ty::TyImplTrait {
trait_name,
items,
Expand All @@ -542,16 +541,18 @@ fn connect_declaration<'eng: 'cfg, 'cfg>(
)?;
Ok(leaves.to_vec())
}
StorageDecl { decl_id, .. } => {
ty::TyDecl::StorageDecl(ty::StorageDecl { decl_id, .. }) => {
let storage = decl_engine.get_storage(decl_id);
connect_storage_declaration(&storage, graph, entry_node, tree_type);
Ok(leaves.to_vec())
}
TypeAliasDecl { .. } => {
ty::TyDecl::TypeAliasDecl(_) => {
// TODO - handle type aliases properly. For now, always skip DCA for them.
Ok(leaves.to_vec())
}
ErrorRecovery(_) | GenericTypeForFunctionScope { .. } => Ok(leaves.to_vec()),
ty::TyDecl::ErrorRecovery(_) | ty::TyDecl::GenericTypeForFunctionScope(_) => {
Ok(leaves.to_vec())
}
}
}

Expand Down Expand Up @@ -1023,25 +1024,25 @@ fn get_trait_fn_node_index<'a>(
let fn_decl = decl_engine.get_function(&function_decl_ref);
if let Some(implementing_type) = fn_decl.implementing_type {
match implementing_type {
ty::TyDecl::TraitDecl { decl_id, .. } => {
ty::TyDecl::TraitDecl(ty::TraitDecl { decl_id, .. }) => {
let trait_decl = decl_engine.get_trait(&decl_id);
Ok(graph
.namespace
.find_trait_method(&trait_decl.name.into(), &fn_decl.name))
}
ty::TyDecl::StructDecl { decl_id, .. } => {
ty::TyDecl::StructDecl(ty::StructDecl { decl_id, .. }) => {
let struct_decl = decl_engine.get_struct(&decl_id);
Ok(graph
.namespace
.find_trait_method(&struct_decl.call_path.suffix.into(), &fn_decl.name))
}
ty::TyDecl::ImplTrait { decl_id, .. } => {
ty::TyDecl::ImplTrait(ty::ImplTrait { decl_id, .. }) => {
let impl_trait = decl_engine.get_impl_trait(&decl_id);
Ok(graph
.namespace
.find_trait_method(&impl_trait.trait_name, &fn_decl.name))
}
ty::TyDecl::AbiDecl { decl_id, .. } => {
ty::TyDecl::AbiDecl(ty::AbiDecl { decl_id, .. }) => {
let abi_decl = decl_engine.get_abi(&decl_id);
Ok(graph
.namespace
Expand Down Expand Up @@ -1940,35 +1941,48 @@ fn construct_dead_code_warning_from_node(
// if this is a function, struct, or trait declaration that is never called, then it is dead
// code.
ty::TyAstNode {
content: ty::TyAstNodeContent::Declaration(ty::TyDecl::FunctionDecl { name, .. }),
content:
ty::TyAstNodeContent::Declaration(ty::TyDecl::FunctionDecl(ty::FunctionDecl {
name,
..
})),
..
} => CompileWarning {
span: name.span(),
warning_content: Warning::DeadFunctionDeclaration,
},
ty::TyAstNode {
content: ty::TyAstNodeContent::Declaration(ty::TyDecl::StructDecl { name, .. }),
content:
ty::TyAstNodeContent::Declaration(ty::TyDecl::StructDecl(ty::StructDecl {
name, ..
})),
..
} => CompileWarning {
span: name.span(),
warning_content: Warning::DeadStructDeclaration,
},
ty::TyAstNode {
content: ty::TyAstNodeContent::Declaration(ty::TyDecl::EnumDecl { name, .. }),
content:
ty::TyAstNodeContent::Declaration(ty::TyDecl::EnumDecl(ty::EnumDecl { name, .. })),
..
} => CompileWarning {
span: name.span(),
warning_content: Warning::DeadEnumDeclaration,
},
ty::TyAstNode {
content: ty::TyAstNodeContent::Declaration(ty::TyDecl::TraitDecl { name, .. }),
content:
ty::TyAstNodeContent::Declaration(ty::TyDecl::TraitDecl(ty::TraitDecl { name, .. })),
..
} => CompileWarning {
span: name.span(),
warning_content: Warning::DeadTrait,
},
ty::TyAstNode {
content: ty::TyAstNodeContent::Declaration(ty::TyDecl::ConstantDecl { name, .. }),
content:
ty::TyAstNodeContent::Declaration(ty::TyDecl::ConstantDecl(ty::ConstantDecl {
name,
..
})),
..
} => CompileWarning {
span: name.span(),
Expand All @@ -1994,7 +2008,10 @@ fn construct_dead_code_warning_from_node(
}
}
ty::TyAstNode {
content: ty::TyAstNodeContent::Declaration(ty::TyDecl::ImplTrait { decl_id, .. }),
content:
ty::TyAstNodeContent::Declaration(ty::TyDecl::ImplTrait(ty::ImplTrait {
decl_id, ..
})),
span,
} => {
let ty::TyImplTrait { .. } = decl_engine.get_impl_trait(decl_id);
Expand All @@ -2004,13 +2021,13 @@ fn construct_dead_code_warning_from_node(
}
}
ty::TyAstNode {
content: ty::TyAstNodeContent::Declaration(ty::TyDecl::AbiDecl { .. }),
content: ty::TyAstNodeContent::Declaration(ty::TyDecl::AbiDecl(_)),
..
} => return None,
// We handle storage fields individually. There is no need to emit any warnings for the
// storage declaration itself.
ty::TyAstNode {
content: ty::TyAstNodeContent::Declaration(ty::TyDecl::StorageDecl { .. }),
content: ty::TyAstNodeContent::Declaration(ty::TyDecl::StorageDecl(_)),
..
} => return None,
// If there is already an error for the declaration, we don't need to emit a dead code warning.
Expand Down Expand Up @@ -2078,26 +2095,26 @@ fn allow_dead_code_ast_node(decl_engine: &DeclEngine, node: &ty::TyAstNode) -> b
match &node.content {
ty::TyAstNodeContent::Declaration(decl) => match &decl {
ty::TyDecl::VariableDecl(_) => false,
ty::TyDecl::ConstantDecl { decl_id, .. } => {
ty::TyDecl::ConstantDecl(ty::ConstantDecl { decl_id, .. }) => {
allow_dead_code(decl_engine.get_constant(decl_id).attributes)
}
ty::TyDecl::FunctionDecl { decl_id, .. } => {
ty::TyDecl::FunctionDecl(ty::FunctionDecl { decl_id, .. }) => {
allow_dead_code(decl_engine.get_function(decl_id).attributes)
}
ty::TyDecl::TraitDecl { decl_id, .. } => {
ty::TyDecl::TraitDecl(ty::TraitDecl { decl_id, .. }) => {
allow_dead_code(decl_engine.get_trait(decl_id).attributes)
}
ty::TyDecl::StructDecl { decl_id, .. } => {
ty::TyDecl::StructDecl(ty::StructDecl { decl_id, .. }) => {
allow_dead_code(decl_engine.get_struct(decl_id).attributes)
}
ty::TyDecl::EnumDecl { decl_id, .. } => {
ty::TyDecl::EnumDecl(ty::EnumDecl { decl_id, .. }) => {
allow_dead_code(decl_engine.get_enum(decl_id).attributes)
}
ty::TyDecl::EnumVariantDecl {
ty::TyDecl::EnumVariantDecl(ty::EnumVariantDecl {
enum_ref,
variant_name,
..
} => decl_engine
}) => decl_engine
.get_enum(enum_ref.id())
.variants
.into_iter()
Expand Down
6 changes: 4 additions & 2 deletions sway-core/src/ir_generation/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,9 @@ pub(crate) fn compile_constants(
) -> Result<(), CompileError> {
let (type_engine, decl_engine) = engines.unwrap();
for decl_name in module_ns.get_all_declared_symbols() {
if let Some(ty::TyDecl::ConstantDecl { decl_id, .. }) = module_ns.symbols.get(decl_name) {
if let Some(ty::TyDecl::ConstantDecl(ty::ConstantDecl { decl_id, .. })) =
module_ns.symbols.get(decl_name)
{
let ty::TyConstantDecl { call_path, .. } = engines.de().get_constant(decl_id);
compile_const_decl(
&mut LookupEnv {
Expand Down Expand Up @@ -259,7 +261,7 @@ fn compile_declarations(
let (type_engine, decl_engine) = engines.unwrap();
for declaration in declarations {
match declaration {
ty::TyDecl::ConstantDecl { decl_id, .. } => {
ty::TyDecl::ConstantDecl(ty::ConstantDecl { decl_id, .. }) => {
let decl = decl_engine.get_constant(decl_id);
compile_const_decl(
&mut LookupEnv {
Expand Down
Loading

0 comments on commit 7011edd

Please sign in to comment.