Add -allow-rewrite-failures option to downgrade rewrite errors to warnings. (#461)

mattmccutchen-cci · web-flow · commit 76e5061b597e · 2021-02-26T15:56:19.000-05:00
While we're here:

- Add note about -allow-unwritable-changes to the unwritable change
  error, and change the existing note to have no location for
  consistency with the others.

- Fix a highly visible typo in a `3c` error message.
diff --git a/clang/include/clang/3C/3C.h b/clang/include/clang/3C/3C.h
@@ -75,6 +75,8 @@ struct _3COptions {
 
   bool DumpUnwritableChanges;
   bool AllowUnwritableChanges;
+
+  bool AllowRewriteFailures;
 };
 
 // The main interface exposed by the 3C to interact with the tool.
diff --git a/clang/include/clang/3C/3CGlobalOptions.h b/clang/include/clang/3C/3CGlobalOptions.h
@@ -32,6 +32,7 @@ extern bool WarnRootCause;
 extern bool WarnAllRootCause;
 extern bool DumpUnwritableChanges;
 extern bool AllowUnwritableChanges;
+extern bool AllowRewriteFailures;
 
 #ifdef FIVE_C
 extern bool RemoveItypes;
diff --git a/clang/lib/3C/3C.cpp b/clang/lib/3C/3C.cpp
@@ -58,6 +58,7 @@ std::set<std::string> FilePaths;
 bool VerifyDiagnosticOutput;
 bool DumpUnwritableChanges;
 bool AllowUnwritableChanges;
+bool AllowRewriteFailures;
 
 #ifdef FIVE_C
 bool RemoveItypes;
@@ -235,6 +236,7 @@ _3CInterface::_3CInterface(const struct _3COptions &CCopt,
   VerifyDiagnosticOutput = CCopt.VerifyDiagnosticOutput;
   DumpUnwritableChanges = CCopt.DumpUnwritableChanges;
   AllowUnwritableChanges = CCopt.AllowUnwritableChanges;
+  AllowRewriteFailures = CCopt.AllowRewriteFailures;
 
 #ifdef FIVE_C
   RemoveItypes = CCopt.RemoveItypes;
diff --git a/clang/lib/3C/RewriteUtils.cpp b/clang/lib/3C/RewriteUtils.cpp
@@ -162,13 +162,26 @@ void rewriteSourceRange(Rewriter &R, const CharSourceRange &Range,
   // crashing with an assert fail.
   if (!RewriteSuccess) {
     clang::DiagnosticsEngine &DE = R.getSourceMgr().getDiagnostics();
-    unsigned ErrorId =
-      DE.getCustomDiagID(
-        ErrFail ? DiagnosticsEngine::Error : DiagnosticsEngine::Warning,
-        "Unable to rewrite converted source range. Intended rewriting: \"%0\"");
-    auto ErrorBuilder = DE.Report(Range.getBegin(), ErrorId);
-    ErrorBuilder.AddSourceRange(R.getSourceMgr().getExpansionRange(Range));
-    ErrorBuilder.AddString(NewText);
+    bool ReportError = ErrFail && !AllowRewriteFailures;
+    {
+      // Put this in a block because Clang only allows one DiagnosticBuilder to
+      // exist at a time.
+      unsigned ErrorId = DE.getCustomDiagID(
+          ReportError ? DiagnosticsEngine::Error : DiagnosticsEngine::Warning,
+          "Unable to rewrite converted source range. Intended rewriting: \"%0\"");
+      auto ErrorBuilder = DE.Report(Range.getBegin(), ErrorId);
+      ErrorBuilder.AddSourceRange(R.getSourceMgr().getExpansionRange(Range));
+      ErrorBuilder.AddString(NewText);
+    }
+    if (ReportError) {
+      unsigned NoteId = DE.getCustomDiagID(
+          DiagnosticsEngine::Note,
+          "you can use the -allow-rewrite-failures option to temporarily "
+          "downgrade this error to a warning");
+      // If we pass the location here, the macro call stack gets dumped again,
+      // which looks silly.
+      DE.Report(NoteId);
+    }
   }
 }
 
@@ -187,18 +200,28 @@ static void emit(Rewriter &R, ASTContext &C) {
       DiagnosticsEngine::Level UnwritableChangeDiagnosticLevel =
           AllowUnwritableChanges ? DiagnosticsEngine::Warning
                                  : DiagnosticsEngine::Error;
-      auto MaybeDumpUnwritableChange = [&]() {
+      auto PrintExtraUnwritableChangeInfo = [&]() {
+        DiagnosticsEngine &DE = C.getDiagnostics();
+        // With -dump-unwritable-changes and not -allow-unwritable-changes, we
+        // want the -allow-unwritable-changes note before the dump.
+        if (!DumpUnwritableChanges) {
+          unsigned DumpNoteId = DE.getCustomDiagID(
+              DiagnosticsEngine::Note,
+              "use the -dump-unwritable-changes option to see the new version "
+              "of the file");
+          DE.Report(DumpNoteId);
+        }
+        if (!AllowUnwritableChanges) {
+          unsigned AllowNoteId = DE.getCustomDiagID(
+              DiagnosticsEngine::Note,
+              "you can use the -allow-unwritable-changes option to temporarily "
+              "downgrade this error to a warning");
+          DE.Report(AllowNoteId);
+        }
         if (DumpUnwritableChanges) {
           errs() << "=== Beginning of new version of " << FE->getName() << " ===\n";
           Buffer->second.write(errs());
           errs() << "=== End of new version of " << FE->getName() << " ===\n";
-        } else {
-          DiagnosticsEngine &DE = C.getDiagnostics();
-          unsigned ID = DE.getCustomDiagID(
-              DiagnosticsEngine::Note,
-              "use the -dump-unwritable-changes option to see the new version "
-              "of the file");
-          DE.Report(SM.translateFileLineCol(FE, 1, 1), ID);
         }
       };
 
@@ -213,7 +236,7 @@ static void emit(Rewriter &R, ASTContext &C) {
             "is not allowed to write to the file "
             "(https://github.com/correctcomputation/checkedc-clang/issues/387)");
         DE.Report(SM.translateFileLineCol(FE, 1, 1), ID);
-        MaybeDumpUnwritableChange();
+        PrintExtraUnwritableChangeInfo();
         continue;
       }
 
@@ -230,7 +253,7 @@ static void emit(Rewriter &R, ASTContext &C) {
               "but is not the main file and thus cannot be written in stdout "
               "mode");
           DE.Report(SM.translateFileLineCol(FE, 1, 1), ID);
-          MaybeDumpUnwritableChange();
+          PrintExtraUnwritableChangeInfo();
         }
         continue;
       }
diff --git a/clang/test/3C/base_subdir/canwrite_constraints_symlink.c b/clang/test/3C/base_subdir/canwrite_constraints_symlink.c
@@ -23,7 +23,8 @@
 // RUN: cd %t.base && 3c -addcr -verify canwrite_constraints_symlink.c --
 
 // expected-error@base_subdir_partial_defn.h:1 {{3C internal error: 3C generated changes to this file even though it is not allowed to write to the file}}
-// expected-note@base_subdir_partial_defn.h:1 {{-dump-unwritable-changes}}
+// expected-note@*:* {{-dump-unwritable-changes}}
+// expected-note@*:* {{-allow-unwritable-changes}}
 
 void
 #include "base_subdir_partial_defn.h"
diff --git a/clang/test/3C/base_subdir/canwrite_constraints_unimplemented.c b/clang/test/3C/base_subdir/canwrite_constraints_unimplemented.c
@@ -8,7 +8,8 @@
 // RUN: cd %S && 3c -addcr -verify %s --
 
 // expected-error@../base_subdir_partial_defn.h:1 {{3C internal error: 3C generated changes to this file even though it is not allowed to write to the file}}
-// expected-note@../base_subdir_partial_defn.h:1 {{-dump-unwritable-changes}}
+// expected-note@*:* {{-dump-unwritable-changes}}
+// expected-note@*:* {{-allow-unwritable-changes}}
 
 // The "../base_subdir_partial_defn.h" path is testing two former base dir
 // matching bugs from
diff --git a/clang/test/3C/macro_rewrite_error.c b/clang/test/3C/macro_rewrite_error.c
@@ -3,6 +3,8 @@
 #define args ();
 typedef int (*a) args // expected-error {{Unable to rewrite converted source range. Intended rewriting: "typedef _Ptr<int (void)> a"}}
 a b;
+// expected-note@*:* {{-allow-rewrite-failures}}
 
 #define MIDDLE x; int *
 int MIDDLE y; // expected-error {{Unable to rewrite converted source range. Intended rewriting: "_Ptr<int> y = ((void *)0)"}}
+// expected-note@*:* {{-allow-rewrite-failures}}
diff --git a/clang/test/3C/stdout_mode_write_other.c b/clang/test/3C/stdout_mode_write_other.c
@@ -5,7 +5,8 @@
 // RUN: 3c -base-dir=%S -addcr -verify %s --
 
 // expected-error@base_subdir_partial_defn.h:1 {{3C generated changes to this file, which is under the base dir but is not the main file and thus cannot be written in stdout mode}}
-// expected-note@base_subdir_partial_defn.h:1 {{-dump-unwritable-changes}}
+// expected-note@*:* {{-dump-unwritable-changes}}
+// expected-note@*:* {{-allow-unwritable-changes}}
 
 void
 #include "base_subdir_partial_defn.h"
diff --git a/clang/tools/3c/3CStandalone.cpp b/clang/tools/3c/3CStandalone.cpp
@@ -226,6 +226,18 @@ static cl::opt<bool> OptAllowUnwritableChanges(
              "may be removed in the future."),
     cl::init(false), cl::cat(_3CCategory));
 
