[OSLog Diagnostics] Improve os log diagnostics emitted in the SIL and Sema diagnostic passes.

include/swift/AST/DiagnosticsSIL.def

@@ -585,8 +585,8 @@ ERROR(oslog_constant_eval_trap, none, "%0", (StringRef))
 ERROR(oslog_too_many_instructions, none, "interpolated expression and arguments "
       "are too complex", ())
 
-ERROR(oslog_invalid_log_message, none, "invalid log message; do not define "
-      "extensions to types defined in the os module", ())
+ERROR(oslog_invalid_log_message, none, "invalid log message; extending "
+      "types defined in the os module is not supported", ())
 
 NOTE(oslog_const_evaluable_fun_error, none, "'%0' failed evaluation", (StringRef))
 
@@ -599,8 +599,9 @@ ERROR(oslog_non_constant_interpolation, none, "'OSLogInterpolation' instance "
 ERROR(oslog_property_not_constant, none, "'OSLogInterpolation.%0' is not a "
       "constant", (StringRef))
 
-ERROR(oslog_message_alive_after_opts, none, "os log string interpolation cannot "
-      "be used in this context", ())
+ERROR(oslog_message_alive_after_opts, none, "string interpolation cannot "
+      "be used in this context; if you are calling an os_log function, "
+      "try a different overload", ())
 
 ERROR(oslog_message_explicitly_created, none, "'OSLogMessage' must be "
       " created from a string interpolation or string literal", ())
@@ -609,7 +610,7 @@ WARNING(oslog_call_in_unreachable_code, none, "os log call will never be "
         "executed and may have undiagnosed errors", ())
 
 ERROR(global_string_pointer_on_non_constant, none, "globalStringTablePointer "
-      "builtin must used only on string literals", ())
+      "builtin must be used only on string literals", ())
 
 ERROR(polymorphic_builtin_passed_non_trivial_non_builtin_type, none, "Argument "
       "of type %0 can not be passed as an argument to a Polymorphic "

lib/SIL/IR/SILConstants.cpp

@@ -28,6 +28,10 @@ llvm::cl::opt<unsigned>
 template <typename... T, typename... U>
 static InFlightDiagnostic diagnose(ASTContext &Context, SourceLoc loc,
                                    Diag<T...> diag, U &&... args) {
+  // The lifetime of StringRef arguments will be extended as necessary by this
+  // utility. The copy happens in onTentativeDiagnosticFlush at the bottom of
+  // DiagnosticEngine.cpp, which is called when the destructor of the
+  // InFlightDiagnostic returned by diagnose runs.
   return Context.Diags.diagnose(loc, diag, std::forward<U>(args)...);
 }
 
@@ -939,8 +943,8 @@ void SymbolicValue::emitUnknownDiagnosticNotes(SILLocation fallbackLoc) {
                triggerLocSkipsInternalLocs);
     return;
   case UnknownReason::Trap: {
-    const char *message = unknownReason.getTrapMessage();
-    diagnose(ctx, diagLoc, diag::constexpr_trap, StringRef(message));
+    diagnose(ctx, diagLoc, diag::constexpr_trap,
+             unknownReason.getTrapMessage());
     if (emitTriggerLocInDiag)
       diagnose(ctx, triggerLoc, diag::constexpr_trap_operation,
                triggerLocSkipsInternalLocs);
@@ -987,7 +991,7 @@ void SymbolicValue::emitUnknownDiagnosticNotes(SILLocation fallbackLoc) {
     std::string demangledCalleeName =
         demangleSymbolNameForDiagnostics(callee->getName());
     diagnose(ctx, diagLoc, diag::constexpr_found_callee_with_no_body,
-             StringRef(demangledCalleeName));
+             demangledCalleeName);
     if (emitTriggerLocInDiag)
       diagnose(ctx, triggerLoc, diag::constexpr_callee_with_no_body,
                triggerLocSkipsInternalLocs);
@@ -1040,7 +1044,7 @@ void SymbolicValue::emitUnknownDiagnosticNotes(SILLocation fallbackLoc) {
                          witnessMethodName);
 
     diagnose(ctx, diagLoc, diag::constexpr_unresolvable_witness_call,
-             StringRef(witnessMethodName));
+             witnessMethodName);
     if (emitTriggerLocInDiag)
       diagnose(ctx, triggerLoc, diag::constexpr_no_witness_table_entry,
                triggerLocSkipsInternalLocs);

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

@@ -108,6 +108,10 @@ using namespace Lowering;
 template <typename... T, typename... U>
 static void diagnose(ASTContext &Context, SourceLoc loc, Diag<T...> diag,
                      U &&... args) {
+  // The lifetime of StringRef arguments will be extended as necessary by this
+  // utility. The copy happens in onTentativeDiagnosticFlush at the bottom of
+  // DiagnosticEngine.cpp, which is called when the destructor of the
+  // InFlightDiagnostic returned by diagnose runs.
   Context.Diags.diagnose(loc, diag, std::forward<U>(args)...);
 }
 
@@ -365,9 +369,8 @@ static bool diagnoseSpecialErrors(SILInstruction *unevaluableInst,
 
   if (unknownReason.getKind() == UnknownReason::Trap) {
     // We have an assertion failure or fatal error.
-    const char *message = unknownReason.getTrapMessage();
     diagnose(ctx, sourceLoc, diag::oslog_constant_eval_trap,
-             StringRef(message));
+             unknownReason.getTrapMessage());
     return true;
   }
   if (unknownReason.getKind() == UnknownReason::TooManyInstructions) {
@@ -1207,7 +1210,9 @@ static void deleteInstructionWithUsersAndFixLifetimes(
 /// Try to dead-code eliminate the OSLogMessage instance \c oslogMessage passed
 /// to the os log call and clean up its dependencies. If the instance cannot be
 /// eliminated, emit diagnostics.
-static void tryEliminateOSLogMessage(SingleValueInstruction *oslogMessage) {
+/// \returns true if elimination is successful and false if it is not successful
+/// and diagnostics is emitted.
+static bool tryEliminateOSLogMessage(SingleValueInstruction *oslogMessage) {
   InstructionDeleter deleter;
   // List of instructions that are possibly dead.
   SmallVector<SILInstruction *, 4> worklist = {oslogMessage};
@@ -1246,13 +1251,15 @@ static void tryEliminateOSLogMessage(SingleValueInstruction *oslogMessage) {
     SILFunction *fun = oslogMessage->getFunction();
     diagnose(fun->getASTContext(), oslogMessage->getLoc().getSourceLoc(),
              diag::oslog_message_alive_after_opts);
+    return false;
   }
+  return true;
 }
 
 /// Constant evaluate instructions starting from \p start and fold the uses
 /// of the SIL value \p oslogMessage.
-/// \returns true if the body of the function containing \p oslogMessage is
-/// modified. Returns false otherwise.
+/// \returns true if folding is successful and false if it is not successful and
+/// diagnostics is emitted.
 static bool constantFold(SILInstruction *start,
                          SingleValueInstruction *oslogMessage,
                          unsigned assertConfig) {
@@ -1279,9 +1286,7 @@ static bool constantFold(SILInstruction *start,
     return false;
 
   substituteConstants(state);
-
-  tryEliminateOSLogMessage(oslogMessage);
-  return true;
+  return tryEliminateOSLogMessage(oslogMessage);
 }
 
 /// Given a call to the initializer of OSLogMessage, which conforms to
@@ -1431,19 +1436,28 @@ suppressGlobalStringTablePointerError(SingleValueInstruction *oslogMessage) {
   SmallVector<SILInstruction *, 8> users;
   getTransitiveUsers(oslogMessage, users);
 
+  // Collect all globalStringTablePointer instructions.
+  SmallVector<BuiltinInst *, 4> globalStringTablePointerInsts;
   for (SILInstruction *user : users) {
     BuiltinInst *bi = dyn_cast<BuiltinInst>(user);
-    if (!bi ||
-        bi->getBuiltinInfo().ID != BuiltinValueKind::GlobalStringTablePointer)
-      continue;
-    // Replace this builtin by a string_literal instruction for an empty string.
+    if (bi &&
+        bi->getBuiltinInfo().ID == BuiltinValueKind::GlobalStringTablePointer)
+      globalStringTablePointerInsts.push_back(bi);
+  }
+
+  // Replace the globalStringTablePointer builtins by a string_literal
+  // instruction for an empty string and clean up dead code.
+  InstructionDeleter deleter;
+  for (BuiltinInst *bi : globalStringTablePointerInsts) {
     SILBuilderWithScope builder(bi);
     StringLiteralInst *stringLiteral = builder.createStringLiteral(
         bi->getLoc(), StringRef(""), StringLiteralInst::Encoding::UTF8);
     bi->replaceAllUsesWith(stringLiteral);
-    // Here, the bulitin instruction is dead, so clean it up.
-    eliminateDeadInstruction(bi);
+    // The bulitin instruction is likely dead. But since we are iterating over
+    // many instructions, do the cleanup at the end.
+    deleter.trackIfDead(bi);
   }
+  deleter.cleanUpDeadInstructions();
 }
 
 /// If the SILInstruction is an initialization of OSLogMessage, return the
@@ -1536,14 +1550,14 @@ class OSLogOptimization : public SILFunctionTransform {
         // The log call is in unreachable code here.
         continue;
       }
-      bool bodyModified =
+      bool foldingSucceeded =
           constantFold(interpolationStart, oslogInit, assertConfig);
-      // If body was not modified, it implies that an error was diagnosed.
+      // If folding did not succeeded, it implies that an error was diagnosed.
       // However, this will also trigger a diagnostics later on since
       // _globalStringTablePointerBuiltin would not be passed a string literal.
       // Suppress this error by synthesizing a dummy string literal for the
       // builtin.
-      if (!bodyModified)
+      if (!foldingSucceeded)
         suppressGlobalStringTablePointerError(oslogInit);
       madeChange = true;
     }

lib/Sema/ConstantnessSemaDiagnostics.cpp

@@ -162,6 +162,12 @@ static Expr *checkConstantness(Expr *expr) {
     if (!isa<ApplyExpr>(expr))
       return expr;
 
+    if (NominalTypeDecl *nominal =
+        expr->getType()->getNominalOrBoundGenericNominal()) {
+      if (nominal->getName() == nominal->getASTContext().Id_OSLogMessage)
+        return expr;
+    }
+
     ApplyExpr *apply = cast<ApplyExpr>(expr);
     ValueDecl *calledValue = apply->getCalledValue();
     if (!calledValue)

test/SILOptimizer/OSLogCompilerDiagnosticsTest.swift

@@ -27,7 +27,7 @@ extension OSLogInterpolation {
 
 func testOSLogInterpolationExtension(a: A) {
   _osLogTestHelper("Error at line: \(a: a)")
-    // expected-error @-1 {{invalid log message; do not define extensions to types defined in the os module}}
+    // expected-error @-1 {{invalid log message; extending types defined in the os module is not supported}}
     // expected-note @-2 {{'OSLogInterpolation.appendLiteral(_:)' failed evaluation}}
     // expected-note @-3 {{value mutable by an unevaluated instruction is not a constant}}
 }
@@ -87,6 +87,6 @@ func testUnreachableLogCallComplex(c: Color)  {
   default: // expected-warning {{default will never be executed}}
     _osLogTestHelper("Some call \(c)")
       // expected-warning@-1 {{os log call will never be executed and may have undiagnosed errors}}
-      // expected-error@-2 {{globalStringTablePointer builtin must used only on string literals}}
+      // expected-error@-2 {{globalStringTablePointer builtin must be used only on string literals}}
   }
 }

test/SILOptimizer/diagnostic_constant_propagation.swift

@@ -361,7 +361,7 @@ func testBuiltinGlobalStringTablePointerNoError() -> UnsafePointer<CChar> {
 }
 
 func testBuiltinGlobalStringTablePointerError(s: String) -> UnsafePointer<CChar> {
-  return _getGlobalStringTablePointer(s) // expected-error {{globalStringTablePointer builtin must used only on string literals}}
+  return _getGlobalStringTablePointer(s) // expected-error {{globalStringTablePointer builtin must be used only on string literals}}
 }
 
 @_transparent

test/Sema/diag_constantness_check_os_log.swift

@@ -119,6 +119,17 @@ func testOSLogInterpolationExtension(a: MyStruct) {
   _osLogTestHelper("Error at line: \(a: a)")
 }
 
+func testExplicitOSLogMessageConstruction() {
+  _osLogTestHelper(OSLogMessage(stringLiteral: "world"))
+    // expected-error@-1 {{argument must be a string interpolation}}
+  _osLogTestHelper(
+    OSLogMessage( // expected-error {{argument must be a string interpolation}}
+      stringInterpolation:
+        OSLogInterpolation(
+          literalCapacity: 0,
+          interpolationCount: 0)))
+}
+
 // Test that @_semantics("oslog.log_with_level") permits values of type OSLog and
 // OSLogType to not be constants.