Merge branch 'master' into ab/formatter

.github/workflows/docs-pr.yml

@@ -80,7 +80,7 @@ jobs:
 
       - name: Build docs
         env:
-          MATOMO_ENV: staging # not really a secret, it will show in the footer anyway
+          ENV: staging # not really a secret, it will show in the footer anyway
         run: yarn workspaces foreach -Rpt --from docs run build
 
       - name: Upload artifact

acvm-repo/acvm/src/compiler/mod.rs

@@ -11,8 +11,8 @@ mod transformers;
 
 pub use optimizers::optimize;
 use optimizers::optimize_internal;
-pub use transformers::transform;
 use transformers::transform_internal;
+pub use transformers::{transform, MIN_EXPRESSION_WIDTH};
 
 /// This module moves and decomposes acir opcodes. The transformation map allows consumers of this module to map
 /// metadata they had about the opcodes to the new opcode structure generated after the transformation.

acvm-repo/acvm/src/compiler/transformers/csat.rs

@@ -6,6 +6,9 @@ use acir::{
 };
 use indexmap::IndexMap;
 
+/// Minimum width accepted by the `CSatTransformer`.
+pub const MIN_EXPRESSION_WIDTH: usize = 3;
+
 /// A transformer which processes any [`Expression`]s to break them up such that they
 /// fit within the [`ProofSystemCompiler`][crate::ProofSystemCompiler]'s width.
 ///
@@ -22,9 +25,11 @@ pub(crate) struct CSatTransformer {
 }
 
 impl CSatTransformer {
-    // Configure the width for the optimizer
+    /// Create an optimizer with a given width.
+    ///
+    /// Panics if `width` is less than `MIN_EXPRESSION_WIDTH`.
     pub(crate) fn new(width: usize) -> CSatTransformer {
-        assert!(width > 2);
+        assert!(width >= MIN_EXPRESSION_WIDTH, "width has to be at least {MIN_EXPRESSION_WIDTH}");
 
         CSatTransformer { width, solvable_witness: HashSet::new() }
     }

acvm-repo/acvm/src/compiler/transformers/mod.rs

@@ -8,6 +8,7 @@ use indexmap::IndexMap;
 mod csat;
 
 pub(crate) use csat::CSatTransformer;
+pub use csat::MIN_EXPRESSION_WIDTH;
 
 use super::{transform_assert_messages, AcirTransformationMap};
 

compiler/noirc_driver/src/lib.rs

@@ -5,6 +5,7 @@
 
 use abi_gen::{abi_type_from_hir_type, value_from_hir_expression};
 use acvm::acir::circuit::ExpressionWidth;
+use acvm::compiler::MIN_EXPRESSION_WIDTH;
 use clap::Args;
 use fm::{FileId, FileManager};
 use iter_extended::vecmap;
@@ -134,7 +135,11 @@ pub fn parse_expression_width(input: &str) -> Result<ExpressionWidth, std::io::E
 
     match width {
         0 => Ok(ExpressionWidth::Unbounded),
-        _ => Ok(ExpressionWidth::Bounded { width }),
+        w if w >= MIN_EXPRESSION_WIDTH => Ok(ExpressionWidth::Bounded { width }),
+        _ => Err(Error::new(
+            ErrorKind::InvalidInput,
+            format!("has to be 0 or at least {MIN_EXPRESSION_WIDTH}"),
+        )),
     }
 }
 

compiler/noirc_frontend/src/elaborator/mod.rs

@@ -3,7 +3,9 @@ use std::{
     rc::Rc,
 };
 
