Skip to content

Commit

Permalink
[analyzer] FixItHint: Apply and test hints with the Clang-Tidy's script
Browse files Browse the repository at this point in the history
Summary:
This patch introduces a way to apply the fix-its by the Analyzer:
`-analyzer-config apply-fixits=true`.

The fix-its should be testable, therefore I have copied the well-tested
`check_clang_tidy.py` script. The idea is that the Analyzer's workflow
is different so it would be very difficult to use only one script for
both Tidy and the Analyzer, the script would diverge a lot.
Example test: `// RUN: %check-analyzer-fixit %s %t -analyzer-checker=core`

When the copy-paste happened the original authors were:
@alexfh, @zinovy.nis, @JonasToth, @hokein, @gribozavr, @lebedev.ri

Reviewed By: NoQ, alexfh, zinovy.nis

Differential Revision: https://reviews.llvm.org/D69746
  • Loading branch information
Charusso committed Mar 4, 2020
1 parent cac0686 commit f69c74d
Show file tree
Hide file tree
Showing 7 changed files with 211 additions and 27 deletions.
4 changes: 4 additions & 0 deletions clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
Expand Up @@ -310,6 +310,10 @@ ANALYZER_OPTION(bool, ShouldEmitFixItHintsAsRemarks, "fixits-as-remarks",
"Emit fix-it hints as remarks for testing purposes",
false)

ANALYZER_OPTION(bool, ShouldApplyFixIts, "apply-fixits",
"Apply the fix-it hints to the files",
false)

//===----------------------------------------------------------------------===//
// Unsigned analyzer options.
//===----------------------------------------------------------------------===//
Expand Down
90 changes: 64 additions & 26 deletions clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
Expand Up @@ -12,7 +12,6 @@

#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
#include "ModelInjector.h"
#include "clang/Analysis/PathDiagnostic.h"
#include "clang/AST/Decl.h"
#include "clang/AST/DeclCXX.h"
#include "clang/AST/DeclObjC.h"
Expand All @@ -21,10 +20,12 @@
#include "clang/Analysis/CFG.h"
#include "clang/Analysis/CallGraph.h"
#include "clang/Analysis/CodeInjector.h"
#include "clang/Analysis/PathDiagnostic.h"
#include "clang/Basic/SourceManager.h"
#include "clang/CrossTU/CrossTranslationUnit.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Rewrite/Core/Rewriter.h"
#include "clang/StaticAnalyzer/Checkers/LocalCheckers.h"
#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
Expand All @@ -33,6 +34,8 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
#include "clang/StaticAnalyzer/Frontend/CheckerRegistration.h"
#include "clang/Tooling/Core/Replacement.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/PostOrderIterator.h"
#include "llvm/ADT/Statistic.h"
#include "llvm/Support/FileSystem.h"
Expand All @@ -46,6 +49,7 @@

using namespace clang;
using namespace ento;
using namespace tooling;

#define DEBUG_TYPE "AnalysisConsumer"

