Emit additional diagnostic for invalid pointer taking operations (#7663)

* Emit special diagnostic for invalid pointer taking operations

* Update source/slang/slang-diagnostic-defs.h

Co-authored-by: ArielG-NV <159081215+ArielG-NV@users.noreply.github.com>

* Add OperatorAddressOf KnownBuiltin modifier

* update error message for non-l-value assignment

* update the diagnostics in the tests

* Use enum based KnownBuiltinDeclName

* format code (#7772)

Co-authored-by: slangbot <186143334+slangbot@users.noreply.github.com>

---------

Co-authored-by: ArielG-NV <159081215+ArielG-NV@users.noreply.github.com>
Co-authored-by: slangbot <ellieh+slangbot@nvidia.com>
Co-authored-by: slangbot <186143334+slangbot@users.noreply.github.com>

8 files changed, 49 insertions, 52 deletions

diff --git a/source/slang/core.meta.slang b/source/slang/core.meta.slang
index 380f79a6a..f42cd25cf 100644
--- a/source/slang/core.meta.slang
+++ b/source/slang/core.meta.slang
@@ -2583,6 +2583,7 @@ __prefix Ref<T> operator*(Ptr<T, addrSpace> value);
 
 __generic<T>
 __intrinsic_op(0)
+[KnownBuiltin($( (int)KnownBuiltinDeclName::OperatorAddressOf))]
 [require(cpp_cuda_spirv)]
 __prefix Ptr<T, $((uint64_t)AddressSpace::UserPointer)ULL> operator&(__ref T value);
 
diff --git a/source/slang/slang-ast-support-types.cpp b/source/slang/slang-ast-support-types.cpp
index 7665fb6d4..3ac352f0a 100644
--- a/source/slang/slang-ast-support-types.cpp
+++ b/source/slang/slang-ast-support-types.cpp
@@ -96,28 +96,4 @@ void printDiagnosticArg(StringBuilder& sb, ParameterDirection direction)
     }
 }
 
-KnownBuiltinDeclName getKnownBuiltinDeclNameFromString(UnownedStringSlice name)
-{
-    if (name == "GeometryStreamAppend")
-        return KnownBuiltinDeclName::GeometryStreamAppend;
-    else if (name == "GeometryStreamRestart")
-        return KnownBuiltinDeclName::GeometryStreamRestart;
-    else if (name == "GetAttributeAtVertex")
-        return KnownBuiltinDeclName::GetAttributeAtVertex;
-    else if (name == "DispatchMesh")
-        return KnownBuiltinDeclName::DispatchMesh;
-    else if (name == "saturated_cooperation")
-        return KnownBuiltinDeclName::saturated_cooperation;
-    else if (name == "saturated_cooperation_using")
-        return KnownBuiltinDeclName::saturated_cooperation_using;
-    else if (name == "IDifferentiable")
-        return KnownBuiltinDeclName::IDifferentiable;
-    else if (name == "IDifferentiablePtr")
-        return KnownBuiltinDeclName::IDifferentiablePtr;
-    else if (name == "NullDifferential")
-        return KnownBuiltinDeclName::NullDifferential;
-    else
-        return KnownBuiltinDeclName::COUNT;
-}
-
 } // namespace Slang
diff --git a/source/slang/slang-ast-support-types.h b/source/slang/slang-ast-support-types.h
index 8fe432bc5..6dc4203a6 100644
--- a/source/slang/slang-ast-support-types.h
+++ b/source/slang/slang-ast-support-types.h
@@ -233,6 +233,7 @@ FIDDLE() namespace Slang
         IDifferentiable,
         IDifferentiablePtr,
         NullDifferential,
+        OperatorAddressOf,
         COUNT
     };
 
diff --git a/source/slang/slang-check-expr.cpp b/source/slang/slang-check-expr.cpp
index 9325eda61..41c3bd510 100644
--- a/source/slang/slang-check-expr.cpp
+++ b/source/slang/slang-check-expr.cpp
@@ -2663,7 +2663,7 @@ Expr* SemanticsExprVisitor::visitTupleExpr(TupleExpr* expr)
     return expr;
 }
 
