[move-compiler] add support for public(package) visibility. (#13408)

## Description 

This adds support for `public(package)` visibility, including feature
gating it between Move 2024 edition. It also includes tests to ensure
visibility and feature gate checking.

## Test Plan 

Add new tests to the compiler, including expected results.

### Type of Change (Check all that apply)

- [ ] protocol change
- [x] user-visible impact
- [ ] breaking change for a client SDKs
- [ ] breaking change for FNs (FN binary must upgrade)
- [ ] breaking change for validators or node operators (must upgrade
binaries)
- [ ] breaking change for on-chain data layout
- [ ] necessitate either a data wipe or data migration

### Release notes

Initial `public(package)` support added to Move 2024.alpha. This
compiler feature is intended to be an easier to use variant of
`public(friend)`

---------

Co-authored-by: Tim Zakian <[email protected]>
Co-authored-by: Todd Nowacki <[email protected]>

Author: 3 people

move-compiler/src/cfgir/ast.rs

@@ -4,9 +4,10 @@
 
 use crate::{
     diagnostics::WarningFilters,
-    expansion::ast::{Attributes, Friend, ModuleIdent, Visibility},
+    expansion::ast::{Attributes, Friend, ModuleIdent},
     hlir::ast::{
         BaseType, Command, Command_, FunctionSignature, Label, SingleType, StructDefinition, Var,
+        Visibility,
     },
     parser::ast::{ConstantName, FunctionName, StructName, ENTRY_MODIFIER},
     shared::{ast_debug::*, unique_map::UniqueMap},

move-compiler/src/diagnostics/codes.rs

@@ -202,6 +202,8 @@ codes!(
         InvalidNonPhantomUse:
             { msg: "invalid non-phantom type parameter usage", severity: Warning },
         InvalidAttribute: { msg: "invalid attribute", severity: NonblockingError },
+        InvalidVisibilityModifier:
+            { msg: "invalid visibility modifier", severity: NonblockingError },
     ],
     // errors name resolution, mostly expansion/translate and naming/translate
     NameResolution: [

move-compiler/src/editions/mod.rs

@@ -26,7 +26,9 @@ pub struct Edition {
 }
 
 #[derive(PartialEq, Eq, Clone, Copy, Debug, PartialOrd, Ord)]
-pub enum FeatureGate {}
+pub enum FeatureGate {
+    PublicPackage,
+}
 
 #[derive(PartialEq, Eq, Clone, Copy, Debug, PartialOrd, Ord, Default)]
 pub enum Flavor {
@@ -39,19 +41,39 @@ pub enum Flavor {
 // Entry
 //**************************************************************************************************
 
-pub fn check_feature(env: &mut CompilationEnv, edition: Edition, loc: Loc, feature: FeatureGate) {
-    let is_supported = SUPPORTED_FEATURES.get(&edition).unwrap().contains(&feature);
-    if !is_supported {
-        env.add_diag(diag!(
+pub fn check_feature(env: &mut CompilationEnv, edition: Edition, feature: &FeatureGate, loc: Loc) {
+    if !edition.supports(feature) {
+        let valid_editions = valid_editions_for_feature(feature)
+            .into_iter()
+            .map(|e| e.to_string())
+            .collect::<Vec<_>>()
+            .join(", ");
+        let mut diag = diag!(
             Editions::FeatureTooNew,
             (
                 loc,
-                format!("{feature} requires edition {edition} or newer")
+                format!(
+                    "{feature} not supported by current edition '{edition}', \
+                    only '{valid_editions}' support this feature"
+                )
             )
-        ))
+        );
+        diag.add_note(
+            "You can update the edition in the 'Move.toml', \
+            or via command line flag if invoking the compiler directly.",
+        );
+        env.add_diag(diag);
     }
 }
 
+pub fn valid_editions_for_feature(feature: &FeatureGate) -> Vec<Edition> {
+    Edition::ALL
+        .iter()
+        .filter(|e| e.supports(feature))
+        .copied()
+        .collect()
+}
+
 //**************************************************************************************************
 // impls
 //**************************************************************************************************
@@ -73,6 +95,10 @@ impl Edition {
 
     pub const ALL: &[Self] = &[Self::LEGACY, Self::E2024_ALPHA];
 
+    pub fn supports(&self, feature: &FeatureGate) -> bool {
+        SUPPORTED_FEATURES.get(self).unwrap().contains(feature)
+    }
+
     // Intended only for implementing the lazy static (supported feature map) above
     fn prev(&self) -> Option<Self> {
         match *self {
@@ -87,7 +113,11 @@ impl Edition {
     fn features(&self) -> BTreeSet<FeatureGate> {
         match *self {
             Self::LEGACY => BTreeSet::new(),
-            Self::E2024_ALPHA => self.prev().unwrap().features(),
+            Self::E2024_ALPHA => {
+                let mut features = self.prev().unwrap().features();
+                features.extend([FeatureGate::PublicPackage]);
+                features
+            }
             _ => self.unknown_edition_panic(),
         }
     }

move-compiler/src/expansion/ast.rs

@@ -88,7 +88,9 @@ pub type Attributes = UniqueMap<AttributeName, Attribute>;
 #[derive(Debug, Clone)]
 pub struct Script {
     pub warning_filter: WarningFilters,
-    // package name metadata from compiler arguments, not used for any language rules
+    // package name metadata from compiler arguments.
+    // It is used primarily for retrieving the associated `PackageConfig`,
+    // but it is also used in determining public(package) visibility.
     pub package_name: Option<Symbol>,
     pub attributes: Attributes,
     pub loc: Loc,
@@ -191,6 +193,7 @@ pub enum StructFields {
 pub enum Visibility {
     Public(Loc),
     Friend(Loc),
+    Package(Loc),
     Internal,
 }
 
@@ -719,13 +722,18 @@ impl AbilitySet {
 }
 
 impl Visibility {
-    pub const PUBLIC: &'static str = P::Visibility::PUBLIC;
     pub const FRIEND: &'static str = P::Visibility::FRIEND;
+    pub const FRIEND_IDENT: &'static str = P::Visibility::FRIEND_IDENT;
     pub const INTERNAL: &'static str = P::Visibility::INTERNAL;
+    pub const PACKAGE: &'static str = P::Visibility::PACKAGE;
+    pub const PACKAGE_IDENT: &'static str = P::Visibility::PACKAGE_IDENT;
+    pub const PUBLIC: &'static str = P::Visibility::PUBLIC;
 
     pub fn loc(&self) -> Option<Loc> {
         match self {
-            Visibility::Public(loc) | Visibility::Friend(loc) => Some(*loc),
+            Visibility::Friend(loc) | Visibility::Package(loc) | Visibility::Public(loc) => {
+                Some(*loc)
+            }
             Visibility::Internal => None,
         }
     }
@@ -837,6 +845,7 @@ impl fmt::Display for Visibility {
             match &self {
                 Visibility::Public(_) => Visibility::PUBLIC,
                 Visibility::Friend(_) => Visibility::FRIEND,
+                Visibility::Package(_) => Visibility::PACKAGE,
                 Visibility::Internal => Visibility::INTERNAL,
             }
         )

move-compiler/src/expansion/translate.rs

@@ -5,6 +5,7 @@
 use crate::{
     diag,
     diagnostics::{codes::WarningFilter, Diagnostic, WarningFilters},
+    editions::FeatureGate,
     expansion::{
         aliases::{AliasMap, AliasSet},
         ast::{self as E, Address, Fields, ModuleIdent, ModuleIdent_, SpecId},
@@ -470,6 +471,9 @@ fn module_(
             P::ModuleMember::Spec(s) => specs.push(spec(context, s)),
         }
     }
+
+    check_visibility_modifiers(context, &functions, &friends, package_name);
+
     context.set_to_outer_scope(old_aliases);
 
     let def = E::ModuleDefinition {
@@ -491,6 +495,84 @@ fn module_(
     (current_module, def)
 }
 
+fn check_visibility_modifiers(
+    context: &mut Context,
+    functions: &UniqueMap<FunctionName, E::Function>,
+    friends: &UniqueMap<ModuleIdent, E::Friend>,
+    package_name: Option<Symbol>,
+) {
+    let mut friend_usage = friends.iter().next().map(|(_, _, friend)| friend.loc);
+    let mut public_package_usage = None;
+    for (_, _, function) in functions {
+        match function.visibility {
+            E::Visibility::Friend(loc) if friend_usage.is_none() => {
+                friend_usage = Some(loc);
+            }
+            E::Visibility::Package(loc) => {
+                context
+                    .env
+                    .check_feature(&FeatureGate::PublicPackage, package_name, loc);
+                public_package_usage = Some(loc);
+            }
+            _ => (),
+        }
+    }
+
+    // Emit any errors.
+    if public_package_usage.is_some() && friend_usage.is_some() {
+        let friend_error_msg = format!(
+            "Cannot define 'friend' modules and use '{}' visibility in the same module",
+            E::Visibility::PACKAGE
+        );
+        let package_definition_msg = format!("'{}' visibility used here", E::Visibility::PACKAGE);
+        for (_, _, friend) in friends {
+            context.env.add_diag(diag!(
+                Declarations::InvalidVisibilityModifier,
+                (friend.loc, friend_error_msg.clone()),
+                (
+                    public_package_usage.unwrap(),
+                    package_definition_msg.clone()
+                )
+            ));
+        }
+        let package_error_msg = format!(
+            "Cannot mix '{}' and '{}' visibilities in the same module",
+            E::Visibility::PACKAGE_IDENT,
+            E::Visibility::FRIEND_IDENT
+        );
+        let friend_error_msg = format!(
+            "Cannot mix '{}' and '{}' visibilities in the same module",
+            E::Visibility::FRIEND_IDENT,
+            E::Visibility::PACKAGE_IDENT
+        );
+        for (_, _, function) in functions {
+            match function.visibility {
+                E::Visibility::Friend(loc) => {
+                    context.env.add_diag(diag!(
+                        Declarations::InvalidVisibilityModifier,
+                        (loc, friend_error_msg.clone()),
+                        (
+                            public_package_usage.unwrap(),
+                            package_definition_msg.clone()
+                        )
+                    ));
+                }
+                E::Visibility::Package(loc) => {
+                    context.env.add_diag(diag!(
+                        Declarations::InvalidVisibilityModifier,
+                        (loc, package_error_msg.clone()),
+                        (
+                            friend_usage.unwrap(),
+                            &format!("'{}' visibility used here", E::Visibility::FRIEND_IDENT)
+                        )
+                    ));
+                }
+                _ => {}
+            }
+        }
+    }
+}
+
 fn script(
     context: &mut Context,
     scripts: &mut Vec<E::Script>,
@@ -536,7 +618,7 @@ fn script_(context: &mut Context, package_name: Option<Symbol>, pscript: P::Scri
     check_valid_module_member_name(context, ModuleMemberKind::Function, pfunction.name.0);
     let (function_name, function) = function_(context, 0, pfunction);
     match &function.visibility {
-        E::Visibility::Public(loc) | E::Visibility::Friend(loc) => {
+        E::Visibility::Friend(loc) | E::Visibility::Package(loc) | E::Visibility::Public(loc) => {
             let msg = format!(
                 "Invalid '{}' visibility modifier. \
                 Script functions are not callable from other Move functions.",
@@ -1367,13 +1449,14 @@ fn function_(
 
 fn visibility(context: &mut Context, pvisibility: P::Visibility) -> E::Visibility {
     match pvisibility {
+        P::Visibility::Friend(loc) => E::Visibility::Friend(loc),
+        P::Visibility::Internal => E::Visibility::Internal,
+        P::Visibility::Package(loc) => E::Visibility::Package(loc),
         P::Visibility::Public(loc) => E::Visibility::Public(loc),
         P::Visibility::Script(loc) => {
             assert!(!context.env.has_errors());
             E::Visibility::Public(loc)
         }
-        P::Visibility::Friend(loc) => E::Visibility::Friend(loc),
-        P::Visibility::Internal => E::Visibility::Internal,
     }
 }
 

move-compiler/src/hlir/ast.rs

@@ -6,10 +6,11 @@ use crate::{
     diagnostics::WarningFilters,
     expansion::ast::{
         ability_modifiers_ast_debug, AbilitySet, Attributes, Friend, ModuleIdent, SpecId,
-        Visibility,
     },
     naming::ast::{BuiltinTypeName, BuiltinTypeName_, StructTypeParameter, TParam},
-    parser::ast::{BinOp, ConstantName, Field, FunctionName, StructName, UnaryOp, ENTRY_MODIFIER},
+    parser::ast::{
+        self as P, BinOp, ConstantName, Field, FunctionName, StructName, UnaryOp, ENTRY_MODIFIER,
+    },
     shared::{ast_debug::*, unique_map::UniqueMap, Name, NumericalAddress, TName},
 };
 use move_ir_types::location::*;
@@ -103,6 +104,14 @@ pub struct Constant {
 // Functions
 //**************************************************************************************************
 
+// package visibility is removed after typing is done
+#[derive(PartialEq, Eq, Debug, Clone)]
+pub enum Visibility {
+    Public(Loc),
+    Friend(Loc),
+    Internal,
+}
+
 #[derive(PartialEq, Eq, Debug, Clone)]
 pub struct FunctionSignature {
     pub type_parameters: Vec<TParam>,
@@ -395,6 +404,19 @@ impl Var {
     }
 }
 
+impl Visibility {
+    pub const FRIEND: &'static str = P::Visibility::FRIEND;
+    pub const INTERNAL: &'static str = P::Visibility::INTERNAL;
+    pub const PUBLIC: &'static str = P::Visibility::PUBLIC;
+
+    pub fn loc(&self) -> Option<Loc> {
+        match self {
+            Visibility::Friend(loc) | Visibility::Public(loc) => Some(*loc),
+            Visibility::Internal => None,
+        }
+    }
+}
+
 impl Command_ {
     pub fn is_terminal(&self) -> bool {
         use Command_::*;
@@ -739,6 +761,20 @@ impl std::fmt::Display for TypeName_ {
     }
 }
 
+impl std::fmt::Display for Visibility {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        write!(
+            f,
+            "{}",
+            match &self {
+                Visibility::Public(_) => Visibility::PUBLIC,
+                Visibility::Friend(_) => Visibility::FRIEND,
+                Visibility::Internal => Visibility::INTERNAL,
+            }
+        )
+    }
+}
+
 impl std::fmt::Display for Label {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         write!(f, "{}", self.0)
@@ -948,6 +984,12 @@ impl AstDebug for FunctionSignature {
     }
 }
 
+impl AstDebug for Visibility {
+    fn ast_debug(&self, w: &mut AstWriter) {
+        w.write(&format!("{} ", self))
+    }
+}
+
 impl AstDebug for Var {
     fn ast_debug(&self, w: &mut AstWriter) {
         w.write(&format!("{}", self.0))

move-compiler/src/hlir/translate.rs

@@ -293,7 +293,7 @@ fn function(context: &mut Context, _name: FunctionName, f: T::Function) -> H::Fu
         warning_filter,
         index,
         attributes,
-        visibility,
+        visibility: evisibility,
         entry,
         signature,
         acquires,
@@ -307,7 +307,7 @@ fn function(context: &mut Context, _name: FunctionName, f: T::Function) -> H::Fu
         warning_filter,
         index,
         attributes,
-        visibility,
+        visibility: visibility(evisibility),
         entry,
         signature,
         acquires,
@@ -384,6 +384,16 @@ fn function_body_defined(
     (locals, body)
 }
 
+fn visibility(evisibility: E::Visibility) -> H::Visibility {
+    match evisibility {
+        E::Visibility::Internal => H::Visibility::Internal,
+        E::Visibility::Friend(loc) => H::Visibility::Friend(loc),
+        // We added any friends we needed during typing, so we convert this over.
+        E::Visibility::Package(loc) => H::Visibility::Friend(loc),
+        E::Visibility::Public(loc) => H::Visibility::Public(loc),
+    }
+}
+
 //**************************************************************************************************
 // Constants
 //**************************************************************************************************