Add warning on mutation of function parameter (#3067)

By default, function parameters in HLSL are mutable, but any changes to a parameter do not affect the values of the arguments after a call:

    void f(int a)
    {
        a++; // allowed, but kind of useless
    }
    ...
    int b = 0;
    f(b);
    // b is still zero

Because the above behavior is a part of HLSL, we cannot easily diagnose such cases as errors without breaking backward compatibility with existing code.

This change makes it an error to invoke a `[mutating]` method on a function parameter, which cannot affect backward compatibility since the notion of `[mutating]` methods is not present in existing HLSL code:

    struct Counter
    {
        int _state;
        [mutating] void increment() { _state++; }
    }
    void f(Counter a)
    {
        a.increment(); // ERROR
    }
    ...
    Counter b = { 0 };
    f(b);
    // b is still zero

The compiler will also diagnose calls to `[mutating]` methods on a field or array element extracted out of a function parameter.

This change does not affect code that directly mutates a function parameter via assignment, or via passing the parameter onward as an argument to an `out` or `inout` call (or, equivalently, as the left-hand operand to a compound assignment operator).

This is a breaking change to existing Slang code, since it could diagnose an error on code that used to be allowed.
Indeed, two tests in the Slang test suite had to be updated to avoid such errors.
It would be possible to turn this diagnostic into a warning, and simply encourage users to enable it as an error.
On balance, though, it seems best to not allow this idiom since it has such a high probability to be an error.

Note: the specific case that motivated this change is use of `RayQuery` values as function parameters.
The root of the problem there is that dxc treats `RayQuery` values as copyable handles to mutable state, while Slang prefers to capture the mutation that occurs through marking the appropriate methods as `[mutating]`.
The Slang approach makes portable codegen for D3D/Vulkan simpler, but requires that we *also* treat a type like `RayQuery` as non-copyable.

This change does not address the problem that the Slang compiler does not enforce the requirement that values of non-copyable types do not get copied.
Instead, the diagnostic here just happens to issue a diagnostic in one important case where a copy would typically occur.

Co-authored-by: Yong He <yonghe@outlook.com>

5 files changed, 86 insertions, 2 deletions

diff --git a/source/slang/slang-check-impl.h b/source/slang/slang-check-impl.h
index 46cc329a9..88f1e2975 100644
--- a/source/slang/slang-check-impl.h
+++ b/source/slang/slang-check-impl.h
@@ -2031,6 +2031,16 @@ namespace Slang
             OverloadResolveContext&		/*context*/,
             OverloadCandidate const&	/*candidate*/);
 
+            /// Check if the given `expr` refers to an `in` function
+            /// parameter, or part of one (through field reference, etc.).
+            ///
+            /// If the expression refers into a parameter, returns
+            /// the declaration of the parameter. Otherwise returns
+            /// null.
+            ///
+        ParamDecl* isReferenceIntoFunctionInputParameter(
+            Expr* expr);
+
         // Create a witness that attests to the fact that `type`
         // is equal to itself.
         TypeEqualityWitness* createTypeEqualityWitness(
diff --git a/source/slang/slang-check-overload.cpp b/source/slang/slang-check-overload.cpp
index 8709ae763..c13e74f69 100644
--- a/source/slang/slang-check-overload.cpp
+++ b/source/slang/slang-check-overload.cpp
@@ -488,6 +488,58 @@ namespace Slang
         return false;
     }
 
+    ParamDecl* SemanticsVisitor::isReferenceIntoFunctionInputParameter(
+        Expr* inExpr)
+    {
+        auto expr = inExpr;
+        for (;;)
+        {
+            if (auto declRefExpr = as<DeclRefExpr>(expr))
+            {
+                auto declRef = declRefExpr->declRef;
+                if(auto paramDeclRef = declRef.as<ParamDecl>())
+                {
+                    if (paramDeclRef.as<ModernParamDecl>())
+                    {
+                        // functions declared in our "modern" style (using
+                        // the `func` keyword) never have mutable `in`
+                        // parameters.
+                        //
+                        return nullptr;
+                    }
+
+                    if (paramDeclRef.getDecl()->findModifier<OutModifier>())
+                    {
+                        // Function parameters marked with `out`, `inout`,
+                        // or `in out` are all mutable in a way where
+                        // the result of mutations will be visible to the
+                        // caller.
+                        //
+                        return nullptr;
+                    }
+
+                    // At this point we have an l-value decl-ref to a
+                    // function parameter that is (implicitly or
+                    // explicitly) declared `in`.
+                    //
+                    return paramDeclRef.getDecl();
+                }
+            }
+            else if (auto memberExpr = as<MemberExpr>(expr))
+            {
+                expr = memberExpr->baseExpression;
+                continue;
+            }
+            else if (auto indexExpr = as<IndexExpr>(expr))
+            {
+                expr = indexExpr->baseExpression;
+                continue;
+            }
+
+            return nullptr;
+        }
+    }
+
     bool SemanticsVisitor::TryCheckOverloadCandidateDirections(
         OverloadResolveContext&     context,
         OverloadCandidate const&    candidate)