-void SemanticsVisitor::maybeDiagnoseThisNotLValue(Expr* expr)
+void SemanticsVisitor::maybeDiagnoseConstVariableAssignment(Expr* expr)
 {
     // We will try to handle expressions of the form:
     //
@@ -2688,15 +2688,11 @@ void SemanticsVisitor::maybeDiagnoseThisNotLValue(Expr* expr)
             break;
         }
     }
-    //
-    // Now we check to see if we have a `this` expression,
-    // and if it is immutable.
-    if (auto thisExpr = as<ThisExpr>(e))
+
+    // Check if we're trying to assign to a non-l-value (const variable, immutable member, etc.)
+    if (!expr->type.isLeftValue)
     {
-        if (!thisExpr->type.isLeftValue)
-        {
-            getSink()->diagnoseWithoutSourceView(thisExpr, Diagnostics::thisIsImmutableByDefault);
-        }
+        getSink()->diagnoseWithoutSourceView(expr, Diagnostics::attemptingToAssignToConstVariable);
     }
 }
 
@@ -2722,14 +2718,10 @@ Expr* SemanticsVisitor::checkAssignWithCheckedOperands(AssignExpr* expr)
         }
         else
         {
-            getSink()->diagnose(expr, Diagnostics::assignNonLValue);
+            // Provide a more helpful diagnostic about const variable assignment
+            maybeDiagnoseConstVariableAssignment(expr->left);
 
-            // As a special case, check if the LHS expression is derived
-            // from a `this` parameter (implicitly or explicitly), which
-            // is immutable. We can give the user a bit more context into
-            // what is going on.
-            //
-            maybeDiagnoseThisNotLValue(expr->left);
+            getSink()->diagnose(expr, Diagnostics::assignNonLValue);
         }
     }
     expr->type = type;
@@ -2963,6 +2955,29 @@ Expr* SemanticsVisitor::CheckInvokeExprWithCheckedOperands(InvokeExpr* expr)
                             }
                             else if (!as<ErrorType>(argExpr->type))
                             {
+                                // Emit additional diagnostic for invalid pointer taking operations
+                                auto funcDeclRef = getDeclRef(m_astBuilder, funcDeclRefExpr);
+                                if (funcDeclRef)
+                                {
+                                    auto knownBuiltinAttr =
+                                        funcDeclRef.getDecl()
+                                            ->findModifier<KnownBuiltinAttribute>();
+                                    if (knownBuiltinAttr)
+                                    {
+                                        if (auto constantIntVal =
+                                                as<ConstantIntVal>(knownBuiltinAttr->name))
+                                        {
+                                            if (constantIntVal->getValue() ==
+                                                (int)KnownBuiltinDeclName::OperatorAddressOf)
+                                            {
+                                                getSink()->diagnose(
+                                                    argExpr,
+                                                    Diagnostics::cannotTakeConstantPointers);
+                                            }
+                                        }
+                                    }
+                                }
+
                                 getSink()->diagnose(
                                     argExpr,
                                     Diagnostics::argumentExpectedLValue,
@@ -3000,7 +3015,7 @@ Expr* SemanticsVisitor::CheckInvokeExprWithCheckedOperands(InvokeExpr* expr)
                                         implicitCastExpr->type);
                                 }
 
-                                maybeDiagnoseThisNotLValue(argExpr);
+                                maybeDiagnoseConstVariableAssignment(argExpr);
                             }
                         }
                     }
diff --git a/source/slang/slang-check-impl.h b/source/slang/slang-check-impl.h
index 55211dde5..6b36031ee 100644
--- a/source/slang/slang-check-impl.h
+++ b/source/slang/slang-check-impl.h
@@ -2278,7 +2278,7 @@ public:
 
     /// Given an immutable `expr` used as an l-value emit a special diagnostic if it was derived
     /// from `this`.
-    void maybeDiagnoseThisNotLValue(Expr* expr);
+    void maybeDiagnoseConstVariableAssignment(Expr* expr);
 
     // Figure out what type an initializer/constructor declaration
     // is supposed to return. In most cases this is just the type
diff --git a/source/slang/slang-check-overload.cpp b/source/slang/slang-check-overload.cpp
index 41aba2674..52b0ef5bc 100644
--- a/source/slang/slang-check-overload.cpp
+++ b/source/slang/slang-check-overload.cpp
@@ -913,7 +913,7 @@ bool SemanticsVisitor::TryCheckOverloadCandidateDirections(
                         context.loc,
                         Diagnostics::mutatingMethodOnImmutableValue,
                         funcDeclRef.getName());
-                    maybeDiagnoseThisNotLValue(context.baseExpr);
+                    maybeDiagnoseConstVariableAssignment(context.baseExpr);
                 }
                 return false;
             }
diff --git a/source/slang/slang-diagnostic-defs.h b/source/slang/slang-diagnostic-defs.h
index 8dbdaaaa6..fdfa968c3 100644
--- a/source/slang/slang-diagnostic-defs.h
+++ b/source/slang/slang-diagnostic-defs.h
@@ -701,6 +701,11 @@ DIAGNOSTIC(
     argumentExpectedLValue,
     "argument passed to parameter '$0' must be l-value.")
 DIAGNOSTIC(
+    30078,
+    Error,
+    cannotTakeConstantPointers,
+    "Not allowed to take pointer of an immutable object")
+DIAGNOSTIC(
     30048,
     Error,
     argumentHasMoreMemoryQualifiersThanParam,
@@ -709,9 +714,10 @@ DIAGNOSTIC(
 DIAGNOSTIC(
     30049,
     Note,
-    thisIsImmutableByDefault,
-    "a 'this' parameter is an immutable parameter by default in Slang; apply the `[mutating]` "
-    "attribute to the function declaration to opt in to a mutable `this`")
+    attemptingToAssignToConstVariable,
+    "attempting to assign to a const variable or immutable member; use '[mutating]' attribute on "
+    "the containing method to allow modification")
+
 DIAGNOSTIC(
     30050,
     Error,
diff --git a/source/slang/slang-ir-fuse-satcoop.cpp b/source/slang/slang-ir-fuse-satcoop.cpp
index fcb3dd5f7..d0c0f75ad 100644
--- a/source/slang/slang-ir-fuse-satcoop.cpp
+++ b/source/slang/slang-ir-fuse-satcoop.cpp
@@ -457,7 +457,7 @@ static IRCall* tryFuseCalls(IRBuilder& builder, IRCall* f, IRCall* g)
 //
 // Identify calls which we can fuse
 //
-IRCall* isKnownFunction(const char* n, IRInst* i)
+IRCall* isKnownFunction(KnownBuiltinDeclName expectedNameEnum, IRInst* i)
 {
     auto call = as<IRCall>(i);
     if (!call)
@@ -478,9 +478,7 @@ IRCall* isKnownFunction(const char* n, IRInst* i)
     if (!h)
         return nullptr;
 
-    // Convert string to enum for comparison
-    auto expectedEnum = getKnownBuiltinDeclNameFromString(UnownedStringSlice(n));
-    if (h->getName() != expectedEnum)
+    if (h->getName() != expectedNameEnum)
         return nullptr;
     return call;
 }
@@ -525,7 +523,7 @@ static void fuseCallsInBlock(IRBuilder& builder, IRBlock* block)
     List<IRCall*> toInline;
     for (auto inst : block->getChildren())
     {
-        if (auto sat_coop = isKnownFunction("saturated_cooperation", inst))
+        if (auto sat_coop = isKnownFunction(KnownBuiltinDeclName::saturated_cooperation, inst))
             toInline.add(sat_coop);
     }
     for (auto c : toInline)
@@ -541,7 +539,7 @@ static void fuseCallsInBlock(IRBuilder& builder, IRBlock* block)
     for (auto inst = block->getFirstInst(); inst != block->getTerminator();
          inst = inst->getNextInst())
     {
-        if (auto call = isKnownFunction("saturated_cooperation_using", inst))
+        if (auto call = isKnownFunction(KnownBuiltinDeclName::saturated_cooperation_using, inst))
         {
             if (lastCall)
             {