From 19107e43a3091bdd6d003124355052ab5659b76b Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Fri, 6 Sep 2024 14:39:38 -0700 Subject: [PATCH 1/5] Prevent modification of const ref varargs, too 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`. We'll see if that has broader consequences, though. ---- Signed-off-by: Lydia Duncan --- compiler/resolution/expandVarArgs.cpp | 2 +- compiler/resolution/functionResolution.cpp | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/compiler/resolution/expandVarArgs.cpp b/compiler/resolution/expandVarArgs.cpp index 5c2a0ef40d16..21963ce4ba6d 100644 --- a/compiler/resolution/expandVarArgs.cpp +++ b/compiler/resolution/expandVarArgs.cpp @@ -474,7 +474,7 @@ static void expandVarArgsBody(FnSymbol* fn, } if (formal->intent == INTENT_CONST || formal->intent == INTENT_CONST_IN || - formal->intent == INTENT_BLANK) { + formal->intent == INTENT_CONST_REF || formal->intent == INTENT_BLANK) { // TODO: Note that this will be overly strict for arrays, syncs, and // atomics, since their default is "pass-by-ref". However, we think this is // okay for now, in part due to thinking it's unlikely to be relied upon and diff --git a/compiler/resolution/functionResolution.cpp b/compiler/resolution/functionResolution.cpp index a6ce2f914d5a..6f83cd99ac40 100644 --- a/compiler/resolution/functionResolution.cpp +++ b/compiler/resolution/functionResolution.cpp @@ -9599,6 +9599,7 @@ static void resolveMoveForRhsCallExpr(CallExpr* call, Type* rhsType) { static void moveSetConstFlagsAndCheck(CallExpr* call, CallExpr* rhs) { if (rhs->isPrimitive(PRIM_GET_MEMBER) || + rhs->isPrimitive(PRIM_GET_MEMBER_VALUE) || rhs->isPrimitive(PRIM_ADDR_OF)) { if (SymExpr* rhsBase = toSymExpr(rhs->get(1))) { From f1993067cff24d38e86dae4f338b2f18583f0e99 Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Fri, 6 Sep 2024 14:43:13 -0700 Subject: [PATCH 2/5] Add tests locking in the fix for const ref Covers both queried and unqueried numbers of arguments ---- Signed-off-by: Lydia Duncan --- test/functions/varargs/constRefVarargs.chpl | 12 ++++++++++++ test/functions/varargs/constRefVarargs.good | 3 +++ test/functions/varargs/constRefVarargsQuery.chpl | 12 ++++++++++++ test/functions/varargs/constRefVarargsQuery.good | 3 +++ 4 files changed, 30 insertions(+) create mode 100644 test/functions/varargs/constRefVarargs.chpl create mode 100644 test/functions/varargs/constRefVarargs.good create mode 100644 test/functions/varargs/constRefVarargsQuery.chpl create mode 100644 test/functions/varargs/constRefVarargsQuery.good diff --git a/test/functions/varargs/constRefVarargs.chpl b/test/functions/varargs/constRefVarargs.chpl new file mode 100644 index 000000000000..63fe9cbb572c --- /dev/null +++ b/test/functions/varargs/constRefVarargs.chpl @@ -0,0 +1,12 @@ +// Taken from https://github.com/chapel-lang/chapel/issues/25858 +proc myPrintln(const ref args...) +{ + writeln("args.type = ", args.type:string); + writeln("args (before) = ", args); + + args[0] *= 10; // should not be allowed + + writeln("args (after) = ", args); +} + +myPrintln(1, 2.3, "four"); diff --git a/test/functions/varargs/constRefVarargs.good b/test/functions/varargs/constRefVarargs.good new file mode 100644 index 000000000000..bf0e7ebb8dc0 --- /dev/null +++ b/test/functions/varargs/constRefVarargs.good @@ -0,0 +1,3 @@ +constRefVarargs.chpl:2: In function 'myPrintln': +constRefVarargs.chpl:7: error: cannot assign to const variable + constRefVarargs.chpl:12: called as myPrintln(args(0): int(64), args(1): real(64), args(2): string) diff --git a/test/functions/varargs/constRefVarargsQuery.chpl b/test/functions/varargs/constRefVarargsQuery.chpl new file mode 100644 index 000000000000..900601268ed4 --- /dev/null +++ b/test/functions/varargs/constRefVarargsQuery.chpl @@ -0,0 +1,12 @@ +// Taken from https://github.com/chapel-lang/chapel/issues/25858, modified +proc myPrintln(const ref args...?k) +{ + writeln("args.type = ", args.type:string); + writeln("args (before) = ", args); + + args[0] *= 10; // should not be allowed + + writeln("args (after) = ", args); +} + +myPrintln(1, 2.3, "four"); diff --git a/test/functions/varargs/constRefVarargsQuery.good b/test/functions/varargs/constRefVarargsQuery.good new file mode 100644 index 000000000000..8ea6b38b7ef3 --- /dev/null +++ b/test/functions/varargs/constRefVarargsQuery.good @@ -0,0 +1,3 @@ +constRefVarargsQuery.chpl:2: In function 'myPrintln': +constRefVarargsQuery.chpl:7: error: cannot assign to const variable + constRefVarargsQuery.chpl:12: called as myPrintln(args(0): int(64), args(1): real(64), args(2): string) From 79afb8dc2977c521b48bfa7d681b263e9fd07e35 Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Thu, 3 Oct 2024 11:49:46 -0700 Subject: [PATCH 3/5] Futurize the DataFrames test that failed and add an alternative that works This test failed due to us now (accurately) detecting that `chpl__buildAssociativeArrayExpr` violates the const-ness of the varargs for owned arguments. In talking with Michael, we decided it was okay for this to get futurized as a result, and added a version of the test that does not fail, showing that you can still create associative arrays that store owned classes (just not using associative array literals) ---- Signed-off-by: Lydia Duncan --- .../HelloDataFrame-createDomFirst.chpl | 22 +++++++++++++++++++ .../HelloDataFrame-createDomFirst.compopts | 1 + .../HelloDataFrame-createDomFirst.good | 20 +++++++++++++++++ .../psahabu/HelloDataFrame-createDomFirst.py | 21 ++++++++++++++++++ .../DataFrames/psahabu/HelloDataFrame.bad | 3 +++ .../DataFrames/psahabu/HelloDataFrame.future | 2 ++ 6 files changed, 69 insertions(+) create mode 100644 test/library/draft/DataFrames/psahabu/HelloDataFrame-createDomFirst.chpl create mode 100644 test/library/draft/DataFrames/psahabu/HelloDataFrame-createDomFirst.compopts create mode 100644 test/library/draft/DataFrames/psahabu/HelloDataFrame-createDomFirst.good create mode 100644 test/library/draft/DataFrames/psahabu/HelloDataFrame-createDomFirst.py create mode 100644 test/library/draft/DataFrames/psahabu/HelloDataFrame.bad create mode 100644 test/library/draft/DataFrames/psahabu/HelloDataFrame.future diff --git a/test/library/draft/DataFrames/psahabu/HelloDataFrame-createDomFirst.chpl b/test/library/draft/DataFrames/psahabu/HelloDataFrame-createDomFirst.chpl new file mode 100644 index 000000000000..765206323fb6 --- /dev/null +++ b/test/library/draft/DataFrames/psahabu/HelloDataFrame-createDomFirst.chpl @@ -0,0 +1,22 @@ +use DataFrames; + +var validBits = [true, false, true, false, true]; + +var columnOne: owned Series? = new owned TypedSeries(["a", "b", "c", "d", "e"], validBits); +var columnTwo: owned Series? = new owned TypedSeries([1, 2, 3, 4, 5], validBits); +var columnThree: owned Series? = new owned TypedSeries([10.0, 20.0, 30.0, 40.0, 50.0]); + +var dom = {"columnOne", "columnTwo", "columnThree"}; +var columns: [dom] owned Series?; +columns["columnOne"] = columnOne; +columns["columnTwo"] = columnTwo; +columns["columnThree"] = columnThree; +var idx = new shared TypedIndex(["rowOne", "rowTwo", "rowThree", "rowFour", "rowFive"]); + +var dataFrame = new owned DataFrame(columns, idx); +var noIndex = new owned DataFrame(columns); +writeln(dataFrame); +writeln(); +writeln(dataFrame["columnThree"]); +writeln(); +writeln(noIndex); diff --git a/test/library/draft/DataFrames/psahabu/HelloDataFrame-createDomFirst.compopts b/test/library/draft/DataFrames/psahabu/HelloDataFrame-createDomFirst.compopts new file mode 100644 index 000000000000..ab90c63f3ded --- /dev/null +++ b/test/library/draft/DataFrames/psahabu/HelloDataFrame-createDomFirst.compopts @@ -0,0 +1 @@ +-snoParSafeWarning diff --git a/test/library/draft/DataFrames/psahabu/HelloDataFrame-createDomFirst.good b/test/library/draft/DataFrames/psahabu/HelloDataFrame-createDomFirst.good new file mode 100644 index 000000000000..0cfb197d2979 --- /dev/null +++ b/test/library/draft/DataFrames/psahabu/HelloDataFrame-createDomFirst.good @@ -0,0 +1,20 @@ + columnOne columnThree columnTwo +rowOne a 10.0 1 +rowTwo None 20.0 None +rowThree c 30.0 3 +rowFour None 40.0 None +rowFive e 50.0 5 + +rowOne 10.0 +rowTwo 20.0 +rowThree 30.0 +rowFour 40.0 +rowFive 50.0 +dtype: real(64) + + columnOne columnThree columnTwo +0 a 10.0 1 +1 None 20.0 None +2 c 30.0 3 +3 None 40.0 None +4 e 50.0 5 diff --git a/test/library/draft/DataFrames/psahabu/HelloDataFrame-createDomFirst.py b/test/library/draft/DataFrames/psahabu/HelloDataFrame-createDomFirst.py new file mode 100644 index 000000000000..b13292b678a9 --- /dev/null +++ b/test/library/draft/DataFrames/psahabu/HelloDataFrame-createDomFirst.py @@ -0,0 +1,21 @@ +import pandas as pd + +columnOne = ["a", None, "c", None, "e"] +columnTwo = [1, None, 3, None, 5] +columnThree = [10.0, 20.0, 30.0, 40.0, 50.0] + +idx = pd.Index(["rowOne", "rowTwo", "rowThree", "rowFour", "rowFive"]) +dataFrame = pd.DataFrame({ "columnOne": columnOne, + "columnTwo": columnTwo, + "columnThree": columnThree }, + idx) +noIndex = pd.DataFrame({ "columnOne": columnOne, + "columnTwo": columnTwo, + "columnThree": columnThree }) + + +print dataFrame +print +print dataFrame["columnThree"] +print +print noIndex diff --git a/test/library/draft/DataFrames/psahabu/HelloDataFrame.bad b/test/library/draft/DataFrames/psahabu/HelloDataFrame.bad new file mode 100644 index 000000000000..3ab33ac3fa28 --- /dev/null +++ b/test/library/draft/DataFrames/psahabu/HelloDataFrame.bad @@ -0,0 +1,3 @@ +HelloDataFrame.chpl:9: error: const actual is passed to a 'ref' formal of init=() +HelloDataFrame.chpl:9: error: const actual is passed to a 'ref' formal of init=() +HelloDataFrame.chpl:9: error: const actual is passed to a 'ref' formal of init=() diff --git a/test/library/draft/DataFrames/psahabu/HelloDataFrame.future b/test/library/draft/DataFrames/psahabu/HelloDataFrame.future new file mode 100644 index 000000000000..71e0e8881ec3 --- /dev/null +++ b/test/library/draft/DataFrames/psahabu/HelloDataFrame.future @@ -0,0 +1,2 @@ +bug: owned objects are supported with associative array literals +#26035 From acae53e6d4cbe333622ad067954166f3353d2acf Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Thu, 3 Oct 2024 13:17:59 -0700 Subject: [PATCH 4/5] Update behavior of test that was added by a separate PR that has since merged This is basically the same thing as removing the deprecated behavior (for the purpose of this particular test, we need to perform the behavior update more generally still), so I don't think the change is concerning ---- Signed-off-by: Lydia Duncan --- test/functions/varargs/varargsModArray.good | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/test/functions/varargs/varargsModArray.good b/test/functions/varargs/varargsModArray.good index 034e4107bec4..7f694aa09881 100644 --- a/test/functions/varargs/varargsModArray.good +++ b/test/functions/varargs/varargsModArray.good @@ -1,4 +1,3 @@ -varargsModArray.chpl:2: warning: inferring a default intent to be 'ref' is deprecated - please use an explicit 'ref' intent for the argument 'args' -args.type = ([domain(1,int(64),one)] int(64),real(64),string) -args (before) = (1 2 3, 2.3, four) -args (after) = (10 2 3, 2.3, four) +varargsModArray.chpl:2: In function 'myPrintln': +varargsModArray.chpl:7: error: cannot assign to const variable + varargsModArray.chpl:13: called as myPrintln(args(0): [domain(1,int(64),one)] int(64), args(1): real(64), args(2): string) From 6aeca91a990e3f41aa07d6f84162508d42b9b56d Mon Sep 17 00:00:00 2001 From: Lydia Duncan Date: Wed, 9 Oct 2024 12:56:50 -0700 Subject: [PATCH 5/5] At Michael's suggestion, limit the application to `PRIM_GET_MEMBER_VALUE` Michael pointed out that `PRIM_GET_MEMBER_VALUE` can situationally return a ref or a value and we don't want to apply the const-ness in the value case (as it will be a copy and thus independent of the behavior of the `const` source). So ensure that the result of the call will be a ref before propagating the `const`. This passed the normally impacted tests locally, will run a full paratest as well ---- Signed-off-by: Lydia Duncan --- compiler/resolution/functionResolution.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/resolution/functionResolution.cpp b/compiler/resolution/functionResolution.cpp index 6f83cd99ac40..5a6b3bb68d9a 100644 --- a/compiler/resolution/functionResolution.cpp +++ b/compiler/resolution/functionResolution.cpp @@ -9599,7 +9599,7 @@ static void resolveMoveForRhsCallExpr(CallExpr* call, Type* rhsType) { static void moveSetConstFlagsAndCheck(CallExpr* call, CallExpr* rhs) { if (rhs->isPrimitive(PRIM_GET_MEMBER) || - rhs->isPrimitive(PRIM_GET_MEMBER_VALUE) || + (rhs->isPrimitive(PRIM_GET_MEMBER_VALUE) && rhs->qualType().isRef()) || rhs->isPrimitive(PRIM_ADDR_OF)) { if (SymExpr* rhsBase = toSymExpr(rhs->get(1))) {