[Bug]: support for associative array literals with owned classes depended on violating const checks for varargs #26035
Labels
Comments
lydia-duncan
added a commit
that referenced
this issue
Oct 9, 2024
[reviewed by @mppf] Resolves #25901 Modifies the same place as the PRs for const and const in, but additionally needed a fix for `moveSetConstFlagsAndCheck` because the local tuple variable was using a `PRIM_GET_MEMBER_VALUE` instead of a `PRIM_GET_MEMBER` (though only in the case where it returns a ref). Also updates a couple of tests that were impacted by the change: - The varargs array test could reasonably be considered fulfilling the promise of the deprecation message it encountered previously. - Futurizes the DataFrame test. We believe it to be okay because the failure mode is accurate (we think `chpl__buildAssociativeArrayExpr` should still be generally using `const` for its varargs, and copying an owned class does modify the original) and was not caught before this change. Add test locking in that there's an alternate way to create an associative array of owned classes that doesn't use the associative array literal syntax. I also opened #26035 to track the future Add tests locking in the fix for const ref. Covers both queried and unqueried numbers of arguments.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Summary of Problem
Description:
When #25902 gets merged, it will improve const and const ref checking for varargs. As a result, trying to create a default associative array literal with owned classes will trigger constness errors, because
chpl__buildAssociativeArrayExprtakes const varargs but tries to copy the arguments (and copying an owned object modifies the object, making it not a const operation).Consulting with Michael, it seems like we probably want
chpl__buildAssociativeArrayExprto continue taking its arguments by const for the most part. It may make sense to carve out a special case for handling owned classes, but we were concerned that would be too much to handle at the moment, so decided to futurize the test that was triggering the error for now until resolving it becomes a priority (e.g. if a user runs into it).Is this issue currently blocking your progress?
no, this can be worked around by explicitly declaring the domain for the associative array ahead of time and populating the contents of the array after its initial declaration (see HelloDataFrame-createDomFirst.chpl, which will be added by #25902)
Steps to Reproduce
Associated Future Test(s):
test/library/draft/DataFrames/psahabu/HelloDataFrame.chpl#25902 (modified)Configuration Information
chpl --version: chapel 2.3 (pre-release)$CHPL_HOME/util/printchplenv --anonymize:gcc --versionorclang --version: N/Amodule list: N/AThe text was updated successfully, but these errors were encountered: