From 891a6cf376c6b2560231502614b37c332f44ddea Mon Sep 17 00:00:00 2001
From: jsmall-nvidia <jsmall@nvidia.com>
Date: Wed, 6 Sep 2023 19:32:06 -0400
Subject: Make a warning if a [mutating] method is called on an in param.
 (#3184)

* Make a warning if a [mutating] method is passed as an in param.

* Kick CI.

---------

Co-authored-by: Yong He <yonghe@outlook.com>
---
 source/slang/slang-check-overload.cpp           |  8 ++++-
 source/slang/slang-diagnostic-defs.h            |  3 +-
 tests/diagnostics/param-mutation.slang          | 39 +++++++++++++++++++++++++
 tests/diagnostics/param-mutation.slang.expected |  8 +++++
 4 files changed, 56 insertions(+), 2 deletions(-)
 create mode 100644 tests/diagnostics/param-mutation.slang
 create mode 100644 tests/diagnostics/param-mutation.slang.expected

diff --git a/source/slang/slang-check-overload.cpp b/source/slang/slang-check-overload.cpp
index 50da2d3eb..5e626705a 100644
--- a/source/slang/slang-check-overload.cpp
+++ b/source/slang/slang-check-overload.cpp
@@ -586,7 +586,13 @@ namespace Slang
                 {
                     if(auto paramDecl = isReferenceIntoFunctionInputParameter(context.baseExpr))
                     {
-                        getSink()->diagnose(context.loc, Diagnostics::mutatingMethodOnFunctionInputParameter,
+                        const bool isNonCopyable = isNonCopyableType(paramDecl->getType());
+
+                        const auto& diagnotic = isNonCopyable ?
+                            Diagnostics::mutatingMethodOnFunctionInputParameterError :
+                            Diagnostics::mutatingMethodOnFunctionInputParameterWarning;
+
+                        getSink()->diagnose(context.loc, diagnotic,
                             funcDeclRef.getName(),
                             paramDecl->getName());
                     }
diff --git a/source/slang/slang-diagnostic-defs.h b/source/slang/slang-diagnostic-defs.h
index 76e96f2d5..5a28c8188 100644
--- a/source/slang/slang-diagnostic-defs.h
+++ b/source/slang/slang-diagnostic-defs.h
@@ -316,7 +316,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(30067, Error, mutatingMethodOnFunctionInputParameterError, "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(30068, Warning, mutatingMethodOnFunctionInputParameterWarning, "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'")
 
diff --git a/tests/diagnostics/param-mutation.slang b/tests/diagnostics/param-mutation.slang
new file mode 100644
index 000000000..835e645c8
--- /dev/null
+++ b/tests/diagnostics/param-mutation.slang
@@ -0,0 +1,39 @@
+// param-mutation.slang
+
+// When a parameter is passed to `in`, it is mutable but generates a warning as it probably (?)
+// isn't what the programmer intended.
+
+//DIAGNOSTIC_TEST:SIMPLE:
+
+struct MutatingStruct
+{
+    [mutating] void setValue(int value) { m_value = value; }
+    int m_value;
+};
+
+int doThing(MutatingStruct s, int v)
+{
+    // Should generate a warning. 
+    s.setValue(v + 1);
+    return s.m_value;
+}
+
+// For non-copyable types (such as HitObject or NonCopyableStruct declared below), if passed as as `in`
+// should produce an error.
+// NOTE! This *doesn't* produce an error (or warning) because NonCopyable types are *implicitly* 
+// made *ref* when parsed as arguments. 
+
+[__NonCopyableType]
+struct NonCopyableStruct
+{
+    [mutating] void setValue(int value) { m_value = value; }
+    int m_value;
+};
+
+int doThing2(NonCopyableStruct s, int v)
+{
+    // Currently doesn't produce an error/warning because NonCopyableStruct is passed as *ref* implicitly.
+    s.setValue(v + 1);
+    return s.m_value;
+}
+
diff --git a/tests/diagnostics/param-mutation.slang.expected b/tests/diagnostics/param-mutation.slang.expected
new file mode 100644
index 000000000..a1fb34fb7
--- /dev/null
+++ b/tests/diagnostics/param-mutation.slang.expected
@@ -0,0 +1,8 @@
+result code = 0
+standard error = {
+tests/diagnostics/param-mutation.slang(17): warning 30068: mutating method 'setValue' called on `in` parameter 's'; changes will not be visible to caller. copy the parameter into a local variable if this behavior is intended
+    s.setValue(v + 1);
+              ^
+}
+standard output = {
+}
-- 
cgit v1.2.3