Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
DanilaFe opened this issue Nov 13, 2024 · 4 comments
Open

Comments

@DanilaFe
Copy link
Contributor

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.

if (actual->qualType().isConst() &&
actual->getValType()->symbol->hasFlag(FLAG_COPY_MUTATES)) {
USR_FATAL_CONT(actual,
"argument %i for tuple construction is const "
"but tuple construction takes ownership",
formalIdx
);

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

bool recordContainingCopyMutatesField(Type* t) {

@DanilaFe DanilaFe changed the title Compiler: the copy mutates pragma is set too aggressively Compiler considers some immutably-copyable types non-immutably-copyable Nov 13, 2024
@vasslitvinov
Copy link
Member

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
Copy link
Member

mppf commented Nov 14, 2024

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
Copy link
Member

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants