summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorCopilot <198982749+Copilot@users.noreply.github.com>2025-08-07 01:01:02 -0700
committerGitHub <noreply@github.com>2025-08-07 08:01:02 +0000
commit9ed7b5264676547855e1378d2b9c9d51c8ba7162 (patch)
tree43eefcd986d1b73dc8d8a13358ce25b1764a2dac
parent063cbeaaea2fb00a10c6058ea4a9632092772ea5 (diff)
Add warning for comma operators used outside for-loops and expand expressions in legacy mode (#7984)
This PR implements a warning system to help users identify potentially unintended comma operator usage in expressions. The comma operator can be confusing when used in contexts like variable initialization where users might have intended to use braces for initialization instead. ## Problem The following code compiles without error but is likely not written as intended: ```slang float4 vColor = (0.f, 0.f, 0.f, 1.f); // Uses comma operators, evaluates to 1.f ``` The intended code should use braces: ```slang float4 vColor = {0.f, 0.f, 0.f, 1.f}; // Proper initialization ``` ## Solution Added a new warning diagnostic (`commaOperatorUsedInExpression`, ID: 41024) that warns when comma operators are used in expressions, with exemptions for contexts where they are commonly intended: - **For-loop side effects**: `for (int i = 0; i < 10; i++, x++)` - no warning - **Expand expressions**: `expand(f(), g(each param))` - no warning - **Slang 2026+ mode**: `let m = (1,2,3)` creates tuples - no warning - **All other expressions**: `float4 v = (a, b, c, d)` and `return a, b` - warns for each comma ## Implementation Details - Added context tracking in `SemanticsContext` with `m_inForLoopSideEffect` flag - Modified `visitForStmt` to use special context when checking side effect expressions - Added comma operator detection in `visitInvokeExpr` for `InfixExpr` nodes - Added language version check using `isSlang2026OrLater()` to disable warnings in Slang 2026+ mode where parentheses create tuples - Performance optimization: language version check is hoisted to avoid unnecessary casting - Warning can be suppressed using `-Wno-41024` command line flag ## Test Coverage Added comprehensive test cases using filecheck format that verify: - Warnings are generated for comma operators in variable initialization (legacy mode only) - Warnings are generated for comma operators in return statements (legacy mode only) - Warnings are generated for comma operators in general expressions (legacy mode only) - No warnings for comma operators in for-loop side effects - No warnings in Slang 2026+ mode where parentheses create tuples - Warning suppression works correctly Example output (legacy mode): ``` warning 41024: comma operator used in expression (may be unintended) float4 vColor = (0.f, 0.f, 0.f, 1.f); ^ warning 41024: comma operator used in expression (may be unintended) return a *= 2, a + 1; ^ ``` Fixes #6732. <!-- START COPILOT CODING AGENT TIPS --> --- 💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click [here](https://survey.alchemer.com/s3/8343779/Copilot-Coding-agent) to start the survey. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: aidanfnv <198290069+aidanfnv@users.noreply.github.com> Co-authored-by: slangbot <ellieh+slangbot@nvidia.com> Co-authored-by: slangbot <186143334+slangbot@users.noreply.github.com> Co-authored-by: aidanfnv <aidanf@nvidia.com> Co-authored-by: csyonghe <2652293+csyonghe@users.noreply.github.com>
-rw-r--r--source/slang/slang-check-expr.cpp34
-rw-r--r--source/slang/slang-check-impl.h17
-rw-r--r--source/slang/slang-check-stmt.cpp5
-rw-r--r--source/slang/slang-diagnostic-defs.h5
-rw-r--r--tests/bugs/gh-471.slang2
-rw-r--r--tests/compute/comma-operator.slang6
-rw-r--r--tests/compute/spirv-array-texel-pointer-atomic.slang4
-rw-r--r--tests/compute/spirv-multisampled-array-texel-pointer-atomic.slang4
-rw-r--r--tests/diagnostics/comma-operator-warning-slang2026.slang19
-rw-r--r--tests/diagnostics/comma-operator-warning.slang24
-rw-r--r--tests/hlsl-intrinsic/texture/partial-resident-texture-combined.slang4
-rw-r--r--tests/hlsl-intrinsic/texture/partial-resident-texture.slang4
12 files changed, 115 insertions, 13 deletions
diff --git a/source/slang/slang-check-expr.cpp b/source/slang/slang-check-expr.cpp
index d2f338e65..425548518 100644
--- a/source/slang/slang-check-expr.cpp
+++ b/source/slang/slang-check-expr.cpp
@@ -3229,6 +3229,40 @@ Expr* SemanticsExprVisitor::visitInvokeExpr(InvokeExpr* expr)
if (auto newExpr = convertToLogicOperatorExpr(expr))
return newExpr;
+ // Check for comma operator usage and emit warning if not in for-loop side effect context
+ // Skip warning in Slang 2026+ mode where parentheses create tuples
+ if (!isSlang2026OrLater(this))
+ {
+ bool exprIsInfixExpr = false;
+ InfixExpr* infixExpr = nullptr;
+ if (auto candidateInfixExpr = as<InfixExpr>(expr))
+ {
+ exprIsInfixExpr = true;
+ infixExpr = candidateInfixExpr;
+ }
+
+ bool functionExprIsVarExpr = false;
+ VarExpr* varExpr = nullptr;
+ if (exprIsInfixExpr)
+ {
+ if (auto candidateVarExpr = as<VarExpr>(infixExpr->functionExpr))
+ {
+ functionExprIsVarExpr = true;
+ varExpr = candidateVarExpr;
+ }
+ }
+
+ if (functionExprIsVarExpr && varExpr->name && varExpr->name->text == ",")
+ {
+ // Allow comma operators in for-loop side effects and expand expressions without
+ // warning
+ if (!getInForLoopSideEffect() && !m_parentExpandExpr)
+ {
+ getSink()->diagnose(infixExpr, Diagnostics::commaOperatorUsedInExpression);
+ }
+ }
+ }
+
expr->functionExpr = CheckTerm(expr->functionExpr);
if (auto baseType = as<DeclRefType>(expr->functionExpr->type))
diff --git a/source/slang/slang-check-impl.h b/source/slang/slang-check-impl.h
index 2675ecee0..48b06eb9f 100644
--- a/source/slang/slang-check-impl.h
+++ b/source/slang/slang-check-impl.h
@@ -1069,6 +1069,18 @@ public:
return result;
}
+ // Setup the flag to indicate we're in a for-loop side effect context where comma operators are
+ // allowed
+ SemanticsContext withInForLoopSideEffect()
+ {
+ SemanticsContext result(*this);
+ result.m_inForLoopSideEffect = true;
+ return result;
+ }
+
+ bool getInForLoopSideEffect() { return m_inForLoopSideEffect; }
+
+
TryClauseType getEnclosingTryClauseType() { return m_enclosingTryClauseType; }
SemanticsContext withEnclosingTryClauseType(TryClauseType tryClauseType)
@@ -1202,6 +1214,11 @@ protected:
// 3. the logic expression is in an array size declaration.
bool m_shouldShortCircuitLogicExpr = true;
+ // Flag to track when we're in a for-loop side effect expression where comma operators are
+ // allowed
+ bool m_inForLoopSideEffect = false;
+
+
ExpandExpr* m_parentExpandExpr = nullptr;
OrderedHashSet<Type*>* m_capturedTypePacks = nullptr;
diff --git a/source/slang/slang-check-stmt.cpp b/source/slang/slang-check-stmt.cpp
index bf8ddc94e..8c9e24d48 100644
--- a/source/slang/slang-check-stmt.cpp
+++ b/source/slang/slang-check-stmt.cpp
@@ -281,7 +281,10 @@ void SemanticsStmtVisitor::visitForStmt(ForStmt* stmt)
}
if (stmt->sideEffectExpression)
{
- stmt->sideEffectExpression = CheckExpr(stmt->sideEffectExpression);
+ // Use special context for for-loop side effect to allow comma operators without warning
+ SemanticsContext sideEffectContext = withInForLoopSideEffect();
+ SemanticsExprVisitor subExprVisitor(sideEffectContext);
+ stmt->sideEffectExpression = subExprVisitor.CheckExpr(stmt->sideEffectExpression);
}
subContext.checkStmt(stmt->statement);
diff --git a/source/slang/slang-diagnostic-defs.h b/source/slang/slang-diagnostic-defs.h
index e9c8acfe3..284cb634c 100644
--- a/source/slang/slang-diagnostic-defs.h
+++ b/source/slang/slang-diagnostic-defs.h
@@ -2512,6 +2512,11 @@ DIAGNOSTIC(
Warning,
methodNeverMutates,
"method marked `[mutable]` but never modifies `this`")
+DIAGNOSTIC(
+ 41024,
+ Warning,
+ commaOperatorUsedInExpression,
+ "comma operator used in expression (may be unintended)")
DIAGNOSTIC(
41024,
diff --git a/tests/bugs/gh-471.slang b/tests/bugs/gh-471.slang
index d59f97281..e48c1da5a 100644
--- a/tests/bugs/gh-471.slang
+++ b/tests/bugs/gh-471.slang
@@ -1,4 +1,4 @@
-//TEST(compute):COMPARE_COMPUTE: -shaderobj
+//TEST(compute):COMPARE_COMPUTE: -shaderobj -xslang -Wno-41024
//TEST_INPUT:ubuffer(data=[0 1 2 3], stride=4):out,name outputBuffer
// Test that "operator comma" works as expected
diff --git a/tests/compute/comma-operator.slang b/tests/compute/comma-operator.slang
index c6cecf1c7..917484d00 100644
--- a/tests/compute/comma-operator.slang
+++ b/tests/compute/comma-operator.slang
@@ -1,7 +1,7 @@
// comma-operator.slang
-//TEST(compute):COMPARE_COMPUTE:-cpu -shaderobj
-//TEST(compute):COMPARE_COMPUTE: -shaderobj
+//TEST(compute):COMPARE_COMPUTE:-cpu -shaderobj -xslang -Wno-41024
+//TEST(compute):COMPARE_COMPUTE: -shaderobj -xslang -Wno-41024
// Test that the "comma operator" behaves as expected
@@ -9,7 +9,7 @@
// confirm that the generated SPIR-V (and by extension DXIL/DXBC)
// doesn't include a subroutine defintion/call for `operator,`
-//TEST:CROSS_COMPILE:-target spirv-assembly -entry computeMain -stage compute
+//TEST:CROSS_COMPILE:-target spirv-assembly -entry computeMain -stage compute -Wno-41024
int test(int inVal)
{
diff --git a/tests/compute/spirv-array-texel-pointer-atomic.slang b/tests/compute/spirv-array-texel-pointer-atomic.slang
index a0013e284..41ee27dfb 100644
--- a/tests/compute/spirv-array-texel-pointer-atomic.slang
+++ b/tests/compute/spirv-array-texel-pointer-atomic.slang
@@ -1,5 +1,5 @@
-//TEST:SIMPLE(filecheck=CHECK): -target spirv -entry computeMain -stage compute
-//TEST(compute):COMPARE_COMPUTE(filecheck-buffer=BUF): -slang -output-using-type -shaderobj -vk
+//TEST:SIMPLE(filecheck=CHECK): -target spirv -entry computeMain -stage compute -Wno-41024
+//TEST(compute):COMPARE_COMPUTE(filecheck-buffer=BUF): -slang -output-using-type -shaderobj -vk -xslang -Wno-41024
//TEST_INPUT: RWTexture2D(format=R32Uint, size=4, content = zero, arrayLength=2, mipMaps = 1):name rwTexture2DArray
[[vk::binding(2)]]
diff --git a/tests/compute/spirv-multisampled-array-texel-pointer-atomic.slang b/tests/compute/spirv-multisampled-array-texel-pointer-atomic.slang
index 48b420b38..d4c90e051 100644
--- a/tests/compute/spirv-multisampled-array-texel-pointer-atomic.slang
+++ b/tests/compute/spirv-multisampled-array-texel-pointer-atomic.slang
@@ -1,5 +1,5 @@
-//TEST:SIMPLE(filecheck=CHECK): -target spirv -entry computeMain -stage compute
-//TEST(compute):COMPARE_COMPUTE(filecheck-buffer=BUF): -slang -output-using-type -shaderobj -vk
+//TEST:SIMPLE(filecheck=CHECK): -target spirv -entry computeMain -stage compute -Wno-41024
+//TEST(compute):COMPARE_COMPUTE(filecheck-buffer=BUF): -slang -output-using-type -shaderobj -vk -xslang -Wno-41024
//DISABLE_TEST(cuda):COMPARE_COMPUTE_EX:-cuda -compute -shaderobj -output-using-type
//TEST_INPUT: RWTexture2D(format=R32Uint, size=4, content = zero, arrayLength=2, sampleCount=two, mipMaps = 1):name rwTexture2DMSArray
diff --git a/tests/diagnostics/comma-operator-warning-slang2026.slang b/tests/diagnostics/comma-operator-warning-slang2026.slang
new file mode 100644
index 000000000..e91f540d4
--- /dev/null
+++ b/tests/diagnostics/comma-operator-warning-slang2026.slang
@@ -0,0 +1,19 @@
+//DIAGNOSTIC_TEST:SIMPLE:
+
+#language 2026
+
+int testCommaOperatorInSlang2026()
+{
+ // In Slang 2026, this creates a tuple - should NOT generate a warning
+ let m = (1, 2, 3);
+
+ // Test accessing tuple elements
+ let x = m._0;
+ let y = m._1;
+ let z = m._2;
+
+ // Another tuple example - should NOT generate a warning
+ var t = (1.0f, 2.0f, 3.0f);
+
+ return x + y + z;
+} \ No newline at end of file
diff --git a/tests/diagnostics/comma-operator-warning.slang b/tests/diagnostics/comma-operator-warning.slang
new file mode 100644
index 000000000..945cc792d
--- /dev/null
+++ b/tests/diagnostics/comma-operator-warning.slang
@@ -0,0 +1,24 @@
+//DIAGNOSTIC_TEST:SIMPLE(filecheck=CHECK):
+
+int testCommaOperator()
+{
+ // This should generate a warning - comma operator in variable initialization
+ // CHECK: ([[# @LINE+1]]): warning 41024:
+ float4 vColor = (0.f, 0.f, 0.f, 1.f);
+
+ // This should NOT generate a warning - comma operator in for-loop side effect
+ for (int i = 0; i < 10; i++, vColor.x++)
+ {
+ }
+
+ // This should generate a warning - comma operator in regular expression
+ // CHECK: ([[# @LINE+1]]): warning 41024:
+ int x = (1, 2, 3);
+
+ // This should now generate a warning - comma operator in return statement
+ // CHECK: ([[# @LINE+2]]): warning 41024:
+ int a = 5;
+ return a *= 2, a + 1;
+}
+
+void someFunction(int value) {} \ No newline at end of file
diff --git a/tests/hlsl-intrinsic/texture/partial-resident-texture-combined.slang b/tests/hlsl-intrinsic/texture/partial-resident-texture-combined.slang
index 2281118b7..4b2f41a15 100644
--- a/tests/hlsl-intrinsic/texture/partial-resident-texture-combined.slang
+++ b/tests/hlsl-intrinsic/texture/partial-resident-texture-combined.slang
@@ -1,7 +1,7 @@
// TODO: There are issues running tests combined texture samplers in DX12. (Github issue #6982)
-//DISABLE_TEST(compute):COMPARE_COMPUTE_EX(filecheck-buffer=CHK):-slang -compute -shaderobj -output-using-type -use-dxil -profile cs_6_7 -dx12 -Xslang -DTARGET_DX12
+//DISABLE_TEST(compute):COMPARE_COMPUTE_EX(filecheck-buffer=CHK):-slang -compute -shaderobj -output-using-type -use-dxil -profile cs_6_7 -dx12 -Xslang -DTARGET_DX12 -xslang -Wno-41024
-//TEST(compute):COMPARE_COMPUTE_EX(filecheck-buffer=CHK):-vk -compute -shaderobj -output-using-type -emit-spirv-directly -render-feature hardware-device -xslang -DVK
+//TEST(compute):COMPARE_COMPUTE_EX(filecheck-buffer=CHK):-vk -compute -shaderobj -output-using-type -emit-spirv-directly -render-feature hardware-device -xslang -DVK -xslang -Wno-41024
//TEST_INPUT: ubuffer(data=[2], stride=4):out,name outputBuffer
RWStructuredBuffer<int> outputBuffer;
diff --git a/tests/hlsl-intrinsic/texture/partial-resident-texture.slang b/tests/hlsl-intrinsic/texture/partial-resident-texture.slang
index 2997463af..bcc011679 100644
--- a/tests/hlsl-intrinsic/texture/partial-resident-texture.slang
+++ b/tests/hlsl-intrinsic/texture/partial-resident-texture.slang
@@ -1,5 +1,5 @@
-//TEST(compute):COMPARE_COMPUTE_EX(filecheck-buffer=CHK):-slang -compute -shaderobj -output-using-type -use-dxil -profile cs_6_7 -dx12
-//TEST(compute):COMPARE_COMPUTE_EX(filecheck-buffer=CHK):-vk -compute -shaderobj -output-using-type -emit-spirv-directly -render-feature hardware-device -xslang -DVK
+//TEST(compute):COMPARE_COMPUTE_EX(filecheck-buffer=CHK):-slang -compute -shaderobj -output-using-type -use-dxil -profile cs_6_7 -dx12 -xslang -Wno-41024
+//TEST(compute):COMPARE_COMPUTE_EX(filecheck-buffer=CHK):-vk -compute -shaderobj -output-using-type -emit-spirv-directly -render-feature hardware-device -xslang -DVK -xslang -Wno-41024
//TEST_INPUT: ubuffer(data=[2], stride=4):out,name outputBuffer
RWStructuredBuffer<int> outputBuffer;