+static cl::opt<bool> OptAllowRewriteFailures(
+    "allow-rewrite-failures",
+    cl::desc("When 3c fails to make a rewrite to a source file (typically "
+             "because of macros), issue a warning instead of an error. This "
+             "option is intended to be used temporarily until you change your "
+             "code to allow 3c to work or you report the problem to the 3C "
+             "team to get it fixed; the option may be removed in the future. "
+             "Note that some kinds of rewrite failures currently generate "
+             "warnings regardless of this option, due to known bugs that "
+             "affect common use cases."),
+    cl::init(false), cl::cat(_3CCategory));
+
 #ifdef FIVE_C
 static cl::opt<bool> OptRemoveItypes(
     "remove-itypes",
@@ -272,6 +284,7 @@ int main(int argc, const char **argv) {
   CcOptions.VerifyDiagnosticOutput = OptVerifyDiagnosticOutput;
   CcOptions.DumpUnwritableChanges = OptDumpUnwritableChanges;
   CcOptions.AllowUnwritableChanges = OptAllowUnwritableChanges;
+  CcOptions.AllowRewriteFailures = OptAllowRewriteFailures;
 
 #ifdef FIVE_C
   CcOptions.RemoveItypes = OptRemoveItypes;
@@ -335,7 +348,7 @@ int main(int argc, const char **argv) {
 
   // Write all the converted files back.
   if (!_3CInterface.writeAllConvertedFilesToDisk()) {
-    errs() << "Failure occurred while trying to rewrite converted files back."
+    errs() << "Failure occurred while trying to rewrite converted files back. "
               "Exiting.\n";
     return 1;
   }
