vala: Detect array element ownership mismatch

Variations of the following are already detected and prohibited:

  string make_string ();
  unowned string s = make_string ();

including with arrays:

  string[] make_string_array ();
  unowned string[] arr = make_string_array ();

however, Vala failed to detect the case where the array itself is owned,
but its elements are not, aka transfer container:

  string[] make_string_array ();
  (unowned string)[] arr = make_string_array ();

To catch this, introduce a new virtual method, transfer_compatible (),
which is overriden in Vala.ArrayType to check for element type
ownership, and use it throughout.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>

diff --git a/tests/Makefile.am b/tests/Makefile.am
index f60b145ae11af108bf2d025655713cba899114ff..d7d76f1278b2d46bd20fce1e6b485e77f95b372b 100644 (file)
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -176,6 +176,9 @@ TESTS = \
        arrays/slice-invalid-start.test \
        arrays/slice-invalid-stop.test \
        arrays/slice-no-array.test \
+       arrays/transfer-container-field.test \
+       arrays/transfer-container-local.test \
+       arrays/transfer-container-return.test \
        chainup/base-arguments.test \
        chainup/base-class-invalid.test \
        chainup/base-enum-invalid.test \

diff --git a/tests/arrays/transfer-container-field.test b/tests/arrays/transfer-container-field.test
new file mode 100644 (file)
index 0000000..8fe1e50
--- /dev/null
+++ b/tests/arrays/transfer-container-field.test
@@ -0,0 +1,12 @@
+Invalid Code
+
+class Foo {
+       private (unowned string)[] field;
+
+       public Foo () {
+               field = new string[] { "what" };
+       }
+}
+
+void main () {
+}

diff --git a/tests/arrays/transfer-container-local.test b/tests/arrays/transfer-container-local.test
new file mode 100644 (file)
index 0000000..9ce0085
--- /dev/null
+++ b/tests/arrays/transfer-container-local.test
@@ -0,0 +1,5 @@
+Invalid Code
+
+void main () {
+       (unowned string)[] foo = new string[] { "foo" };
+}

diff --git a/tests/arrays/transfer-container-return.test b/tests/arrays/transfer-container-return.test
new file mode 100644 (file)
index 0000000..3d4260d
--- /dev/null
+++ b/tests/arrays/transfer-container-return.test
@@ -0,0 +1,8 @@
+Invalid Code
+
+(unowned string)[] return_string_array () {
+       return new string[] { "foo" };
+}
+
+void main () {
+}

diff --git a/vala/valaarraytype.vala b/vala/valaarraytype.vala
index 59323331f1c773a57b8d51d071f5f0c29bf08b2f..606f13cde55783d8f616ebe7719e2ff607812755 100644 (file)
--- a/vala/valaarraytype.vala
+++ b/vala/valaarraytype.vala
@@ -267,6 +267,22 @@ public class Vala.ArrayType : ReferenceType {
                return false;
        }
 
+       public override bool transfer_compatible (DataType target_type) {
+               if (!compatible (target_type)) {
+                       return false;
+               }
+               if (!(target_type is ArrayType)) {
+                       // Allow pointers etc.
+                       return true;
+               }
+               if (value_owned && !target_type.value_owned) {
+                       return false;
+               }
+
+               ArrayType target_array_type = (ArrayType) target_type;
+               return element_type.transfer_compatible (target_array_type.element_type);
+       }
+
        public override bool is_reference_type_or_type_parameter () {
                return true;
        }

diff --git a/vala/valaassignment.vala b/vala/valaassignment.vala
index e9fb6054ab8dd3151c1fe52bcde9ee680acfca66..0980f945922d317ed59598c6165ced64ec119a54 100644 (file)
--- a/vala/valaassignment.vala
+++ b/vala/valaassignment.vala
@@ -385,18 +385,9 @@ public class Vala.Assignment : Expression {
                                }
 
                                if (!(ma.symbol_reference is Property)) {
-                                       if (right.value_type.is_disposable ()) {
-                                               /* rhs transfers ownership of the expression */
-                                               if (!(left.value_type is PointerType) && !left.value_type.value_owned) {
-                                                       /* lhs doesn't own the value */
-                                                       error = true;
-                                                       Report.error (source_reference, "Invalid assignment from owned expression to unowned variable");
-                                               }
-                                       } else if (left.value_type.value_owned) {
-                                               /* lhs wants to own the value
-                                                * rhs doesn't transfer the ownership
-                                                * code generator needs to add reference
-                                                * increment calls */
+                                       if (!right.value_type.transfer_compatible (left.value_type)) {
+                                               error = true;
+                                               Report.error (source_reference, "Invalid assignment from owned expression to unowned variable");
                                        }
                                }
                        }
@@ -426,31 +417,24 @@ public class Vala.Assignment : Expression {
                                return false;
                        }
 
