Assert on conflicting fixes for -fix option

[Ka-Ka]

Summary

Changed the assert and corresponding warning message for conflicting fixes
to only trigger if clang-tidy is run with -fix or -fix-errors.
The Fix and FixErrors variables are now passed as parameters when creating
respective objects.

This is not a finished patch. We need input if there are a more appropriate fix of this problem.

Patch by: Henric Karlsson and Björn Pettersson

lib/StaticAnalyzer/Core/ExprEngineC.cpp

void ExprEngine::VisitLogicalExpr(const BinaryOperator* B, ExplodedNode *Pred,
 
   ExplodedNode *N = Pred;
   while (!N->getLocation().getAs<BlockEntrance>()) {
     ProgramPoint P = N->getLocation();
     assert(P.getAs<PreStmt>()|| P.getAs<PreStmtPurgeDeadSymbols>());
     (void) P;
     assert(N->pred_size() == 1);
     N = *N->pred_begin();
+    if (N->pred_size() != 1) {
+      // FIXME: The CFG-based reasoning below does not apply.
+      // Do not set the value for the expression. It'd be UnknownVal by default.
+      Bldr.generateNode(B, Pred, state);
+      return;
+    }
   }
   assert(N->pred_size() == 1);
   N = *N->pred_begin();
   BlockEdge BE = N->getLocation().castAs<BlockEdge>();
   SVal X;
 
   // Determine the value of the expression by introspecting how we
   // got this location in the CFG.  This requires looking at the previous

tools/extra/clang-tidy/ClangTidy.h

 /// \param StoreCheckProfile If provided, and EnableCheckProfile is true,
 /// the profile will not be output to stderr, but will instead be stored
 /// as a JSON file in the specified directory.
 std::vector<ClangTidyError>
 runClangTidy(clang::tidy::ClangTidyContext &Context,
              const tooling::CompilationDatabase &Compilations,
              ArrayRef<std::string> InputFiles,
              llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS,
-             bool EnableCheckProfile = false,
+             bool FixResolutions, bool EnableCheckProfile = false,
              llvm::StringRef StoreCheckProfile = StringRef());
 
 // FIXME: This interface will need to be significantly extended to be useful.
 // FIXME: Implement confidence levels for displaying/fixing errors.
 //
 /// \brief Displays the found \p Errors to the users. If \p Fix is true, \p
 /// Errors containing fixes are automatically applied and reformatted. If no
 /// clang-format configuration file is found, the given \P FormatStyle is used.

tools/extra/clang-tidy/ClangTidy.cpp

getCheckOptions(const ClangTidyOptions &Options,
   return Factory.getCheckOptions();
 }
 
 std::vector<ClangTidyError>
 runClangTidy(clang::tidy::ClangTidyContext &Context,
              const CompilationDatabase &Compilations,
              ArrayRef<std::string> InputFiles,
              llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS,
-             bool EnableCheckProfile, llvm::StringRef StoreCheckProfile) {
+             bool FixResolutions, bool EnableCheckProfile,
+             llvm::StringRef StoreCheckProfile) {
   ClangTool Tool(Compilations, InputFiles,
                  std::make_shared<PCHContainerOperations>(), BaseFS);
 
   // Add extra arguments passed by the clang-tidy command-line.
   ArgumentsAdjuster PerFileExtraArgumentsInserter =
       [&Context](const CommandLineArguments &Args, StringRef Filename) {
         ClangTidyOptions Opts = Context.getOptionsForFile(Filename);
         CommandLineArguments AdjustedArgs = Args;
ArgumentsAdjuster PluginArgumentsRemover =
         return AdjustedArgs;
       };
 
   Tool.appendArgumentsAdjuster(PerFileExtraArgumentsInserter);
   Tool.appendArgumentsAdjuster(PluginArgumentsRemover);
   Context.setEnableProfiling(EnableCheckProfile);
   Context.setProfileStoragePrefix(StoreCheckProfile);
 
-  ClangTidyDiagnosticConsumer DiagConsumer(Context);
+  ClangTidyDiagnosticConsumer DiagConsumer(Context, true, FixResolutions);
   DiagnosticsEngine DE(new DiagnosticIDs(), new DiagnosticOptions(),
                        &DiagConsumer, /*ShouldOwnClient=*/false);
   Context.setDiagnosticsEngine(&DE);
   Tool.setDiagnosticConsumer(&DiagConsumer);
 
   class ActionFactory : public FrontendActionFactory {
   public:
     ActionFactory(ClangTidyContext &Context) : ConsumerFactory(Context) {}

tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.h

 /// \brief A diagnostic consumer that turns each \c Diagnostic into a
 /// \c SourceManager-independent \c ClangTidyError.
 //
 // FIXME: If we move away from unit-tests, this can be moved to a private
 // implementation file.
 class ClangTidyDiagnosticConsumer : public DiagnosticConsumer {
 public:
   ClangTidyDiagnosticConsumer(ClangTidyContext &Ctx,
-                              bool RemoveIncompatibleErrors = true);
+                              bool RemoveIncompatibleErrors = true,
+                              bool FixResolutions = false);
 
   // FIXME: The concept of converting between FixItHints and Replacements is
   // more generic and should be pulled out into a more useful Diagnostics
   // library.
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const Diagnostic &Info) override;
 
   // Retrieve the diagnostics that were captured.
private:
 
   /// \brief Updates \c LastErrorRelatesToUserCode and LastErrorPassesLineFilter
   /// according to the diagnostic \p Location.
   void checkFilters(SourceLocation Location, const SourceManager& Sources);
   bool passesLineFilter(StringRef FileName, unsigned LineNumber) const;
 
   ClangTidyContext &Context;
   bool RemoveIncompatibleErrors;
+  bool FixResolutions;
   std::vector<ClangTidyError> Errors;
   std::unique_ptr<llvm::Regex> HeaderFilter;
   bool LastErrorRelatesToUserCode;
   bool LastErrorPassesLineFilter;
   bool LastErrorWasIgnored;
 };
 
 } // end namespace tidy
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYDIAGNOSTICCONSUMER_H

tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

 using namespace clang;
 using namespace tidy;
 
 namespace {
 class ClangTidyDiagnosticRenderer : public DiagnosticRenderer {
 public:
   ClangTidyDiagnosticRenderer(const LangOptions &LangOpts,
                               DiagnosticOptions *DiagOpts,
-                              ClangTidyError &Error)
-      : DiagnosticRenderer(LangOpts, DiagOpts), Error(Error) {}
+                              ClangTidyError &Error,
+                              bool FixResolutions)
+      : DiagnosticRenderer(LangOpts, DiagOpts), Error(Error),
+        FixResolutions(FixResolutions) {}
 
 protected:
   void emitDiagnosticMessage(FullSourceLoc Loc, PresumedLoc PLoc,
                              DiagnosticsEngine::Level Level, StringRef Message,
                              ArrayRef<CharSourceRange> Ranges,
                              DiagOrStoredDiag Info) override {
     // Remove check name from the message.
     // FIXME: Remove this once there's a better way to pass check names than
for (const auto &FixIt : Hints) {
       assert(Range.getBegin().isFileID() && Range.getEnd().isFileID() &&
              "Only file locations supported in fix-it hints.");
 
       tooling::Replacement Replacement(Loc.getManager(), Range,
                                        FixIt.CodeToInsert);
       llvm::Error Err = Error.Fix[Replacement.getFilePath()].add(Replacement);
       // FIXME: better error handling (at least, don't let other replacements be
       // applied).
-      if (Err) {
+      if (Err && FixResolutions) {
         llvm::errs() << "Fix conflicts with existing fix! "
                      << llvm::toString(std::move(Err)) << "\n";
         assert(false && "Fix conflicts with existing fix!");
+      } else if (Err) {
+        // Replacements wont be used for fixing anything.
+        consumeError(std::move(Err));
       }
     }
   }
 
   void emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) override {}
 
   void emitImportLocation(FullSourceLoc Loc, PresumedLoc PLoc,
                           StringRef ModuleName) override {}
 
   void emitBuildingModuleLocation(FullSourceLoc Loc, PresumedLoc PLoc,
                                   StringRef ModuleName) override {}
 
   void endDiagnostic(DiagOrStoredDiag D,
                      DiagnosticsEngine::Level Level) override {
     assert(!Error.Message.Message.empty() && "Message has not been set");
   }
 
 private:
   ClangTidyError &Error;
+  bool FixResolutions;
 };
 } // end anonymous namespace
 
 ClangTidyError::ClangTidyError(StringRef CheckName,
                                ClangTidyError::Level DiagLevel,
                                StringRef BuildDirectory, bool IsWarningAsError)
     : tooling::Diagnostic(CheckName, DiagLevel, BuildDirectory),
       IsWarningAsError(IsWarningAsError) {}
