GNOME Bugzilla – Bug 661876
Using foreach to iterate over an unowned compact class instance fails to compile.
Last modified: 2018-05-22 14:11:39 UTC
Created attachment 199095 [details] Test case I've attached a test vala file that demonstrates the issue; (fail to) compile it with 'valac -c test.vala' The compact class 'TestList' implements a get(i) method and an int size property, so it can be used with foreach. When an unowned reference to this object is used in a foreach loop, the following error is produced: test.vala:20.33-20.33: error: duplicating TestList instance, use unowned variable or explicitly invoke copy method foreach (unowned TestItem b in a) { ^ Compilation failed: 1 error(s), 0 warning(s)
Created attachment 199111 [details] [review] Do not copy collection in the foreach statement Fixes bug 661876.
I believe the main reason why we do the copy is to ensure the collection is alive during the foreach. You could do something like collection=null inside the foreach otherwise which will free it. So I'm resilient to changing this.
There are a lot of ways to invalidate a collection while iterating over it; in some cases, adding or removing items from the collection would do it. There's no guarantee that copying an object will prevent those types of errors, as the data source may be shared between the objects. I think that setting the collection to null while iterating over it falls more into the category of "shooting oneself in the foot"; I'd much rather not have to do a (possibly quite heavyweight) copy just to be protected from a programming error that I didn't make. If the collection is reference-counted, holding an extra reference on the collection while iterating over it wouldn't be too bad, though. This issue only really is a problem with the non-reference counted "Compact" classes.
(In reply to comment #3) > There are a lot of ways to invalidate a collection while iterating over it; in > some cases, adding or removing items from the collection would do it. There's > no guarantee that copying an object will prevent those types of errors, as the > data source may be shared between the objects. It's guaranteed as long as you don't use pointers. That's all about what we care of.
The thing is that in this case, I expect the loop foreach (unowned TestItem b in a) { // do stuff } to be semantically equivalent to the manually coded loop for (int i = 0; i < a.size; i++) { unowned TestItem b = a.get(i); // do stuff } which naturally provides none of this sort of protection; and I wouldn't expect it to do so. I would, of course, expect that it would complain about a null assignment if you had --enable-experimental-non-null enabled; in that case it would be an error to provide a nullable type to the foreach loop. (By the way, I've tested the patch on top of vala-0.14.0, and it works as expected.)
So, have you decided whether to commit the patch in comment #1 , or should I just close this bug as "wontfix" and stick to the old-fashioned integer for loops on the compact classes?
Review of attachment 199111 [details] [review]: The unowned variable must be used only in case the collection is compact, in all other cases the collection must be referenced.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/vala/issues/240.