Skip to content

Commit

Permalink
Introduce DeclRef and remove excessive clones to DeclId (FuelLabs…
Browse files Browse the repository at this point in the history 
…#4037)

## Description

This PR is a subset of FuelLabs#3744. It introduces `DeclRef`, which is a smart
wrapper type around `DeclId` and contains additional information aside
from the `DeclId`. This allows us to make `DeclId` a copy type and
remove excessive clones of `DeclId` and `DeclRef`.

This PR is a subset of FuelLabs#3744, so while not explicitly necessary right
now on `master`, there will be additional fields added to `DeclRef` in
FuelLabs#3744.

In detail, this PR:
1. Changes `DeclId` to a copy type defined as:
    ```rust
    #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Ord, PartialOrd)]
    pub struct DeclId(usize);
    ```
2. Creates a new `DeclRef` type which is a handle to a use of a
declaration: (there will be more fields added to this in FuelLabs#3744)
    ```rust
    /// A reference to the use of a declaration.
    #[derive(Debug, Clone)]
    struct DeclRef {
        /// The name of the declaration.
        name: Ident,
        /// The index into the [DeclEngine].
        id: DeclId,
        /// The [Span] of the entire declaration.
        decl_span: Span,
    }
    ```
3. Changes the definition of the `TyDeclaration::FunctionDeclaration`
variant to contain additional fields: (there will be more fields added
to this in FuelLabs#3744)
    ```rust
        FunctionDeclaration {
            name: Ident,
            decl_id: DeclId,
            decl_span: Span,
        },
    ```
4. Changes the definiton of the
`TyExpressionVariant::FunctionApplication` variant to contain a
`DeclRef` instead of just the previous `DeclId`. The
`TyExpressionVariant::FunctionApplication` variant gets a `DeclRef`
because `DeclRef` is a handle to a declaration _usage_, while the
`TyDeclaration::FunctionDeclaration` variant does not get a `DeclRef`
because it is simply an AST node for the function declaration. This
distinction will be more clear/necessary in FuelLabs#3744.
5. Changes the `DeclEngine` API to take `&T where ...`, allowing us to
remove more unnecessary instances of `.clone()`.

## Checklist

- [x] I have linked to any relevant issues.
- [ ] 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).
- [ ] 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.

---------

Co-authored-by: emilyaherbert <[email protected]>
  • Loading branch information
@emilyaherbert
emilyaherbert and emilyaherbert authored Feb 14, 2023
1 parent 0c1e14e commit 30cdb10
Show file tree
Hide file tree
Showing 62 changed files with 1,498 additions and 1,043 deletions.
14 changes: 7 additions & 7 deletions forc-pkg/src/pkg.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use std::{
use sway_core::{
abi_generation::{evm_json_abi, fuel_json_abi},
asm_generation::ProgramABI,
decl_engine::{DeclEngine, DeclId},
decl_engine::{DeclEngine, DeclRef},
fuel_prelude::{
fuel_crypto,
fuel_tx::{self, Contract, ContractId, StorageSlot},
Expand Down Expand Up @@ -2696,9 +2696,9 @@ impl PkgEntry {
finalized_entry: &FinalizedEntry,
decl_engine: &DeclEngine,
) -> Result<Self> {
let pkg_entry_kind = match &finalized_entry.test_decl_id {
Some(test_decl_id) => {
let pkg_test_entry = PkgTestEntry::from_decl(test_decl_id.clone(), decl_engine)?;
let pkg_entry_kind = match &finalized_entry.test_decl_ref {
Some(test_decl_ref) => {
let pkg_test_entry = PkgTestEntry::from_decl(test_decl_ref.clone(), decl_engine)?;
PkgEntryKind::Test(pkg_test_entry)
}
None => PkgEntryKind::Main,
Expand All @@ -2722,9 +2722,9 @@ impl PkgEntryKind {
}

impl PkgTestEntry {
fn from_decl(decl_id: DeclId, decl_engine: &DeclEngine) -> Result<Self> {
let span = decl_id.span();
let test_function_decl = decl_engine.get_function(decl_id, &span)?;
fn from_decl(decl_ref: DeclRef, decl_engine: &DeclEngine) -> Result<Self> {
let span = decl_ref.span();
let test_function_decl = decl_engine.get_function(&decl_ref, &span)?;

let test_args: HashSet<String> = test_function_decl
.attributes
Expand Down
53 changes: 33 additions & 20 deletions forc-plugins/forc-doc/src/descriptor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,16 @@ use sway_core::{
decl_engine::*,
language::ty::{TyDeclaration, TyTraitFn},
};
use sway_types::Spanned;

trait RequiredMethods {
fn to_methods(&self, decl_engine: &DeclEngine) -> Result<Vec<TyTraitFn>>;
}
impl RequiredMethods for Vec<sway_core::decl_engine::DeclId> {
impl RequiredMethods for Vec<sway_core::decl_engine::DeclRef> {
fn to_methods(&self, decl_engine: &DeclEngine) -> Result<Vec<TyTraitFn>> {
self.iter()
.map(|decl_id| {
.map(|DeclRef { id, decl_span, .. }| {
decl_engine
.get_trait_fn(decl_id.clone(), &decl_id.span())
.get_trait_fn(id, decl_span)
.map_err(|e| anyhow::anyhow!("{}", e))
})
.collect::<anyhow::Result<_>>()
Expand All @@ -44,8 +43,10 @@ impl Descriptor {
use swayfmt::parse;
use TyDeclaration::*;
match ty_decl {
StructDeclaration(ref decl_id) => {
let struct_decl = decl_engine.get_struct(decl_id.clone(), &decl_id.span())?;
StructDeclaration {
decl_id, decl_span, ..
} => {
let struct_decl = decl_engine.get_struct(decl_id, decl_span)?;
if !document_private_items && struct_decl.visibility.is_private() {
Ok(Descriptor::NonDocumentable)
} else {
Expand Down Expand Up @@ -76,8 +77,10 @@ impl Descriptor {
}))
}
}
EnumDeclaration(ref decl_id) => {
let enum_decl = decl_engine.get_enum(decl_id.clone(), &decl_id.span())?;
EnumDeclaration {
decl_id, decl_span, ..
} => {
let enum_decl = decl_engine.get_enum(decl_id, decl_span)?;
if !document_private_items && enum_decl.visibility.is_private() {
Ok(Descriptor::NonDocumentable)
} else {
Expand Down Expand Up @@ -108,8 +111,10 @@ impl Descriptor {
}))
}
}
TraitDeclaration(ref decl_id) => {
let trait_decl = decl_engine.get_trait(decl_id.clone(), &decl_id.span())?;
TraitDeclaration {
decl_id, decl_span, ..
} => {
let trait_decl = decl_engine.get_trait(decl_id, decl_span)?;
if !document_private_items && trait_decl.visibility.is_private() {
Ok(Descriptor::NonDocumentable)
} else {
Expand Down Expand Up @@ -143,8 +148,10 @@ impl Descriptor {
}))
}
}
AbiDeclaration(ref decl_id) => {
let abi_decl = decl_engine.get_abi(decl_id.clone(), &decl_id.span())?;
AbiDeclaration {
decl_id, decl_span, ..
} => {
let abi_decl = decl_engine.get_abi(decl_id, decl_span)?;
let item_name = abi_decl.name;
let attrs_opt =
(!abi_decl.attributes.is_empty()).then(|| abi_decl.attributes.to_html_string());
Expand Down Expand Up @@ -172,8 +179,10 @@ impl Descriptor {
raw_attributes: attrs_opt,
}))
}
StorageDeclaration(ref decl_id) => {
let storage_decl = decl_engine.get_storage(decl_id.clone(), &decl_id.span())?;
StorageDeclaration {
decl_id, decl_span, ..
} => {
let storage_decl = decl_engine.get_storage(decl_id, decl_span)?;
let item_name = sway_types::BaseIdent::new_no_trim(
sway_types::span::Span::from_string(CONTRACT_STORAGE.to_string()),
);
Expand Down Expand Up @@ -203,12 +212,12 @@ impl Descriptor {
}))
}
// Uncomment this when we decide how to handle ImplTraits
// ImplTrait(ref decl_id) => {
// ImplTrait { decl_id, decl_span, .. } => {
// TODO: figure out how to use this, likely we don't want to document this directly.
//
// This declaration type may make more sense to document as part of another declaration
// much like how we document method functions for traits or fields on structs.
// let impl_trait = decl_engine.get_impl_trait(decl_id.clone(), &decl_id.span())?;
// let impl_trait = decl_engine.get_impl_trait(&decl_ref, decl_span)?;
// let item_name = impl_trait.trait_name.suffix;
// Ok(Descriptor::Documentable(Document {
// module_info: module_info.clone(),
Expand All @@ -230,8 +239,10 @@ impl Descriptor {
// raw_attributes: None,
// }))
// }
FunctionDeclaration(ref decl_id) => {
let fn_decl = decl_engine.get_function(decl_id.clone(), &decl_id.span())?;
FunctionDeclaration {
decl_id, decl_span, ..
} => {
let fn_decl = decl_engine.get_function(decl_id, decl_span)?;
if !document_private_items && fn_decl.visibility.is_private() {
Ok(Descriptor::NonDocumentable)
} else {
Expand Down Expand Up @@ -260,8 +271,10 @@ impl Descriptor {
}))
}
}
ConstantDeclaration(ref decl_id) => {
let const_decl = decl_engine.get_constant(decl_id.clone(), &decl_id.span())?;
ConstantDeclaration {
decl_id, decl_span, ..
} => {
let const_decl = decl_engine.get_constant(decl_id, decl_span)?;
if !document_private_items && const_decl.visibility.is_private() {
Ok(Descriptor::NonDocumentable)
} else {
Expand Down
2 changes: 1 addition & 1 deletion forc-plugins/forc-doc/src/doc.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ impl Document {
pub(crate) fn html_filename(&self) -> String {
use sway_core::language::ty::TyDeclaration::StorageDeclaration;
let name = match &self.item_body.ty_decl {
StorageDeclaration(_) => None,
StorageDeclaration { .. } => None,
_ => Some(self.item_header.item_name.as_str()),
};

Expand Down
111 changes: 60 additions & 51 deletions forc-plugins/forc-doc/src/render.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,75 +52,79 @@ impl RenderedDocumentation {
match module_map.get_mut(&location) {
Some(doc_links) => {
match doc.item_body.ty_decl {
StructDeclaration(_) => match doc_links.get_mut(&BlockTitle::Structs) {
StructDeclaration { .. } => match doc_links.get_mut(&BlockTitle::Structs) {
Some(links) => links.push(doc.link()),
None => {
doc_links.insert(BlockTitle::Structs, vec![doc.link()]);
}
},
EnumDeclaration(_) => match doc_links.get_mut(&BlockTitle::Enums) {
EnumDeclaration { .. } => match doc_links.get_mut(&BlockTitle::Enums) {
Some(links) => links.push(doc.link()),
None => {
doc_links.insert(BlockTitle::Enums, vec![doc.link()]);
}
},
TraitDeclaration(_) => match doc_links.get_mut(&BlockTitle::Traits) {
TraitDeclaration { .. } => match doc_links.get_mut(&BlockTitle::Traits) {
Some(links) => links.push(doc.link()),
None => {
doc_links.insert(BlockTitle::Traits, vec![doc.link()]);
}
},
AbiDeclaration(_) => match doc_links.get_mut(&BlockTitle::Abi) {
AbiDeclaration { .. } => match doc_links.get_mut(&BlockTitle::Abi) {
Some(links) => links.push(doc.link()),
None => {
doc_links.insert(BlockTitle::Abi, vec![doc.link()]);
}
},
StorageDeclaration(_) => {
StorageDeclaration { .. } => {
match doc_links.get_mut(&BlockTitle::ContractStorage) {
Some(links) => links.push(doc.link()),
None => {
doc_links.insert(BlockTitle::ContractStorage, vec![doc.link()]);
}
}
}
FunctionDeclaration(_) => match doc_links.get_mut(&BlockTitle::Functions) {
Some(links) => links.push(doc.link()),
None => {
doc_links.insert(BlockTitle::Functions, vec![doc.link()]);
FunctionDeclaration { .. } => {
match doc_links.get_mut(&BlockTitle::Functions) {
Some(links) => links.push(doc.link()),
None => {
doc_links.insert(BlockTitle::Functions, vec![doc.link()]);
}
}
},
ConstantDeclaration(_) => match doc_links.get_mut(&BlockTitle::Constants) {
Some(links) => links.push(doc.link()),
None => {
doc_links.insert(BlockTitle::Constants, vec![doc.link()]);
}
ConstantDeclaration { .. } => {
match doc_links.get_mut(&BlockTitle::Constants) {
Some(links) => links.push(doc.link()),
None => {
doc_links.insert(BlockTitle::Constants, vec![doc.link()]);
}
}
},
}
_ => {} // TODO: ImplTraitDeclaration
}
}
None => {
let mut doc_links: BTreeMap<BlockTitle, Vec<DocLink>> = BTreeMap::new();
match doc.item_body.ty_decl {
StructDeclaration(_) => {
StructDeclaration { .. } => {
doc_links.insert(BlockTitle::Structs, vec![doc.link()]);
}
EnumDeclaration(_) => {
EnumDeclaration { .. } => {
doc_links.insert(BlockTitle::Enums, vec![doc.link()]);
}
TraitDeclaration(_) => {
TraitDeclaration { .. } => {
doc_links.insert(BlockTitle::Traits, vec![doc.link()]);
}
AbiDeclaration(_) => {
AbiDeclaration { .. } => {
doc_links.insert(BlockTitle::Abi, vec![doc.link()]);
}
StorageDeclaration(_) => {
StorageDeclaration { .. } => {
doc_links.insert(BlockTitle::ContractStorage, vec![doc.link()]);
}
FunctionDeclaration(_) => {
FunctionDeclaration { .. } => {
doc_links.insert(BlockTitle::Functions, vec![doc.link()]);
}
ConstantDeclaration(_) => {
ConstantDeclaration { .. } => {
doc_links.insert(BlockTitle::Constants, vec![doc.link()]);
}
_ => {} // TODO: ImplTraitDeclaration
Expand Down Expand Up @@ -156,55 +160,60 @@ impl RenderedDocumentation {
}
// Above we check for the module a link belongs to, here we want _all_ links so the check is much more shallow.
match doc.item_body.ty_decl {
StructDeclaration(_) => match all_docs.links.get_mut(&BlockTitle::Structs) {
StructDeclaration { .. } => match all_docs.links.get_mut(&BlockTitle::Structs) {
Some(links) => links.push(doc.link()),
None => {
all_docs.links.insert(BlockTitle::Structs, vec![doc.link()]);
}
},
EnumDeclaration(_) => match all_docs.links.get_mut(&BlockTitle::Enums) {
EnumDeclaration { .. } => match all_docs.links.get_mut(&BlockTitle::Enums) {
Some(links) => links.push(doc.link()),
None => {
all_docs.links.insert(BlockTitle::Enums, vec![doc.link()]);
}
},
TraitDeclaration(_) => match all_docs.links.get_mut(&BlockTitle::Traits) {
TraitDeclaration { .. } => match all_docs.links.get_mut(&BlockTitle::Traits) {
Some(links) => links.push(doc.link()),
None => {
all_docs.links.insert(BlockTitle::Traits, vec![doc.link()]);
}
},
AbiDeclaration(_) => match all_docs.links.get_mut(&BlockTitle::Abi) {
AbiDeclaration { .. } => match all_docs.links.get_mut(&BlockTitle::Abi) {
Some(links) => links.push(doc.link()),
None => {
all_docs.links.insert(BlockTitle::Abi, vec![doc.link()]);
}
},
StorageDeclaration(_) => match all_docs.links.get_mut(&BlockTitle::ContractStorage)
{
Some(links) => links.push(doc.link()),
None => {
all_docs
.links
.insert(BlockTitle::ContractStorage, vec![doc.link()]);
StorageDeclaration { .. } => {
match all_docs.links.get_mut(&BlockTitle::ContractStorage) {
Some(links) => links.push(doc.link()),
None => {
all_docs
.links
.insert(BlockTitle::ContractStorage, vec![doc.link()]);
}
}
},
FunctionDeclaration(_) => match all_docs.links.get_mut(&BlockTitle::Functions) {
Some(links) => links.push(doc.link()),
None => {
all_docs
.links
.insert(BlockTitle::Functions, vec![doc.link()]);
}
FunctionDeclaration { .. } => {
match all_docs.links.get_mut(&BlockTitle::Functions) {
Some(links) => links.push(doc.link()),
None => {
all_docs
.links
.insert(BlockTitle::Functions, vec![doc.link()]);
}
}
},
ConstantDeclaration(_) => match all_docs.links.get_mut(&BlockTitle::Constants) {
Some(links) => links.push(doc.link()),
None => {
all_docs
.links
.insert(BlockTitle::Constants, vec![doc.link()]);
}
ConstantDeclaration { .. } => {
match all_docs.links.get_mut(&BlockTitle::Constants) {
Some(links) => links.push(doc.link()),
None => {
all_docs
.links
.insert(BlockTitle::Constants, vec![doc.link()]);
}
}
},
}
_ => {} // TODO: ImplTraitDeclaration
}
}
Expand Down Expand Up @@ -661,13 +670,13 @@ impl Renderable for TyTraitFn {
write!(fn_sig, ") -> {}", self.return_type_span.as_str())?;
let multiline = fn_sig.chars().count() >= 60;

let method_id = format!("tymethod.{}", self.name.as_str());
let method_ref = format!("tymethod.{}", self.name.as_str());
Ok(box_html! {
div(class="methods") {
div(id=&method_id, class="method has-srclink") {
div(id=&method_ref, class="method has-srclink") {
h4(class="code-header") {
: "fn ";
a(class="fnname", href=format!("{IDENTITY}{method_id}")) {
a(class="fnname", href=format!("{IDENTITY}{method_ref}")) {
: self.name.as_str();
}
: "(";
Expand Down
4 changes: 2 additions & 2 deletions sway-core/src/asm_generation/finalized_asm.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use super::{
ProgramABI, ProgramKind,
};
use crate::asm_lang::allocated_ops::{AllocatedOp, AllocatedOpcode};
use crate::decl_engine::DeclId;
use crate::decl_engine::DeclRef;
use crate::error::*;
use crate::source_map::SourceMap;

Expand Down Expand Up @@ -36,7 +36,7 @@ pub struct FinalizedEntry {
pub selector: Option<[u8; 4]>,
/// If this entry is constructed from a test function contains the declaration id for that
/// function, otherwise contains `None`.
pub test_decl_id: Option<DeclId>,
pub test_decl_ref: Option<DeclRef>,
}

/// The bytecode for a sway program as well as the byte offsets of configuration-time constants in
Expand Down
Loading

0 comments on commit 30cdb10

Please sign in to comment.