Expand Down Expand Up @@ -84,11 +88,16 @@ void ento::createTextPathDiagnosticConsumer(
namespace {
class ClangDiagPathDiagConsumer : public PathDiagnosticConsumer {
DiagnosticsEngine &Diag;
bool IncludePath = false, ShouldEmitAsError = false, FixitsAsRemarks = false;
LangOptions LO;

bool IncludePath = false;
bool ShouldEmitAsError = false;
bool FixitsAsRemarks = false;
bool ApplyFixIts = false;

public:
ClangDiagPathDiagConsumer(DiagnosticsEngine &Diag)
: Diag(Diag) {}
ClangDiagPathDiagConsumer(DiagnosticsEngine &Diag, LangOptions LO)
: Diag(Diag), LO(LO) {}
~ClangDiagPathDiagConsumer() override {}
StringRef getName() const override { return "ClangDiags"; }

Expand All @@ -102,6 +111,7 @@ class ClangDiagPathDiagConsumer : public PathDiagnosticConsumer {
void enablePaths() { IncludePath = true; }
void enableWerror() { ShouldEmitAsError = true; }
void enableFixitsAsRemarks() { FixitsAsRemarks = true; }
void enableApplyFixIts() { ApplyFixIts = true; }

void FlushDiagnosticsImpl(std::vector<const PathDiagnostic *> &Diags,
FilesMade *filesMade) override {
Expand All @@ -111,29 +121,44 @@ class ClangDiagPathDiagConsumer : public PathDiagnosticConsumer {
: Diag.getCustomDiagID(DiagnosticsEngine::Warning, "%0");
unsigned NoteID = Diag.getCustomDiagID(DiagnosticsEngine::Note, "%0");
unsigned RemarkID = Diag.getCustomDiagID(DiagnosticsEngine::Remark, "%0");
SourceManager &SM = Diag.getSourceManager();

Replacements Repls;
auto reportPiece = [&](unsigned ID, FullSourceLoc Loc, StringRef String,
ArrayRef<SourceRange> Ranges,
ArrayRef<FixItHint> Fixits) {
if (!FixitsAsRemarks && !ApplyFixIts) {
Diag.Report(Loc, ID) << String << Ranges << Fixits;
return;
}

Diag.Report(Loc, ID) << String << Ranges;
if (FixitsAsRemarks) {
for (const FixItHint &Hint : Fixits) {
llvm::SmallString<128> Str;
llvm::raw_svector_ostream OS(Str);
// FIXME: Add support for InsertFromRange and
// BeforePreviousInsertion.
assert(!Hint.InsertFromRange.isValid() && "Not implemented yet!");
assert(!Hint.BeforePreviousInsertions && "Not implemented yet!");
OS << SM.getSpellingColumnNumber(Hint.RemoveRange.getBegin()) << "-"
<< SM.getSpellingColumnNumber(Hint.RemoveRange.getEnd()) << ": '"
<< Hint.CodeToInsert << "'";
Diag.Report(Loc, RemarkID) << OS.str();
}
}

if (ApplyFixIts) {
for (const FixItHint &Hint : Fixits) {
Replacement Repl(SM, Hint.RemoveRange, Hint.CodeToInsert);

auto reportPiece =
[&](unsigned ID, SourceLocation Loc, StringRef String,
ArrayRef<SourceRange> Ranges, ArrayRef<FixItHint> Fixits) {
if (!FixitsAsRemarks) {
Diag.Report(Loc, ID) << String << Ranges << Fixits;
} else {
Diag.Report(Loc, ID) << String << Ranges;
for (const FixItHint &Hint : Fixits) {
SourceManager &SM = Diag.getSourceManager();
llvm::SmallString<128> Str;
llvm::raw_svector_ostream OS(Str);
// FIXME: Add support for InsertFromRange and
// BeforePreviousInsertion.
assert(!Hint.InsertFromRange.isValid() && "Not implemented yet!");
assert(!Hint.BeforePreviousInsertions && "Not implemented yet!");
OS << SM.getSpellingColumnNumber(Hint.RemoveRange.getBegin())
<< "-" << SM.getSpellingColumnNumber(Hint.RemoveRange.getEnd())
<< ": '" << Hint.CodeToInsert << "'";
Diag.Report(Loc, RemarkID) << OS.str();
}
if (llvm::Error Err = Repls.add(Repl)) {
llvm::errs() << "Error applying replacement " << Repl.toString()
<< ": " << Err << "\n";
}
};
}
}
};

for (std::vector<const PathDiagnostic *>::iterator I = Diags.begin(),
E = Diags.end();
Expand Down Expand Up @@ -165,6 +190,16 @@ class ClangDiagPathDiagConsumer : public PathDiagnosticConsumer {
Piece->getString(), Piece->getRanges(), Piece->getFixits());
}
}

if (!ApplyFixIts || Repls.empty())
return;

Rewriter Rewrite(SM, LO);
if (!applyAllReplacements(Repls, Rewrite)) {
llvm::errs() << "An error occured during applying fix-it.\n";
}

Rewrite.overwriteChangedFiles();
}
};
} // end anonymous namespace
Expand Down Expand Up @@ -257,7 +292,7 @@ class AnalysisConsumer : public AnalysisASTConsumer,
if (Opts->AnalysisDiagOpt != PD_NONE) {
// Create the PathDiagnosticConsumer.
ClangDiagPathDiagConsumer *clangDiags =
new ClangDiagPathDiagConsumer(PP.getDiagnostics());
new ClangDiagPathDiagConsumer(PP.getDiagnostics(), PP.getLangOpts());
PathConsumers.push_back(clangDiags);

if (Opts->AnalyzerWerror)
Expand All @@ -266,6 +301,9 @@ class AnalysisConsumer : public AnalysisASTConsumer,
if (Opts->ShouldEmitFixItHintsAsRemarks)
clangDiags->enableFixitsAsRemarks();

if (Opts->ShouldApplyFixIts)
clangDiags->enableApplyFixIts();

if (Opts->AnalysisDiagOpt == PD_TEXT) {
clangDiags->enablePaths();

Expand Down
2 changes: 2 additions & 0 deletions clang/lib/StaticAnalyzer/Frontend/CMakeLists.txt
Expand Up @@ -21,4 +21,6 @@ add_clang_library(clangStaticAnalyzerFrontend
clangLex
clangStaticAnalyzerCheckers
clangStaticAnalyzerCore
clangRewrite
clangToolingCore
)
3 changes: 2 additions & 1 deletion clang/test/Analysis/analyzer-config.c
Expand Up @@ -11,6 +11,7 @@
// CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtExec = 0x04
// CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtRead = 0x01
// CHECK-NEXT: alpha.security.taint.TaintPropagation:Config = ""
// CHECK-NEXT: apply-fixits = false
// CHECK-NEXT: avoid-suppressing-null-argument-paths = false
// CHECK-NEXT: c++-allocator-inlining = true
// CHECK-NEXT: c++-container-inlining = false
Expand Down Expand Up @@ -100,4 +101,4 @@
// CHECK-NEXT: unroll-loops = false
// CHECK-NEXT: widen-loops = false
// CHECK-NEXT: [stats]
// CHECK-NEXT: num-entries = 97
// CHECK-NEXT: num-entries = 98
121 changes: 121 additions & 0 deletions clang/test/Analysis/check-analyzer-fixit.py
@@ -0,0 +1,121 @@
#!/usr/bin/env python
#
#===- check-analyzer-fixit.py - Static Analyzer test helper ---*- python -*-===#
#
# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
# See https://llvm.org/LICENSE.txt for license information.
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
#
#===------------------------------------------------------------------------===#
#
# This file copy-pasted mostly from the Clang-Tidy's 'check_clang_tidy.py'.
#
#===------------------------------------------------------------------------===#

r"""
Clang Static Analyzer test helper
=================================
This script runs the Analyzer in fix-it mode and verify fixes, warnings, notes.
Usage:
check-analyzer-fixit.py <source-file> <temp-file> [analyzer arguments]
Example:
// RUN: %check-analyzer-fixit %s %t -analyzer-checker=core
"""

import argparse
import os
import re
import subprocess
import sys


def write_file(file_name, text):
with open(file_name, 'w') as f:
f.write(text)


def run_test_once(args, extra_args):
input_file_name = args.input_file_name
temp_file_name = args.temp_file_name
clang_analyzer_extra_args = extra_args

file_name_with_extension = input_file_name
_, extension = os.path.splitext(file_name_with_extension)
if extension not in ['.c', '.hpp', '.m', '.mm']:
extension = '.cpp'
temp_file_name = temp_file_name + extension

with open(input_file_name, 'r') as input_file:
input_text = input_file.read()

# Remove the contents of the CHECK lines to avoid CHECKs matching on
# themselves. We need to keep the comments to preserve line numbers while
# avoiding empty lines which could potentially trigger formatting-related
# checks.
cleaned_test = re.sub('// *CHECK-[A-Z0-9\-]*:[^\r\n]*', '//', input_text)
write_file(temp_file_name, cleaned_test)

original_file_name = temp_file_name + ".orig"
write_file(original_file_name, cleaned_test)

try:
builtin_include_dir = subprocess.check_output(
['clang', '-print-file-name=include'], stderr=subprocess.STDOUT)
except subprocess.CalledProcessError as e:
print('Cannot print Clang include directory: ' + e.output.decode())

builtin_include_dir = os.path.normpath(builtin_include_dir)

args = (['clang', '-cc1', '-internal-isystem', builtin_include_dir,
'-nostdsysteminc', '-analyze', '-analyzer-constraints=range',
'-analyzer-config', 'apply-fixits=true']
+ clang_analyzer_extra_args + ['-verify', temp_file_name])

print('Running ' + str(args) + '...')

try:
clang_analyzer_output = \
subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
except subprocess.CalledProcessError as e:
print('Clang Static Analyzer test failed:\n' + e.output.decode())
raise

print('----------------- Clang Static Analyzer output -----------------\n' +
clang_analyzer_output +
'\n--------------------------------------------------------------')

try:
diff_output = subprocess.check_output(
['diff', '-u', original_file_name, temp_file_name],
stderr=subprocess.STDOUT)
except subprocess.CalledProcessError as e:
diff_output = e.output

print('----------------------------- Fixes ----------------------------\n' +
diff_output.decode() +
'\n--------------------------------------------------------------')

try:
subprocess.check_output(
['FileCheck', '-input-file=' + temp_file_name, input_file_name,
'-check-prefixes=CHECK-FIXES', '-strict-whitespace'],
stderr=subprocess.STDOUT)
except subprocess.CalledProcessError as e:
print('FileCheck failed:\n' + e.output.decode())
raise


def main():
parser = argparse.ArgumentParser()
parser.add_argument('input_file_name')
parser.add_argument('temp_file_name')

args, extra_args = parser.parse_known_args()
run_test_once(args, extra_args)


if __name__ == '__main__':
main()
13 changes: 13 additions & 0 deletions clang/test/Analysis/virtualcall-fixit.cpp
@@ -0,0 +1,13 @@
// RUN: %check_analyzer_fixit %s %t \
// RUN: -analyzer-checker=core,optin.cplusplus.VirtualCall \
// RUN: -analyzer-config optin.cplusplus.VirtualCall:ShowFixIts=true

struct S {
virtual void foo();
S() {
foo();
// expected-warning@-1 {{Call to virtual method 'S::foo' during construction bypasses virtual dispatch}}
// CHECK-FIXES: S::foo();
}
~S();
};
5 changes: 5 additions & 0 deletions clang/test/lit.cfg.py
Expand Up @@ -77,6 +77,11 @@
if config.clang_staticanalyzer_z3 == '1':
config.available_features.add('z3')

check_analyzer_fixit_path = os.path.join(
config.test_source_root, "Analysis", "check-analyzer-fixit.py")
config.substitutions.append(
('%check_analyzer_fixit',
'%s %s' % (config.python_executable, check_analyzer_fixit_path)))

llvm_config.add_tool_substitutions(tools, tool_dirs)

Expand Down

0 comments on commit f69c74d

Please sign in to comment.