fix(const-param): say when a callee's const parameter is the only evidence

include/analysis/ConstParamAnalysis.hpp

@@ -28,7 +28,10 @@ namespace ctrace::stack::analysis
         std::uint64_t pointerConstOnly : 1 = false; // ex: T * const param
         std::uint64_t isReference : 1 = false;
         std::uint64_t isRvalueRef : 1 = false;
-        std::uint64_t reservedFlags : 61 = 0;
+        // The parameter is only passed to callees whose parameter is declared to point to const:
+        // their bodies are not read, so the object is not known to be unmodified (#167).
+        std::uint64_t reliesOnCalleeConst : 1 = false;
+        std::uint64_t reservedFlags : 60 = 0;
     };
 
     // For the functions defined in the modules analyzed together, keyed by linker symbol: whether

run_test.py

@@ -2821,6 +2821,43 @@ def reported(files: list[str], extra: list[str]) -> Optional[list[str]]:
     return ok
 
 
+def check_const_param_contract_message_across_files() -> bool:
+    """
+    #167: a pointer passed only to a function defined in another file, whose parameter points to
+    const, is reported for that contract alone. The message must say so: the callee's body is not
+    checked, and this one casts the qualifier away.
+    """
+    print("=== Testing the const parameter message when a callee's contract is the evidence ===")
+    sources = {
+        "def.c": "int bump(const int* p)\n{\n    *(int*)p += 1;\n    return *p;\n}\n",
+        "use.c": "int bump(const int* p);\n\nint use(int* p)\n{\n    return bump(p);\n}\n",
+    }
+    headline = (
+        "ConstParameterNotModified.Pointer: parameter 'p' in function 'use' is only read, "
+        "or passed where a pointer to const is expected"
+    )
+    caveat = "the callee's body is not checked: it may still modify the pointed object through a cast"
+    old = "is never used to modify the pointed object"
+    with tempfile.TemporaryDirectory(prefix="ct_const_contract_") as tmp:
+        root = Path(tmp)
+        for name, text in sources.items():
+            (root / name).write_text(text)
+        result = run_analyzer([str(root / "def.c"), str(root / "use.c"), "--format=json"])
+    try:
+        payload = json.loads(result.stdout or "")
+    except json.JSONDecodeError:
+        return fail_check("no JSON output", (result.stdout or "") + (result.stderr or ""))
+    messages = [
+        d.get("details", {}).get("message", "")
+        for d in payload.get("diagnostics", [])
+        if str(d.get("ruleId", "")).startswith("ConstParameterNotModified")
+    ]
+    if len(messages) != 1 or headline not in messages[0] or caveat not in messages[0] or old in messages[0]:
+        return fail_check(f"expected one contract message for use, got {messages}")
+    print("  ✅ use: reported for the contract of bump, and says so")
+    print()
+    return True
+
 def check_duplicate_if_cross_tu() -> bool:
     """
     #157: a function defined in another file is deterministic when every definition of it is, as
@@ -5459,6 +5496,7 @@ def record_ok(ok: bool):
         check_uninitialized_cross_tu,
         check_uninitialized_cross_tu_no_effect,
         check_const_param_cross_tu,
+        check_const_param_contract_message_across_files,
         check_duplicate_if_cross_tu,
         check_global_read_before_write_identity_across_files,
         check_size_minus_one_cross_tu,

src/analysis/ConstParamAnalysis.cpp

@@ -3,6 +3,7 @@
 #include "analysis/IRValueUtils.hpp"
 #include "analysis/AnalyzerUtils.hpp"
 
+#include <algorithm>
 #include <cctype>
 
 #include <llvm/ADT/SmallPtrSet.h>
@@ -288,17 +289,22 @@ namespace ctrace::stack::analysis
         enum class ParamWriteState : std::uint8_t
         {
             NoWrite = 0,
-            Unknown = 1,
-            MayWrite = 2
+            // No write, as far as the callees' declared const parameters say: their bodies are
+            // not read, and one may cast the qualifier away (#167).
+            NoWriteByContract = 1,
+            Unknown = 2,
+            MayWrite = 3
         };
 
+        static bool writesNothing(ParamWriteState state)
+        {
+            return state == ParamWriteState::NoWrite || state == ParamWriteState::NoWriteByContract;
+        }
+
+        // The least certain of both.
         static ParamWriteState mergeParamWriteState(ParamWriteState lhs, ParamWriteState rhs)
         {
-            if (lhs == ParamWriteState::MayWrite || rhs == ParamWriteState::MayWrite)
-                return ParamWriteState::MayWrite;
-            if (lhs == ParamWriteState::Unknown || rhs == ParamWriteState::Unknown)
-                return ParamWriteState::Unknown;
-            return ParamWriteState::NoWrite;
+            return std::max(lhs, rhs);
         }
 
         static bool calleeParamIsReadOnly(const llvm::Function* callee, unsigned argIndex)
@@ -374,7 +380,7 @@ namespace ctrace::stack::analysis
                 if (it != otherModules->functions.end() && it->second.size() == CB.arg_size() &&
                     it->second[argIndex])
                 {
-                    return ParamWriteState::NoWrite;
+                    return ParamWriteState::NoWriteByContract;
                 }
             }
 
@@ -397,7 +403,7 @@ namespace ctrace::stack::analysis
             }
 
             if (calleeParamIsReadOnly(callee, argIndex))
-                return ParamWriteState::NoWrite;
+                return ParamWriteState::NoWriteByContract;
 
             return ParamWriteState::Unknown;
         }
@@ -700,7 +706,7 @@ namespace ctrace::stack::analysis
                 const ParamWriteState writeState = readOnlyByMetadata
                                                        ? ParamWriteState::NoWrite
                                                        : valueWriteState(&Arg, F, otherModules);
-                if (writeState != ParamWriteState::NoWrite)
+                if (!writesNothing(writeState))
                     continue;
 
                 const std::string paramName =
@@ -719,6 +725,7 @@ namespace ctrace::stack::analysis
                     typeInfo.isPointer && typeInfo.pointerConst && !typeInfo.pointeeConst;
                 issue.isReference = typeInfo.isReference;
                 issue.isRvalueRef = typeInfo.isRvalueReference;
+                issue.reliesOnCalleeConst = writeState == ParamWriteState::NoWriteByContract;
 
                 issue.currentType = buildTypeString(typeInfo, baseName, false, true, paramName);
                 if (typeInfo.isRvalueReference)

src/analyzer/DiagnosticEmitter.cpp

@@ -1019,39 +1019,58 @@ namespace ctrace::stack::analyzer
             else if (issue.isReference)
                 subLabel = issue.isRvalueRef ? "ReferenceRvaluePreferValue" : "Reference";
 
+            // Resting on a callee's declared const parameter, the rule only knows how the
+            // parameter is used, not that the object is unmodified (#167).
+            const char* object = issue.isReference ? "referred" : "pointed";
+            const std::string contractUse = std::string("only read, or passed where a ") +
+                                            (issue.isReference ? "reference" : "pointer") +
+                                            " to const is expected";
+            body << "\t" << prefixForSeverity(DiagnosticSeverity::Info)
+                 << " ConstParameterNotModified." << subLabel << ": parameter '" << paramName
+                 << "' in function '" << displayFuncName << "' ";
+            if (issue.isRvalueRef)
+            {
+                body << "is an rvalue reference and is "
+                     << (issue.reliesOnCalleeConst ? contractUse
+                                                   : "never used to modify the referred object");
+            }
+            else if (issue.pointerConstOnly)
+            {
+                body << "is declared '" << issue.currentType << "' but the pointed object is "
+                     << (issue.reliesOnCalleeConst ? contractUse : "never modified");
+            }
+            else if (issue.reliesOnCalleeConst)
+            {
+                body << "is " << contractUse;
+            }
+            else
+            {
+                body << "is never used to modify the " << object << " object";
+            }
+            body << "\n";
+
             if (issue.isRvalueRef)
             {
-                body << "\t" << prefixForSeverity(DiagnosticSeverity::Info)
-                     << " ConstParameterNotModified." << subLabel << ": parameter '" << paramName
-                     << "' in function '" << displayFuncName
-                     << "' is an rvalue reference and is never used to modify the referred "
-                        "object\n";
                 body << kDiagIndentArrow << "consider passing by value (" << issue.suggestedType
                      << ") or const reference (" << issue.suggestedTypeAlt << ")\n";
                 body << kDiagIndentArrow << "current type: " << issue.currentType << "\n";
             }
             else if (issue.pointerConstOnly)
             {
-                body << "\t" << prefixForSeverity(DiagnosticSeverity::Info)
-                     << " ConstParameterNotModified." << subLabel << ": parameter '" << paramName
-                     << "' in function '" << displayFuncName << "' is declared '"
-                     << issue.currentType << "' but the pointed object is never modified\n";
                 body << kDiagIndentArrow << "consider '" << issue.suggestedType
                      << "' for API const-correctness\n";
             }
-            else
-            {
-                body << "\t" << prefixForSeverity(DiagnosticSeverity::Info)
-                     << " ConstParameterNotModified." << subLabel << ": parameter '" << paramName
-                     << "' in function '" << displayFuncName << "' is never used to modify the "
-                     << (issue.isReference ? "referred" : "pointed") << " object\n";
-            }
 
             if (!issue.isRvalueRef)
             {
                 body << kDiagIndentArrow << "current type: " << issue.currentType << "\n";
                 body << kDiagIndentArrow << "suggested type: " << issue.suggestedType << "\n";
             }
+            if (issue.reliesOnCalleeConst)
+            {
+                body << kDiagIndentArrow << "the callee's body is not checked: it may still modify "
+                     << "the " << object << " object through a cast\n";
+            }
 
             DiagnosticBuilder builder;
             builder.function(issue.funcName)

test/pointer_reference-const_correctness/advanced-cases.c

@@ -20,7 +20,7 @@ void consume_mut(int* p)
 }
 
 // at line 25, column 0
-// [ !Info! ] ConstParameterNotModified.Pointer: parameter 'p' in function 'caller_const' is never used to modify the pointed object
+// [ !Info! ] ConstParameterNotModified.Pointer: parameter 'p' in function 'caller_const' is only read, or passed where a pointer to const is expected
 //          ↳ current type: int *p
 //          ↳ suggested type: const int *p
 void caller_const(int* p)

test/pointer_reference-const_correctness/advanced-cases.cpp

@@ -12,7 +12,7 @@ void consume_ref_mut(int& v)
 }
 
 // at line 17, column 0
-// [ !Info! ] ConstParameterNotModified.Reference: parameter 'v' in function 'caller_ref(int&)' is never used to modify the referred object
+// [ !Info! ] ConstParameterNotModified.Reference: parameter 'v' in function 'caller_ref(int&)' is only read, or passed where a reference to const is expected
 //          ↳ current type: int &v
 //          ↳ suggested type: const int &v
 void caller_ref(int& v)

test/pointer_reference-const_correctness/const-callee-contract.c

@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: Apache-2.0
+// #167: a parameter only passed where a pointer to const is expected is not known to be
+// unmodified. The callee's body is not checked, and this one casts the qualifier away.
+// not contains: parameter 'p' in function 'use' is never used to modify the pointed object
+
+static int bump(const int* p)
+{
+    *(int*)p += 1;
+    return *p;
+}
+
+// at line 17, column 0
+// [ !Info! ] ConstParameterNotModified.Pointer: parameter 'p' in function 'use' is only read, or passed where a pointer to const is expected
+//          ↳ current type: int *p
+//          ↳ suggested type: const int *p
+//          ↳ the callee's body is not checked: it may still modify the pointed object through a cast
+int use(int* p)
+{
+    return bump(p);
+}
+
+// Without a call, the function's own body establishes that the object is not modified.
+
+// at line 28, column 0
+// [ !Info! ] ConstParameterNotModified.Pointer: parameter 'q' in function 'peek' is never used to modify the pointed object
+//          ↳ current type: int *q
+//          ↳ suggested type: const int *q
+int peek(int* q)
+{
+    return *q;
+}