[compiler-v2] Revise ability checking for type parameters (#15006)

* [compiler-v2] Revise ability checking for type parameters Closes #14490 Move has some unusual semantics for ability checking of type parameters - In most contexts, a type parameter is assumed to have all abilities during checking. Then when the parameter is instantiated, checking takes place. - However, there is one exemption, namely when a type parameter is used to instantiate another type parameter which has an abilitie constraint, as in `f<T:drop>`. If we instantiate `f<R>` then `R` needs to be checked for `drop`. This PR solves this by adding a field `type_param_exempted` to `HasAbilities(abilities, type_param_exempted)` to the ability constraint. * Adding enum `AbilityCheckingScope` to make the change clearer.
aptos-labs · Oct 18, 2024 · 87348fc · 87348fc
1 parent 20ba96d
commit 87348fc
Show file tree

Hide file tree

Showing 5 changed files with 74 additions and 39 deletions.
diff --git a/third_party/move/move-compiler-v2/tests/checking/abilities/bug_14490_field_abilities.exp b/third_party/move/move-compiler-v2/tests/checking/abilities/bug_14490_field_abilities.exp
@@ -0,0 +1,9 @@
+
+Diagnostics:
+error: type `S<T>` is missing required ability `drop`
+  ┌─ tests/checking/abilities/bug_14490_field_abilities.move:7:16
+  │
+7 │         entry: S<T>,
+  │                ^^^^
+  │
+  = required by declaration of field `entry`
diff --git a/third_party/move/move-compiler-v2/tests/checking/abilities/bug_14490_field_abilities.move b/third_party/move/move-compiler-v2/tests/checking/abilities/bug_14490_field_abilities.move
@@ -0,0 +1,9 @@
+module 0x815::demo {
+    struct S<T: store> has store {
+        field: T,
+    }
+
+    struct E<T: store> has store, drop {
+        entry: S<T>,
+    }
+}
diff --git a/...ty/move/move-compiler-v2/tests/checking/positional_fields/named_tuple_ability_decl_ok.exp b/...ty/move/move-compiler-v2/tests/checking/positional_fields/named_tuple_ability_decl_ok.exp
@@ -38,15 +38,15 @@ module 0x42::test {
         0: T,
         1: u8,
     }
-    struct S3<T: key> has key {
+    struct S3<T: key> has store, key {
         0: T,
         1: u8,
     }
     struct S4<T: key> has drop {
         x: u8,
         y: T,
     }
-    struct S5<T: copy + key> has key {
+    struct S5<T: copy + store + key> has key {
         0: T,
         1: S3<T>,
     }

diff --git a/...y/move/move-compiler-v2/tests/checking/positional_fields/named_tuple_ability_decl_ok.move b/...y/move/move-compiler-v2/tests/checking/positional_fields/named_tuple_ability_decl_ok.move
@@ -5,14 +5,14 @@ module 0x42::test {
 
     struct S2<T>(T, u8) has key;
 
-    struct S3<T: key>(T, u8) has key;
+    struct S3<T: key>(T, u8) has key, store;
 
     struct S4<T: key> has drop {
         x: u8,
         y: T,
     }
 
-    struct S5<T: copy + key>(T, S3<T>) has key;
+    struct S5<T: copy + key + store>(T, S3<T>) has key;
 
     struct S6<phantom T: store>();
 }
diff --git a/third_party/move/move-model/src/ty.rs b/third_party/move/move-model/src/ty.rs
@@ -150,7 +150,7 @@ pub enum Constraint {
     /// as a type argument for a phantom type parameter.
     NoPhantom,
     /// The type must have the given set of abilities.
-    HasAbilities(AbilitySet),
+    HasAbilities(AbilitySet, AbilityCheckingScope),
     /// The type variable defaults to the given type if no other binding is found. This is
     /// a pseudo constraint which never fails, but used to generate a default for
     /// inference.
@@ -160,6 +160,19 @@ pub enum Constraint {
     NoFunction,
 }
 
+/// Scope of ability checking.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+pub enum AbilityCheckingScope {
+    /// Type parameters are excluded from ability checking. This is in usages the case
+    /// where we check abilities for field types, for example, since those constraints
+    /// are modulo an actual type instantiation.
+    ExcludeTypeParams,
+    /// Type parameters are included in ability checking. This is the case if
+    /// we check ability constraints for a type instantiation, as in `S<T>`,
+    /// and we have `struct S<X:A>`.
+    IncludeTypeParams,
+}
+
 /// A type to describe the context from where a constraint stems. Used for
 /// error reporting.
 #[derive(Debug, Clone, Default)]
@@ -403,7 +416,7 @@ impl Constraint {
     pub fn accumulating(&self) -> bool {
         matches!(
             self,
-            Constraint::HasAbilities(_)
+            Constraint::HasAbilities(..)
                 | Constraint::WithDefault(_)
                 | Constraint::NoPhantom
                 | Constraint::NoTuple
@@ -519,7 +532,9 @@ impl Constraint {
             (Constraint::NoReference, Constraint::NoReference) => Ok(true),
             (Constraint::NoTuple, Constraint::NoTuple) => Ok(true),
             (Constraint::NoPhantom, Constraint::NoPhantom) => Ok(true),
-            (Constraint::HasAbilities(a1), Constraint::HasAbilities(a2)) => {
+            (Constraint::HasAbilities(a1, scope1), Constraint::HasAbilities(a2, scope2))
+                if scope1 == scope2 =>
+            {
                 *a1 = a1.union(*a2);
                 Ok(true)
             },
@@ -556,7 +571,10 @@ impl Constraint {
             result.push(Constraint::NoPhantom)
         }
         if !abilities.is_empty() {
-            result.push(Constraint::HasAbilities(*abilities));
+            result.push(Constraint::HasAbilities(
+                *abilities,
+                AbilityCheckingScope::IncludeTypeParams,
+            ));
         }
         result
     }
@@ -573,23 +591,22 @@ impl Constraint {
 
     /// Returns the constraints which need to be satisfied for a field type,
     /// given a struct with declared abilities.
-    pub fn for_field(struct_abilities: AbilitySet, field_ty: &Type) -> Vec<Constraint> {
+    pub fn for_field(struct_abilities: AbilitySet, _field_ty: &Type) -> Vec<Constraint> {
         let mut result = vec![
             Constraint::NoPhantom,
             Constraint::NoTuple,
             Constraint::NoReference,
             Constraint::NoFunction,
         ];
-        let abilities = if !field_ty.depends_from_type_parameter() {
-            if struct_abilities.has_ability(Ability::Key) {
-                struct_abilities.remove(Ability::Key).add(Ability::Store)
-            } else {
-                struct_abilities
-            }
+        let abilities = if struct_abilities.has_ability(Ability::Key) {
+            struct_abilities.remove(Ability::Key).add(Ability::Store)
         } else {
-            AbilitySet::EMPTY
+            struct_abilities
         };
-        result.push(Constraint::HasAbilities(abilities));
+        result.push(Constraint::HasAbilities(
+            abilities,
+            AbilityCheckingScope::ExcludeTypeParams,
+        ));
         result
     }
 
@@ -652,7 +669,7 @@ impl Constraint {
             Constraint::NoFunction => "no-func".to_string(),
             Constraint::NoTuple => "no-tuple".to_string(),
             Constraint::NoPhantom => "no-phantom".to_string(),
-            Constraint::HasAbilities(required_abilities) => {
+            Constraint::HasAbilities(required_abilities, _) => {
                 format!("{}", required_abilities)
             },
             Constraint::WithDefault(_ty) => "".to_owned(),
@@ -800,20 +817,6 @@ impl Type {
         matches!(self, Type::TypeParameter(..))
     }
 
-    /// Returns true if the type depends on type parameters.
-    pub fn depends_from_type_parameter(&self) -> bool {
-        use Type::*;
-        match self {
-            TypeParameter(_) => true,
-            Tuple(ts) | Struct(_, _, ts) | ResourceDomain(_, _, Some(ts)) => {
-                ts.iter().any(|t| t.depends_from_type_parameter())
-            },
-            Vector(t) | Reference(_, t) | TypeDomain(t) => t.depends_from_type_parameter(),
-            Fun(t, r) => t.depends_from_type_parameter() || r.depends_from_type_parameter(),
-            _ => false,
-        }
-    }
-
     /// Determines whether this is a primitive.
     pub fn is_primitive(&self) -> bool {
         matches!(self, Type::Primitive(_))
@@ -1797,9 +1800,15 @@ impl Substitution {
                         constraint_unsatisfied_error()
                     }
                 },
-                (Constraint::HasAbilities(required_abilities), ty) => {
-                    self.eval_ability_constraint(context, loc, *required_abilities, ty, ctx_opt)
-                },
+                (Constraint::HasAbilities(required_abilities, scope), ty) => self
+                    .eval_ability_constraint(
+                        context,
+                        loc,
+                        *required_abilities,
+                        *scope,
+                        ty,
+                        ctx_opt,
+                    ),
                 (Constraint::NoReference, ty) => {
                     if ty.is_reference() {
                         constraint_unsatisfied_error()
@@ -1838,6 +1847,7 @@ impl Substitution {
         context: &mut impl UnificationContext,
         loc: &Loc,
         required_abilities: AbilitySet,
+        required_abilities_scope: AbilityCheckingScope,
         ty: &Type,
         ctx_opt: Option<ConstraintContext>,
     ) -> Result<(), TypeUnificationError> {
@@ -1865,6 +1875,7 @@ impl Substitution {
                         context,
                         loc,
                         required_abilities,
+                        required_abilities_scope,
                         t,
                         ctx_opt.clone().map(|ctx| ctx.derive_tuple_element(i)),
                     )?;
@@ -1877,6 +1888,7 @@ impl Substitution {
                     context,
                     loc,
                     required_abilities,
+                    required_abilities_scope,
                     t,
                     ctx_opt.map(|ctx| ctx.derive_vector_type_param()),
                 )
@@ -1900,6 +1912,7 @@ impl Substitution {
                             context,
                             loc,
                             required,
+                            required_abilities_scope,
                             t,
                             ctx_opt
                                 .clone()
@@ -1926,8 +1939,12 @@ impl Substitution {
                 Ok(())
             },
             TypeParameter(idx) => {
-                let tparam = context.type_param(*idx);
-                check(tparam.1.abilities)
+                if required_abilities_scope == AbilityCheckingScope::IncludeTypeParams {
+                    let tparam = context.type_param(*idx);
+                    check(tparam.1.abilities)
+                } else {
+                    Ok(())
+                }
             },
             Fun(_, _) => check(AbilitySet::FUNCTIONS),
             Reference(_, _) => check(AbilitySet::REFERENCES),
@@ -1941,7 +1958,7 @@ impl Substitution {
                     *idx,
                     loc.clone(),
                     WideningOrder::LeftToRight,
-                    Constraint::HasAbilities(required_abilities),
+                    Constraint::HasAbilities(required_abilities, required_abilities_scope),
                     ctx_opt,
                 )
             },
@@ -2997,7 +3014,7 @@ impl TypeUnificationError {
                             ty.display(display_context)
                         )
                     },
-                    Constraint::HasAbilities(_) | Constraint::WithDefault(_) => {
+                    Constraint::HasAbilities(..) | Constraint::WithDefault(_) => {
                         unreachable!("unexpected constraint in error message")
                     },
                 };