From c85392a7bb5c0946901f9941c71fcb1699d46180 Mon Sep 17 00:00:00 2001
From: Nikita Strygin <dcnick3@users.noreply.github.com>
Date: Fri, 1 Sep 2023 14:13:38 +0300
Subject: [PATCH] [refactor] #2573: clean up comments

Signed-off-by: Nikita Strygin <dcnick3@users.noreply.github.com>
---
 ffi/derive/src/attr_parse/derive.rs |  6 ++++--
 ffi/derive/src/convert.rs           | 18 +++++-------------
 ffi/derive/src/impl_visitor.rs      |  1 -
 ffi/derive/src/lib.rs               |  3 ++-
 4 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/ffi/derive/src/attr_parse/derive.rs b/ffi/derive/src/attr_parse/derive.rs
index 4b2203f5647..f33a62f592b 100644
--- a/ffi/derive/src/attr_parse/derive.rs
+++ b/ffi/derive/src/attr_parse/derive.rs
@@ -90,8 +90,10 @@ impl FromAttributes for DeriveAttrs {
                         Derive::Other(path.to_token_stream().to_string())
                     };
 
-                    // I __think__ it's an error to use the same derive twice
-                    // I don't think we care in this case though
+                    // Funnily, rust allows the usage of the same derive multiple times
+                    // In most cases this will lead to a "Conflicting implementations of trait" errors,
+                    //      but technically it's not an error by itself
+                    // We do handle the duplicate derives just fine
                     derives.push(derive);
                 }
             }
diff --git a/ffi/derive/src/convert.rs b/ffi/derive/src/convert.rs
index 5e5bec85918..cb409c622af 100644
--- a/ffi/derive/src/convert.rs
+++ b/ffi/derive/src/convert.rs
@@ -87,10 +87,9 @@ impl syn2::parse::Parse for SpannedFfiTypeToken {
     }
 }
 
+/// This represents an `#[ffi_type(...)]` attribute on a type
 #[derive(Debug, PartialEq, Eq, Copy, Clone)]
 pub enum FfiTypeKindAttribute {
-    // NOTE: these are not all the possible ffi type kinds, but only those that can be referred to using an attribute
-    // Enums are treated quite funnily...
     Opaque,
     UnsafeRobust,
     Local,
@@ -114,6 +113,7 @@ impl syn2::parse::Parse for FfiTypeKindAttribute {
     }
 }
 
+/// This represents an `#[ffi_type(...)]` attribute on a field
 #[derive(Debug, PartialEq, Eq, Copy, Clone)]
 pub enum FfiTypeKindFieldAttribute {
     UnsafeNonOwning,
@@ -211,6 +211,7 @@ pub struct FfiTypeInput {
     pub ffi_type_attr: FfiTypeAttr,
     pub getset_attr: GetSetStructAttrs,
     pub span: Span,
+    /// The original DeriveInput this structure was parsed from
     pub ast: syn2::DeriveInput,
 }
 
@@ -250,14 +251,6 @@ impl darling::FromDeriveInput for FfiTypeInput {
     }
 }
 
-impl quote::ToTokens for FfiTypeInput {
-    fn to_tokens(&self, tokens: &mut TokenStream) {
-        // TODO: this ignores changes that may be done to Generics
-        // Would this be a problem?
-        self.ast.to_tokens(tokens);
-    }
-}
-
 #[derive(FromVariant)]
 pub struct FfiTypeVariant {
     pub ident: syn2::Ident,
@@ -301,7 +294,7 @@ pub fn derive_ffi_type(emitter: &mut Emitter, input: &syn2::DeriveInput) -> Toke
         if variants.is_empty() {
             emit!(
                 emitter,
-                input.span(),
+                input.span,
                 "Uninhabited enums are not allowed in FFI"
             );
         }
@@ -309,7 +302,6 @@ pub fn derive_ffi_type(emitter: &mut Emitter, input: &syn2::DeriveInput) -> Toke
 
     // the logic of `is_opaque` is somewhat convoluted and I am not sure if it is even correct
     // there is also `is_opaque_struct`...
-    // NOTE: this mirrors the logic in `is_opaque`. The function should be updated to use darling-parsed attrs together with the call sites
     if input.is_opaque() {
         return derive_ffi_type_for_opaque_item(name, &input.generics);
     }
@@ -717,7 +709,7 @@ fn derive_ffi_type_for_repr_c(emitter: &mut Emitter, input: &FfiTypeInput) -> To
             .kind
             .map_or_else(Span::call_site, |kind| kind.span());
         // TODO: this error message may be unclear. Consider adding a note about the `#[ffi_type]` attribute
-        emit!(emitter, span, "The type should be marked with #[repr(C)]");
+        emit!(emitter, span, "To make an FFI type robust you must mark it with `#[repr(C)]`. Alternatively, try using `#[ffi_type(opaque)]` to make it opaque");
     }
 
     let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl();
diff --git a/ffi/derive/src/impl_visitor.rs b/ffi/derive/src/impl_visitor.rs
index 318355464a8..6c547b10020 100644
--- a/ffi/derive/src/impl_visitor.rs
+++ b/ffi/derive/src/impl_visitor.rs
@@ -306,7 +306,6 @@ impl<'ast> Visit<'ast> for ImplVisitor<'ast, '_> {
         }
         if node.defaultness.is_some() {
             emit!(self.emitter, node.defaultness, "Default impl not supported");
-            // TODO: should we return here, or can the execution continue?
         }
 
         for it in &node.attrs {
diff --git a/ffi/derive/src/lib.rs b/ffi/derive/src/lib.rs
index 9e28dd13f40..3f018429b26 100644
--- a/ffi/derive/src/lib.rs
+++ b/ffi/derive/src/lib.rs
@@ -74,10 +74,11 @@ pub fn ffi(input: TokenStream) -> TokenStream {
         .into_iter()
         .map(|item| {
             if !matches!(item.vis, syn2::Visibility::Public(_)) {
-                emit!(emitter, item, "Only public types are allowed in FFI");
+                emit!(emitter, item.span, "Only public types are allowed in FFI");
             }
 
             if !item.is_opaque() {
+                let item = item.ast;
                 return quote! {
                     #[derive(iroha_ffi::FfiType)]
                     #item