@@ -519,6 +571,25 @@ namespace Slang
                     }
                     return false;
                 }
+
+                // The parameters of functions declared using traditional/legacy
+                // syntax are currently exposed as mutable locals within the body
+                // of the relevant function. As such, it is legal to call `[mutating]`
+                // methods on such a function parameter. However, doing so is typically
+                // indicative of an error on the programmer's part.
+                //
+                // We will detect such cases here and issue a diagnostic that explains
+                // the situation.
+                //
+                if(context.baseExpr && context.mode == OverloadResolveContext::Mode::ForReal)
+                {
+                    if(auto paramDecl = isReferenceIntoFunctionInputParameter(context.baseExpr))
+                    {
+                        getSink()->diagnose(context.loc, Diagnostics::mutatingMethodOnFunctionInputParameter,
+                            funcDeclRef.getName(),
+                            paramDecl->getName());
+                    }
+                }
             }
         }
 
diff --git a/source/slang/slang-diagnostic-defs.h b/source/slang/slang-diagnostic-defs.h
index e2c00ec38..8f2413c51 100644
--- a/source/slang/slang-diagnostic-defs.h
+++ b/source/slang/slang-diagnostic-defs.h
@@ -301,6 +301,8 @@ DIAGNOSTIC(30064, Note,  implicitCastUsedAsLValue, "argument was implicitly cast
 DIAGNOSTIC(30065, Error, newCanOnlyBeUsedToInitializeAClass, "`new` can only be used to initialize a class")
 DIAGNOSTIC(30066, Error, classCanOnlyBeInitializedWithNew, "a class can only be initialized by a `new` clause")
 
+DIAGNOSTIC(30067, Error, mutatingMethodOnFunctionInputParameter, "mutating method '$0' called on `in` parameter '$1'; changes will not be visible to caller. copy the parameter into a local variable if this behavior is intended")
+
 DIAGNOSTIC(30100, Error, staticRefToNonStaticMember, "type '$0' cannot be used to refer to non-static member '$1'")
 
 DIAGNOSTIC(30200, Error, redeclaration, "declaration of '$0' conflicts with existing declaration")
diff --git a/tests/bugs/mutating/mutating-call-in-loop-dce.slang b/tests/bugs/mutating/mutating-call-in-loop-dce.slang
index 372cb61b0..15e5db304 100644
--- a/tests/bugs/mutating/mutating-call-in-loop-dce.slang
+++ b/tests/bugs/mutating/mutating-call-in-loop-dce.slang
@@ -18,8 +18,9 @@ struct C
         a = 0;
     }
 }
-int doSomething(C d)
+int doSomething(C d_in)
 {
+    C d = d_in;
     int rs = 0;
     for (int i = 0; i < 10; i++)
         rs = d.add();
diff --git a/tests/pipeline/ray-tracing/ray-query-subroutine.slang b/tests/pipeline/ray-tracing/ray-query-subroutine.slang
index 74489b6f8..122a2c966 100644
--- a/tests/pipeline/ray-tracing/ray-query-subroutine.slang
+++ b/tests/pipeline/ray-tracing/ray-query-subroutine.slang
@@ -7,7 +7,7 @@
 RWStructuredBuffer<int> gOutput;
 RaytracingAccelerationStructure gScene;
 
-float3 helper<let N : uint>(RayQuery<N> q)
+float3 helper<let N : uint>(inout RayQuery<N> q)
 {
     RayDesc ray;
     ray.Origin = 0;