-                       if (right.value_type.is_disposable ()) {
-                               /* rhs transfers ownership of the expression */
-
-                               DataType element_type;
+                       DataType element_type;
 
-                               if (ea.container.value_type is ArrayType) {
-                                       unowned ArrayType array_type = (ArrayType) ea.container.value_type;
-                                       element_type = array_type.element_type;
-                               } else {
-                                       var args = ea.container.value_type.get_type_arguments ();
-                                       assert (args.size == 1);
-                                       element_type = args.get (0);
-                               }
+                       if (ea.container.value_type is ArrayType) {
+                               unowned ArrayType array_type = (ArrayType) ea.container.value_type;
+                               element_type = array_type.element_type;
+                       } else if (ea.container.value_type is PointerType) {
+                               unowned PointerType pointer_type = (PointerType) ea.container.value_type;
+                               element_type = pointer_type.base_type;
+                       } else {
+                               var args = ea.container.value_type.get_type_arguments ();
+                               assert (args.size == 1);
+                               element_type = args.get (0);
+                       }
 
-                               if (!(element_type is PointerType) && !element_type.value_owned) {
-                                       /* lhs doesn't own the value */
-                                       error = true;
-                                       Report.error (source_reference, "Invalid assignment from owned expression to unowned variable");
-                                       return false;
-                               }
-                       } else if (left.value_type.value_owned) {
-                               /* lhs wants to own the value
-                                * rhs doesn't transfer the ownership
-                                * code generator needs to add reference
-                                * increment calls */
+                       if (!right.value_type.transfer_compatible (element_type)) {
+                               error = true;
+                               Report.error (source_reference, "Invalid assignment from owned expression to unowned variable");
+                               return false;
                        }
                } else {
                        return true;

diff --git a/vala/valadatatype.vala b/vala/valadatatype.vala
index 939a6a9e73e05a656f3fb5318f387be55d9cf200..2a879aecdb44d23691ebac2dc05244ba400bc03d 100644 (file)
--- a/vala/valadatatype.vala
+++ b/vala/valadatatype.vala
@@ -403,6 +403,21 @@ public abstract class Vala.DataType : CodeNode {
                return false;
        }
 
+       public virtual bool transfer_compatible (DataType target_type) {
+               // Check if an instance of this type can transfer ownership (if any)
+               // to an instance of the target type.
+               if (!compatible (target_type)) {
+                       return false;
+               }
+               if (target_type is PointerType) {
+                       return true;
+               }
+               if (is_disposable () && !target_type.value_owned) {
+                       return false;
+               }
+               return true;
+       }
+
        /**
         * Returns whether instances of this type are invokable.
         *

diff --git a/vala/valafield.vala b/vala/valafield.vala
index 0204d24d87617a4368d8351923e2e73c54cb06df..525071b1120879bdb0540e7f3bfc1a4df80fa1ce 100644 (file)
--- a/vala/valafield.vala
+++ b/vala/valafield.vala
@@ -180,14 +180,10 @@ public class Vala.Field : Variable, Lockable {
                                return false;
                        }
 
-                       if (initializer.value_type.is_disposable ()) {
-                               /* rhs transfers ownership of the expression */
-                               if (!(variable_type is PointerType) && !variable_type.value_owned) {
-                                       /* lhs doesn't own the value */
-                                       error = true;
-                                       Report.error (source_reference, "Invalid assignment from owned expression to unowned variable");
-                                       return false;
-                               }
+                       if (!initializer.value_type.transfer_compatible (variable_type)) {
+                               error = true;
+                               Report.error (source_reference, "Invalid assignment from owned expression to unowned variable");
+                               return false;
                        }
 
                        if (parent_symbol is Namespace && !initializer.is_constant ()) {

diff --git a/vala/valaforeachstatement.vala b/vala/valaforeachstatement.vala
index 69d49eccb445b1f663efd65c588e75992343fb4d..4ff1531046dfb61b879154a8feb99ff3c645cbb3 100644 (file)
--- a/vala/valaforeachstatement.vala
+++ b/vala/valaforeachstatement.vala
@@ -360,7 +360,7 @@ public class Vala.ForeachStatement : Block {
                        error = true;
                        Report.error (source_reference, "Foreach: Cannot convert from `%s' to `%s'", element_type.to_string (), type_reference.to_string ());
                        return false;
-               } else if (element_type.is_disposable () && element_type.value_owned && !type_reference.value_owned) {
+               } else if (!element_type.transfer_compatible (type_reference)) {
                        error = true;
                        Report.error (source_reference, "Foreach: Invalid assignment from owned expression to unowned variable");
                        return false;

diff --git a/vala/valalocalvariable.vala b/vala/valalocalvariable.vala
index 099152c20c86e73d5f7ca3a4a2225867726e6fa3..3a2d30aafcbf75088baa569ccb732d00bb7147e3 100644 (file)
--- a/vala/valalocalvariable.vala
+++ b/vala/valalocalvariable.vala
@@ -242,14 +242,10 @@ public class Vala.LocalVariable : Variable {
                                return false;
                        }
 
-                       if (initializer.value_type.is_disposable ()) {
-                               /* rhs transfers ownership of the expression */
-                               if (!(variable_type is PointerType) && !variable_type.value_owned) {
-                                       /* lhs doesn't own the value */
-                                       error = true;
-                                       Report.error (source_reference, "Invalid assignment from owned expression to unowned variable");
-                                       return false;
-                               }
+                       if (!initializer.value_type.transfer_compatible (variable_type)) {
+                               error = true;
+                               Report.error (source_reference, "Invalid assignment from owned expression to unowned variable");
+                               return false;
                        }
                }
 

diff --git a/vala/valareturnstatement.vala b/vala/valareturnstatement.vala
index 3873083ee648228398c689316f43873dacf78d0e..d51cae77ccf925fcc15909fa9494dc112f93748b 100644 (file)
--- a/vala/valareturnstatement.vala
+++ b/vala/valareturnstatement.vala
@@ -123,16 +123,14 @@ public class Vala.ReturnStatement : CodeNode, Statement {
                        return false;
                }
 
-               if (return_expression.value_type.is_disposable () &&
-                   !context.analyzer.current_return_type.value_owned) {
+               if (!return_expression.value_type.transfer_compatible (context.analyzer.current_return_type)) {
                        error = true;
                        Report.error (source_reference, "Return value transfers ownership but method return type hasn't been declared to transfer ownership");
                        return false;
                }
 
                unowned LocalVariable? local = return_expression.symbol_reference as LocalVariable;
-               if (local != null && local.variable_type.is_disposable () &&
-                   !context.analyzer.current_return_type.value_owned) {
+               if (local != null && !local.variable_type.transfer_compatible (context.analyzer.current_return_type)) {
                        error = true;
                        Report.error (source_reference, "Local variable with strong reference used as return value and method return type has not been declared to transfer ownership");
                        return false;

diff --git a/vala/valasemanticanalyzer.vala b/vala/valasemanticanalyzer.vala
index b560f2a7ac206e58b861cd2500eff84b1bc7d6b5..bce6529c309bdfacabf53f13ea9f43a53dd96c55 100644 (file)
--- a/vala/valasemanticanalyzer.vala
+++ b/vala/valasemanticanalyzer.vala
@@ -643,22 +643,18 @@ public class Vala.SemanticAnalyzer : CodeVisitor {
                                        return false;
                                }
 
-                               // weak variables can only be used with weak ref parameters
-                               if (arg.target_type.is_disposable ()) {
-                                       if (!(arg.value_type is PointerType) && !arg.value_type.value_owned) {
-                                               /* variable doesn't own the value */
-                                               Report.error (arg.source_reference, "Argument %d: Cannot pass unowned ref argument to owned reference parameter", i + 1);
-                                               return false;
-                                       }
+                               // unowned variables can only be used with unowned ref parameters
+                               if (!arg.target_type.transfer_compatible (arg.value_type)) {
+                                       /* variable doesn't own the value */
+                                       Report.error (arg.source_reference, "Argument %d: Cannot pass unowned ref argument to owned reference parameter", i + 1);
+                                       return false;
                                }
 
                                // owned variables can only be used with owned ref parameters
-                               if (arg.value_type.is_disposable ()) {
-                                       if (!arg.target_type.value_owned) {
-                                               /* parameter doesn't own the value */
-                                               Report.error (arg.source_reference, "Argument %d: Cannot pass owned ref argument to unowned reference parameter", i + 1);
-                                               return false;
-                                       }
+                               if (!arg.value_type.transfer_compatible (arg.target_type)) {
+                                       /* parameter doesn't own the value */
+                                       Report.error (arg.source_reference, "Argument %d: Cannot pass owned ref argument to unowned reference parameter", i + 1);
+                                       return false;
                                }
                        } else if (arg_type == 3) {
                                if (direction != ParameterDirection.OUT) {
@@ -666,13 +662,11 @@ public class Vala.SemanticAnalyzer : CodeVisitor {
                                        return false;
                                }
 
-                               // weak variables can only be used with weak out parameters
-                               if (arg.target_type.is_disposable ()) {
-                                       if (!(arg.value_type is PointerType) && !arg.value_type.value_owned) {
-                                               /* variable doesn't own the value */
-                                               Report.error (arg.source_reference, "Invalid assignment from owned expression to unowned variable");
-                                               return false;
-                                       }
+                               // unowned variables can only be used with unowned out parameters
+                               if (!arg.target_type.transfer_compatible (arg.value_type)) {
+                                       /* variable doesn't own the value */
+                                       Report.error (arg.source_reference, "Invalid assignment from owned expression to unowned variable");
+                                       return false;
                                }
                        }
                }