chapel-lang / chapel

a Productive Parallel Programming Language
https://chapel-lang.org
Other
1.8k stars 422 forks source link

Compiler considers some immutably-copyable types non-immutably-copyable #26234

Open DanilaFe opened 6 days ago

DanilaFe commented 6 days ago

The copy mutates pragma, which indicates that a type is changed when it's copied (e.g., copying a nilable owned resets the copied-from variable to nil). This pragma is propagated to records that contain owned fields, etc. This propagation continues even if normally-defined type (e.g., set in the standard library) defines a non-mutating copy constructor via init=. As a result, types like set are considered copy mutates and copying them requires a mutable variable.

Thus, whereas the following program works:

record R {
  var containedValue;
}

const x = new R(42);
var y = x;
writeln((x, y));

The following program does not:

use Set;

const x = new set(int);
var y = x;
writeln((x, y));

Printing the following error:

setcopy.chpl:1: In module 'setcopy':
setcopy.chpl:9: error: argument 1 for tuple construction is const but tuple construction takes ownership

Note that this is specifically caused by the fact that copying into the tuple would mutate the value (as far as the compiler believes), but this is not possible.

https://github.com/chapel-lang/chapel/blob/e1b1d03f487521432676709a755e3949f46717fd/compiler/resolution/lateConstCheck.cpp#L715-L721

The issue is that recordContainingCopyMutatesField incorrectly applies COPY_MUTATES even if a non-mutating init= is defined for the type.

https://github.com/chapel-lang/chapel/blob/e1b1d03f487521432676709a755e3949f46717fd/compiler/resolution/resolveFunction.cpp#L392

vasslitvinov commented 6 days ago

Here are my thoughts on this topic.

recordContainingCopyMutatesField should return a boolean indicating whether the record copy mutates a field. Instead of querying to hasFlag(FLAG_COPY_MUTATES), we should invoke recordContainingCopyMutatesField(). This is analogous to propagateNotPOD().

We have FLAG_TYPE_INIT_EQUAL_FROM_REF and FLAG_TYPE_INIT_EQUAL_FROM_CONST, which get calculated and set upon calling setRecordCopyableFlags(). Consider merging the former flag with FLAG_COPY_MUTATES.

mppf commented 6 days ago

I don't have a reproducer yet but I have been running into an issue that might be related with a record containing a type t; var field: t instantiated with nothing/none.

lydia-duncan commented 6 days ago

If I remember right, @dlongnecke-cray did some work to make sure collection types supported containing classes correctly and might have some insights into why things are the way they are, if you haven't checked with him already

vasslitvinov commented 5 days ago

Instantiating with none may cause problems regardless of this issue. I used to have a reproducer, not finding it right now.