StringRef ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
   llvm::DenseMap<unsigned, std::string>::const_iterator I =
       CheckNamesByDiagnosticID.find(DiagnosticID);
   if (I != CheckNamesByDiagnosticID.end())
     return I->second;
   return "";
 }
 
 ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
-    ClangTidyContext &Ctx, bool RemoveIncompatibleErrors)
+    ClangTidyContext &Ctx, bool RemoveIncompatibleErrors , bool FixResolutions )
     : Context(Ctx), RemoveIncompatibleErrors(RemoveIncompatibleErrors),
-      LastErrorRelatesToUserCode(false), LastErrorPassesLineFilter(false),
-      LastErrorWasIgnored(false) {}
+      FixResolutions(FixResolutions), LastErrorRelatesToUserCode(false),
+      LastErrorPassesLineFilter(false), LastErrorWasIgnored(false) {}
 
 void ClangTidyDiagnosticConsumer::finalizeLastError() {
   if (!Errors.empty()) {
     ClangTidyError &Error = Errors.back();
     if (!Context.isCheckEnabled(Error.DiagnosticName) &&
         Error.DiagLevel != ClangTidyError::Error) {
       ++Context.Stats.ErrorsIgnoredCheckFilter;
       Errors.pop_back();
if (DiagLevel == DiagnosticsEngine::Note) {
     bool IsWarningAsError = DiagLevel == DiagnosticsEngine::Warning &&
                             Context.treatAsError(CheckName);
     Errors.emplace_back(CheckName, Level, Context.getCurrentBuildDirectory(),
                         IsWarningAsError);
   }
 
   ClangTidyDiagnosticRenderer Converter(
       Context.getLangOpts(), &Context.DiagEngine->getDiagnosticOptions(),
-      Errors.back());
+      Errors.back(), FixResolutions);
   SmallString<100> Message;
   Info.FormatDiagnostic(Message);
   FullSourceLoc Loc;
   if (Info.getLocation().isValid() && Info.hasSourceManager())
     Loc = FullSourceLoc(Info.getLocation(), Info.getSourceManager());
   Converter.emitDiagnostic(Loc, DiagLevel, Message, Info.getRanges(),
                            Info.getFixItHints());
 

tools/extra/clang-tidy/tool/ClangTidyMain.cpp

static int clangTidyMain(int argc, const char **argv) {
   llvm::InitializeAllTargetInfos();
   llvm::InitializeAllTargetMCs();
   llvm::InitializeAllAsmParsers();
 
   ClangTidyContext Context(std::move(OwningOptionsProvider),
                            AllowEnablingAnalyzerAlphaCheckers);
   std::vector<ClangTidyError> Errors =
       runClangTidy(Context, OptionsParser.getCompilations(), PathList, BaseFS,
-                   EnableCheckProfile, ProfilePrefix);
+                   (Fix || FixErrors), EnableCheckProfile, ProfilePrefix);
   bool FoundErrors = llvm::find_if(Errors, [](const ClangTidyError &E) {
                        return E.DiagLevel == ClangTidyError::Error;
                      }) != Errors.end();
 
   const bool DisableFixes = Fix && FoundErrors && !FixErrors;
 
   unsigned WErrorCount = 0;
 

tools/extra/test/clang-tidy/clang-tidy-logicalexpr-crash.c

+// RUN: clang-tidy -checks='-*,clang-analyzer-*' %s
+
+a;
+fn1() {
+  int b = -a ?: a;
+  (b || 0) && 9;
+}