-use crate::{ast::ItemVisibility, hir_def::traits::ResolvedTraitBound, StructField, TypeBindings};
+use crate::{
+    ast::ItemVisibility, hir_def::traits::ResolvedTraitBound, StructField, StructType, TypeBindings,
+};
 use crate::{
     ast::{
         BlockExpression, FunctionKind, GenericTypeArgs, Ident, NoirFunction, NoirStruct, Param,
@@ -53,7 +55,7 @@ mod unquote;
 
 use fm::FileId;
 use iter_extended::vecmap;
-use noirc_errors::{Location, Span};
+use noirc_errors::{Location, Span, Spanned};
 use types::bind_ordered_generics;
 
 use self::traits::check_trait_impl_method_matches_declaration;
@@ -398,13 +400,36 @@ impl<'context> Elaborator<'context> {
 
         self.run_function_lints(&func_meta, &modifiers);
 
+        // Check arg and return-value visibility of standalone functions.
+        if self.should_check_function_visibility(&func_meta, &modifiers) {
+            let name = Ident(Spanned::from(
+                func_meta.name.location.span,
+                self.interner.definition_name(func_meta.name.id).to_string(),
+            ));
+            for (_, typ, _) in func_meta.parameters.iter() {
+                self.check_type_is_not_more_private_then_item(
+                    &name,
+                    modifiers.visibility,
+                    typ,
+                    name.span(),
+                );
+            }
+            self.check_type_is_not_more_private_then_item(
+                &name,
+                modifiers.visibility,
+                func_meta.return_type(),
+                name.span(),
+            );
+        }
+
         self.introduce_generics_into_scope(func_meta.all_generics.clone());
 
         // The DefinitionIds for each parameter were already created in define_function_meta
         // so we need to reintroduce the same IDs into scope here.
         for parameter in &func_meta.parameter_idents {
             let name = self.interner.definition_name(parameter.id).to_owned();
-            self.add_existing_variable_to_scope(name, parameter.clone(), true);
+            let warn_if_unused = !(func_meta.trait_impl.is_some() && name == "self");
+            self.add_existing_variable_to_scope(name, parameter.clone(), warn_if_unused);
         }
 
         self.add_trait_constraints_to_scope(&func_meta);
@@ -1279,14 +1304,49 @@ impl<'context> Elaborator<'context> {
         let typ = self.resolve_type(alias.type_alias_def.typ);
 
         if visibility != ItemVisibility::Private {
-            self.check_aliased_type_is_not_more_private(name, visibility, &typ, span);
+            self.check_type_is_not_more_private_then_item(name, visibility, &typ, span);
         }
 
         self.interner.set_type_alias(alias_id, typ, generics);
         self.generics.clear();
     }
 
-    fn check_aliased_type_is_not_more_private(
+    /// Find the struct in the parent module so we can know its visibility
+    fn find_struct_visibility(&self, struct_type: &StructType) -> Option<ItemVisibility> {
+        let parent_module_id = struct_type.id.parent_module_id(self.def_maps);
+        let parent_module_data = self.get_module(parent_module_id);
+        let per_ns = parent_module_data.find_name(&struct_type.name);
+        per_ns.types.map(|(_, vis, _)| vis)
+    }
+
+    /// Check whether a functions return value and args should be checked for private type visibility.
+    fn should_check_function_visibility(
+        &self,
+        func_meta: &FuncMeta,
+        modifiers: &FunctionModifiers,
+    ) -> bool {
+        // Private functions don't leak anything.
+        if modifiers.visibility == ItemVisibility::Private {
+            return false;
+        }
+        // Implementing public traits on private types is okay, they can't be used unless the type itself is accessible.
+        if func_meta.trait_impl.is_some() {
+            return false;
+        }
+        // Public struct functions should not expose private types.
+        if let Some(struct_visibility) = func_meta.struct_id.and_then(|id| {
+            let struct_def = self.get_struct(id);
+            let struct_def = struct_def.borrow();
+            self.find_struct_visibility(&struct_def)
+        }) {
+            return struct_visibility != ItemVisibility::Private;
+        }
+        // Standalone functions should be checked
+        true
+    }
+
+    /// Check that an item such as a struct field or type alias is not more visible than the type it refers to.
+    fn check_type_is_not_more_private_then_item(
         &mut self,
         name: &Ident,
         visibility: ItemVisibility,
@@ -1302,11 +1362,7 @@ impl<'context> Elaborator<'context> {
                 // then it's either accessible (all good) or it's not, in which case a different
                 // error will happen somewhere else, but no need to error again here.
                 if struct_module_id.krate == self.crate_id {
-                    // Find the struct in the parent module so we can know its visibility
-                    let parent_module_id = struct_type.id.parent_module_id(self.def_maps);
-                    let parent_module_data = self.get_module(parent_module_id);
-                    let per_ns = parent_module_data.find_name(&struct_type.name);
-                    if let Some((_, aliased_visibility, _)) = per_ns.types {
+                    if let Some(aliased_visibility) = self.find_struct_visibility(&struct_type) {
                         if aliased_visibility < visibility {
                             self.push_err(ResolverError::TypeIsMorePrivateThenItem {
                                 typ: struct_type.name.to_string(),
@@ -1318,16 +1374,16 @@ impl<'context> Elaborator<'context> {
                 }
 
                 for generic in generics {
-                    self.check_aliased_type_is_not_more_private(name, visibility, generic, span);
+                    self.check_type_is_not_more_private_then_item(name, visibility, generic, span);
                 }
             }
             Type::Tuple(types) => {
                 for typ in types {
-                    self.check_aliased_type_is_not_more_private(name, visibility, typ, span);
+                    self.check_type_is_not_more_private_then_item(name, visibility, typ, span);
                 }
             }
             Type::Alias(alias_type, generics) => {
-                self.check_aliased_type_is_not_more_private(
+                self.check_type_is_not_more_private_then_item(
                     name,
                     visibility,
                     &alias_type.borrow().get_type(generics),
@@ -1336,17 +1392,17 @@ impl<'context> Elaborator<'context> {
             }
             Type::Function(args, return_type, env, _) => {
                 for arg in args {
-                    self.check_aliased_type_is_not_more_private(name, visibility, arg, span);
+                    self.check_type_is_not_more_private_then_item(name, visibility, arg, span);
                 }
-                self.check_aliased_type_is_not_more_private(name, visibility, return_type, span);
-                self.check_aliased_type_is_not_more_private(name, visibility, env, span);
+                self.check_type_is_not_more_private_then_item(name, visibility, return_type, span);
+                self.check_type_is_not_more_private_then_item(name, visibility, env, span);
             }
             Type::MutableReference(typ) | Type::Array(_, typ) | Type::Slice(typ) => {
-                self.check_aliased_type_is_not_more_private(name, visibility, typ, span);
+                self.check_type_is_not_more_private_then_item(name, visibility, typ, span);
             }
             Type::InfixExpr(left, _op, right) => {
-                self.check_aliased_type_is_not_more_private(name, visibility, left, span);
-                self.check_aliased_type_is_not_more_private(name, visibility, right, span);
+                self.check_type_is_not_more_private_then_item(name, visibility, left, span);
+                self.check_type_is_not_more_private_then_item(name, visibility, right, span);
             }
             Type::FieldElement
             | Type::Integer(..)
@@ -1383,6 +1439,22 @@ impl<'context> Elaborator<'context> {
                 }
             }
 
+            // Check that the a public struct doesn't have a private type as a public field.
+            if typ.struct_def.visibility != ItemVisibility::Private {
+                for field in &fields {
+                    let ident = Ident(Spanned::from(
+                        field.name.span(),
+                        format!("{}::{}", typ.struct_def.name, field.name),
+                    ));
+                    self.check_type_is_not_more_private_then_item(
+                        &ident,
+                        field.visibility,
+                        &field.typ,
+                        field.name.span(),
+                    );
+                }
+            }
+
             let fields_len = fields.len();
             self.interner.update_struct(*type_id, |struct_def| {
                 struct_def.set_fields(fields);

compiler/noirc_frontend/src/tests/unused_items.rs

@@ -274,3 +274,23 @@ fn no_warning_on_indirect_struct_if_it_has_an_abi_attribute() {
     "#;
     assert_no_errors(src);
 }
+
+#[test]
+fn no_warning_on_self_in_trait_impl() {
+    let src = r#" 
+    struct Bar {}
+
+    trait Foo {
+        fn foo(self, a: u64);
+    }
+
+    impl Foo for Bar {
+        fn foo(self, _a: u64) {}
+    }
+
+    fn main() {
+        let _ = Bar {};
+    }
+    "#;
+    assert_no_errors(src);
+}