Repository: aptos-labs/aptos-core (language: python)

Issue:
[Bug][compiler-v2] ability missing warning comes too late
# 🐛 Bug

Compiling the following Move code with V2 compiler 

```move
module demo::demo {
    struct S<T: store> has store {
        field: T,
    }

    struct E<T: store> has store, drop {
        entry: S<T>,
    }
}
```
yields an error at the File Format generation phase:

```
bug: bytecode verification failed with unexpected status code `FIELD_MISSING_TYPE_ABILITY`. This is a compiler bug, consider reporting it.
  ┌─ /<redacted>/test/sources/test.move:1:1
  │  
1 │ ╭ module demo::demo {
2 │ │     struct S<T: store> has store {
3 │ │         field: T,
4 │ │     }
  · │
8 │ │     }
9 │ │ }
  │ ╰─^
```

Which is a bit late in the process.

With V1 compiler, the error message is more dev-friendly:

```
error[E05001]: ability constraint not satisfied
  ┌─ /<redacted>/test/sources/test.move:7:16
  │
2 │     struct S<T: store> has store {
  │            - To satisfy the constraint, the 'drop' ability would need to be added here
  ·
7 │         entry: S<T>,
  │                ^^^^
  │                │
  │                Invalid field type. The struct was declared with the ability 'drop' so all fields require the ability 'drop'
  │                The type '(demo=0x97751FCABB9A35CABA546D45C30E91CEB8B4622386C2DBF81B9B6D64A9F53AEF)::demo::S<_>' does not have the ability 'drop'
```



Discussion / hints:
This is the bytecode verifier as a verification step after code generation, and any error like this is definitely an unintended bug, though safe because the code can't be deployed.

Fix patch:
diff --git a/third_party/move/move-model/src/ty.rs b/third_party/move/move-model/src/ty.rs
index e19f9172b3868..cdd969189a873 100644
--- 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")
                     },
                 };


Test patch:
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
new file mode 100644
index 0000000000000..047c555c3bb60
--- /dev/null
+++ 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
new file mode 100644
index 0000000000000..c4e5e4b9d4e2f
--- /dev/null
+++ 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/third_party/move/move-compiler-v2/tests/checking/positional_fields/named_tuple_ability_decl_ok.exp b/third_party/move/move-compiler-v2/tests/checking/positional_fields/named_tuple_ability_decl_ok.exp
index 5b6703568a6ab..a6056236ef1fc 100644
--- a/third_party/move/move-compiler-v2/tests/checking/positional_fields/named_tuple_ability_decl_ok.exp
+++ b/third_party/move/move-compiler-v2/tests/checking/positional_fields/named_tuple_ability_decl_ok.exp
@@ -38,7 +38,7 @@ module 0x42::test {
         0: T,
         1: u8,
     }
-    struct S3<T: key> has key {
+    struct S3<T: key> has store, key {
         0: T,
         1: u8,
     }
@@ -46,7 +46,7 @@ module 0x42::test {
         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/third_party/move/move-compiler-v2/tests/checking/positional_fields/named_tuple_ability_decl_ok.move b/third_party/move/move-compiler-v2/tests/checking/positional_fields/named_tuple_ability_decl_ok.move
index e215df6ba5eef..495b88e6eb4f8 100644
--- a/third_party/move/move-compiler-v2/tests/checking/positional_fields/named_tuple_ability_decl_ok.move
+++ b/third_party/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>();
 }