Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix yielding objects from an iterator record whose copy mutates them (#…
…26230) Closes #11299. Closes #14246. Thank you @vasslitvinov for the initial investigation and for suggesting the direction. Specifically, this PR introduces loop pragmas and uses them to make certain variables mutable, addressing the constness errors. This PR fixes an issue in which yielding things-that-contain-`owned` (but not `owned` by itself) caused constness errors in the compiler, with the following rough shape. E.g., in the following program: ```Chapel iter asdf() do yield (new owned RootClass(),); var fdsa = asdf(); compilerWarning(fdsa.type:string); writeln(fdsa); ``` Produced the following error prior to this PR: ``` $CHPL_HOME/modules/internal/ChapelArray.chpl:3573: In function 'chpl__initCopy': $CHPL_HOME/modules/internal/ChapelArray.chpl:3611: error: const actual is passed to 'ref' formal 'x' of chpl__initCopy() vasstmp.chpl:2: called as chpl__initCopy(ir: _ir_asdf, definedConst: bool) note: generic instantiations are underlined in the above callstack vasstmp.chpl:1: In module 'vasstmp': vasstmp.chpl:3: note: [domain(1,int(64),one)] 1*owned RootClass ``` Reviewed by @jabraham17 (parser) and @vasslitvinov (the rest) -- thanks! ## Background The problem is caused by the fact that copying of elements of types like `owned C?` mutates the element. This happens because the value (pointer) gets _moved_ into the new copy of the smart pointer, resetting the old copy to `nil`. ```Chapel var x = new C?(); var y = x; // fine, but x is now nil ``` This, in turn, means that copying `x` requires `x` to be mutable. ```Chapel const x = new C?(); var y = x; // not fine, constness error: const x is passed to ref formal of init= ``` This is all fine and good. However, another component of the problem is that Chapel by default makes loop indices constant. ```Chapel for x in someIterator() { // x is 'const' var y = x; // would not be fine but there's a compiler hack; see the following paragraph } ``` As you may read in the above comment, the code above ought not to work for the same reason as the `const x` snippet: copying `x` would mutate it, but `x` is constant. However, the Chapel compiler actually explicitly overrides the constness rule for a restricted set of types (those with the `FLAG_COPY_MUTATES` flag set): https://github.com/chapel-lang/chapel/blob/ed9522e3cdbf9aab881b0602dcb2d070ff650079/compiler/resolution/functionResolution.cpp#L9474-L9476 `FLAG_COPY_MUTATES` is supposed to apply to all types which get mutated by `init=` (which includes `owned`, but also any record that has a default `init=` and an `owned` __field__, since copying such a record would also copy, and mutate, its fields). However, at this point, `FLAG_COPY_MUTATES` is not propagated, so _only `owned`_ is affected by this hack. Other types that are mutated by copying issue constness errors here. It's possible to change this logic to ensure that `FLAG_COPY_MUTATES` is propagated to other types. However, this would constitute a significant semantic change, and make loop indices of a large variety of types (anything with an `owned` field) mutable when iterated over. This is undesirable; moreover, the current computation that propagates `FLAG_COPY_MUTATES` is incorrect / overly aggressive. For instance, `set` is marked `FLAG_COPY_MUTATES`, even though it supports a non-mutating copy constructor. This is another reason to avoid leaning on the flag. Instead, we observe that it's sound to leave variables yielded by value mutable. Though we don't want to change user-facing behavior (which until now has kept loop indices constant), we would like to be able to opt into it for copying functions in `ChapelArray`, which which emit the constness errors this PR aims to resolve. if the index variables of the loops that back the `ChapelArray` functions are mutable, they can be safely copied into the array-being-initialized, mutating the (temporary and subsequently unused) index variable. ## Implementation This PR achieves mutable loop index variables by adding support for loop flags (`#pragma "bla" for ...`). To this, the PR: * Introduces two new flags, `loop indices mutable` and `loop index mutable`. The former is applied to loops; however, since in the production compiler pragmas can only be applied to symbols, the latter is applied to each index variable. Index variables marked 'loop index mutable' do not get set to `const`. * Applies the `loop indices mutable` pragma to affected functions in `ChapelArray`. This fixes the constness errors when copying out of these indices into a new array. * Changes loop attribute group parsing to consume pragmas even if attributes were not encountered. This enables full-fledged loop pragmas and resolve an inconsistency (pragmas would be consumed if attributes were present, but left alone otherwise). * Resolves a parser state bug present on `main`, but triggered more often under the changes from the previous bullet. Prior to this PR, pragmas for variable declarations were collected __after__ parsing the entire variable declaration. This meant they remained in parser state throughout parsing the initialization expression. As a result, any statement contained therein (e.g. via FCF, lambda, etc.) would have these pragmas applied to it instead. Since loop expressions now also trigger this behavior, the following code was incorrectly parsed: ```Chapel pragma "bla" var x = for i in 1..10 do i; ``` * To fix this, this PR changes several productions in the parser to eagerly consume attribute groups (before moving on to the type expression). This matches several other productions already in place (forwarding declarations, procedures, etc.). ## Testing - [x] paratest
- Loading branch information
