diff --git a/common/analysis/BUILD b/common/analysis/BUILD index c7bf1da30..65902a6cf 100644 --- a/common/analysis/BUILD +++ b/common/analysis/BUILD @@ -9,6 +9,7 @@ package( "//verilog/CST:__subpackages__", "//verilog/analysis:__subpackages__", "//verilog/tools/kythe:__pkg__", + "//verilog/tools/lint:__subpackages__", ], ) @@ -84,6 +85,20 @@ cc_library( ], ) +cc_library( + name = "violation_handler", + srcs = ["violation_handler.cc"], + hdrs = ["violation_handler.h"], + deps = [ + ":lint_rule_status", + "//common/strings:diff", + "//common/util:file_util", + "//common/util:user_interaction", + "@com_google_absl//absl/status", + "@com_google_absl//absl/strings", + ] +) + cc_test( name = "command_file_lexer_test", size = "small", diff --git a/common/analysis/lint_rule_status.cc b/common/analysis/lint_rule_status.cc index da332bcd4..6ab56dc5b 100644 --- a/common/analysis/lint_rule_status.cc +++ b/common/analysis/lint_rule_status.cc @@ -92,19 +92,6 @@ void LintStatusFormatter::FormatLintRuleStatus(std::ostream* stream, } } -struct LintViolationWithStatus { - const LintViolation* violation; - const LintRuleStatus* status; - - LintViolationWithStatus(const LintViolation* v, const LintRuleStatus* s) - : violation(v), status(s) {} - - bool operator<(const LintViolationWithStatus& r) const { - // compares addresses which correspond to locations within the same string - return violation->token.text().data() < r.violation->token.text().data(); - } -}; - void LintStatusFormatter::FormatLintRuleStatuses( std::ostream* stream, const std::vector& statuses, absl::string_view base, absl::string_view path, diff --git a/common/analysis/lint_rule_status.h b/common/analysis/lint_rule_status.h index 8739f546f..da5d29178 100644 --- a/common/analysis/lint_rule_status.h +++ b/common/analysis/lint_rule_status.h @@ -179,6 +179,19 @@ struct LintRuleStatus { std::set violations; }; +struct LintViolationWithStatus { + const LintViolation* violation; + const LintRuleStatus* status; + + LintViolationWithStatus(const LintViolation* v, const LintRuleStatus* s) + : violation(v), status(s) {} + + bool operator<(const LintViolationWithStatus& r) const { + // compares addresses which correspond to locations within the same string + return violation->token.text().data() < r.violation->token.text().data(); + } +}; + // LintStatusFormatter is a class for printing LintRuleStatus's and // LintViolations to an output stream // Usage: diff --git a/common/analysis/violation_handler.cc b/common/analysis/violation_handler.cc new file mode 100644 index 000000000..b80f64067 --- /dev/null +++ b/common/analysis/violation_handler.cc @@ -0,0 +1,248 @@ +// Copyright 2017-2021 The Verible Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "common/analysis/violation_handler.h" + +#include "absl/status/status.h" +#include "common/strings/diff.h" +#include "common/util/file_util.h" +#include "common/util/user_interaction.h" + +namespace verible { +namespace { + +void PrintFix(std::ostream& stream, absl::string_view text, + const verible::AutoFix& fix) { + std::string after = fix.Apply(text); + verible::LineDiffs diff(text, after); + + verible::LineDiffsToUnifiedDiff(stream, diff, 1); +} + +void PrintFixAlternatives(std::ostream& stream, absl::string_view text, + const std::vector& fixes) { + const bool print_alternative_number = fixes.size() > 1; + for (size_t i = 0; i < fixes.size(); ++i) { + if (print_alternative_number) { + stream << verible::term::inverse(absl::StrCat( + "[ ", (i + 1), ". Alternative ", fixes[i].Description(), " ]\n")); + } else { + stream << verible::term::inverse( + absl::StrCat("[ ", fixes[i].Description(), " ]\n")); + } + PrintFix(stream, text, fixes[i]); + } +} + +} // namespace + +void ViolationPrinter::HandleViolations( + const std::set& violations, absl::string_view base, + absl::string_view path) { + verible::LintStatusFormatter formatter(base); + for (auto violation : violations) { + formatter.FormatViolation(stream_, *violation.violation, base, path, + violation.status->url, + violation.status->lint_rule_name); + (*stream_) << std::endl; + } +} + +void ViolationFixer::CommitFixes(absl::string_view source_content, + absl::string_view source_path, + const verible::AutoFix& fix) const { + if (fix.Edits().empty()) { + return; + } + std::string fixed_content = fix.Apply(source_content); + + if (patch_stream_) { + verible::LineDiffs diff(source_content, fixed_content); + verible::LineDiffsToUnifiedDiff(*patch_stream_, diff, 1, source_path); + } else { + const absl::Status write_status = + verible::file::SetContents(source_path, fixed_content); + if (!write_status.ok()) { + LOG(ERROR) << "Failed to write fixes to file '" << source_path + << "': " << write_status.ToString(); + return; + } + } +} + +void ViolationFixer::HandleViolations( + const std::set& violations, absl::string_view base, + absl::string_view path) { + verible::AutoFix fix; + verible::LintStatusFormatter formatter(base); + for (auto violation : violations) { + HandleViolation(*violation.violation, base, path, violation.status->url, + violation.status->lint_rule_name, formatter, &fix); + } + + CommitFixes(base, path, fix); +} + +void ViolationFixer::HandleViolation( + const verible::LintViolation& violation, absl::string_view base, + absl::string_view path, absl::string_view url, absl::string_view rule_name, + const verible::LintStatusFormatter& formatter, verible::AutoFix* fix) { + std::stringstream violation_message; + formatter.FormatViolation(&violation_message, violation, base, path, url, + rule_name); + (*message_stream_) << violation_message.str() << std::endl; + + if (violation.autofixes.empty()) { + return; + } + + static std::string_view previous_fix_conflict = + "The fix conflicts with " + "previously applied fixes, rejecting.\n"; + + Answer answer; + for (bool first_round = true; /**/; first_round = false) { + if (ultimate_answer_.choice != AnswerChoice::kUnknown) { + answer = ultimate_answer_; + } else if (auto found = rule_answers_.find(rule_name); + found != rule_answers_.end()) { + answer = found->second; + // If the ApplyAll specifies alternative not available here, use first. + if (answer.alternative >= violation.autofixes.size()) { + answer.alternative = 0; + } + } else { + if (is_interactive_ && first_round) { // Show the user what is available. + PrintFixAlternatives(*message_stream_, base, violation.autofixes); + } + answer = answer_chooser_(violation, rule_name); + } + + switch (answer.choice) { + case AnswerChoice::kApplyAll: + ultimate_answer_ = {AnswerChoice::kApply}; + [[fallthrough]]; + case AnswerChoice::kApplyAllForRule: + rule_answers_[rule_name] = {AnswerChoice::kApply, answer.alternative}; + [[fallthrough]]; + case AnswerChoice::kApply: // Apply fix chosen in the alternatative + if (answer.alternative >= violation.autofixes.size()) + continue; // ask again. + if (!fix->AddEdits(violation.autofixes[answer.alternative].Edits())) { + *message_stream_ << previous_fix_conflict; + } + break; + case AnswerChoice::kRejectAll: + ultimate_answer_ = {AnswerChoice::kReject}; + [[fallthrough]]; + case AnswerChoice::kRejectAllForRule: + rule_answers_[rule_name] = {AnswerChoice::kReject}; + [[fallthrough]]; + case AnswerChoice::kReject: + return; + + case AnswerChoice::kPrintFix: + PrintFixAlternatives(*message_stream_, base, violation.autofixes); + continue; + case AnswerChoice::kPrintAppliedFixes: + PrintFix(*message_stream_, base, *fix); + continue; + + default: + continue; + } + + break; + } +} + +ViolationFixer::Answer ViolationFixer::InteractiveAnswerChooser( + const verible::LintViolation& violation, absl::string_view rule_name) { + static absl::string_view fixed_help_message = + "n - reject fix\n" + "a - apply this and all remaining fixes for violations of this rule\n" + "d - reject this and all remaining fixes for violations of this rule\n" + "A - apply this and all remaining fixes\n" + "D - reject this and all remaining fixes\n" + "p - show fix\n" + "P - show fixes applied in this file so far\n" + "? - print this help and prompt again\n"; + + const size_t fix_count = violation.autofixes.size(); + std::string help_message; + std::string alternative_list; // Show alternatives in the short-menu. + if (fix_count > 1) { + help_message = + absl::StrCat("y - apply first fix\n[1-", fix_count, + "] - apply given alternative\n", fixed_help_message); + for (size_t i = 0; i < fix_count; ++i) + absl::StrAppend(&alternative_list, (i + 1), ","); + } else { + help_message = absl::StrCat("y - apply fix\n", fixed_help_message); + } + + for (;;) { + const char c = verible::ReadCharFromUser( + std::cin, std::cerr, verible::IsInteractiveTerminalSession(), + verible::term::bold("Autofix is available. Apply? [" + + alternative_list + "y,n,a,d,A,D,p,P,?] ")); + + // Single character digit chooses the available alternative. + if (c >= '1' && c <= '9' && + c < static_cast('1' + violation.autofixes.size())) { + return {AnswerChoice::kApply, static_cast(c - '1')}; + } + + switch (c) { + case 'y': + return {AnswerChoice::kApply, 0}; + + // TODO(hzeller): Should we provide a way to choose 'all for rule' + // including an alternative ? Maybe with a two-letter response + // such as 1a, 2a, 3a ? Current assumption of interaction is + // single character. + case 'a': + return {AnswerChoice::kApplyAllForRule}; + case 'A': + return {AnswerChoice::kApplyAll}; // No alternatives + case 'n': + return {AnswerChoice::kReject}; + case 'd': + return {AnswerChoice::kRejectAllForRule}; + case 'D': + return {AnswerChoice::kRejectAll}; + + case '\0': + // EOF: received when too few "answers" have been piped to stdin. + std::cerr << "Received EOF while there are questions left. " + << "Rejecting all remaining fixes." << std::endl; + return {AnswerChoice::kRejectAll}; + + case 'p': + return {AnswerChoice::kPrintFix}; + case 'P': + return {AnswerChoice::kPrintAppliedFixes}; + + case '\n': + continue; + + case '?': + default: + std::cerr << help_message << std::endl; + continue; + } + } +} + +} // namespace verible diff --git a/common/analysis/violation_handler.h b/common/analysis/violation_handler.h new file mode 100644 index 000000000..000eab8ca --- /dev/null +++ b/common/analysis/violation_handler.h @@ -0,0 +1,151 @@ +// Copyright 2017-2021 The Verible Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef VERIBLE_COMMON_ANALYSIS_VIOLATION_HANDLER_H_ +#define VERIBLE_COMMON_ANALYSIS_VIOLATION_HANDLER_H_ + +#include +#include +#include +#include + +#include "absl/strings/string_view.h" +#include "common/analysis/lint_rule_status.h" + +namespace verible { + +// Interface for implementing violation handlers. +// +// The linting process produces a list of violations found in source code. Those +// violations are then sorted and passed to `HandleViolations()` method of an +// instance passed to LintOneFile(). +class ViolationHandler { + public: + virtual ~ViolationHandler() = default; + + // This method is called with a list of sorted violations found in file + // located at `path`. It can be called multiple times with statuses generated + // from different files. `base` contains source code from the file. + virtual void HandleViolations( + const std::set& violations, + absl::string_view base, absl::string_view path) = 0; +}; + +// ViolationHandler that prints all violations in a form of user-friendly +// messages. +class ViolationPrinter : public ViolationHandler { + public: + explicit ViolationPrinter(std::ostream* stream) + : stream_(stream), formatter_(nullptr) {} + + void HandleViolations( + const std::set& violations, + absl::string_view base, absl::string_view path) final; + + protected: + std::ostream* const stream_; + verible::LintStatusFormatter* formatter_; +}; + +// ViolationHandler that prints all violations and gives an option to fix those +// that have autofixes available. +// +// By default, when violation has an autofix available, ViolationFixer asks an +// user what to do. The answers can be provided by AnswerChooser callback passed +// to the constructor as the answer_chooser parameter. The callback is called +// once for each fixable violation with a current violation object and a +// violated rule name as arguments, and must return one of the values from +// AnswerChoice enum. +// +// When the constructor's patch_stream parameter is not null, the fixes are +// written to specified stream in unified diff format. Otherwise the fixes are +// applied directly to the source file. +// +// The HandleLintRuleStatuses method can be called multiple times with statuses +// generated from different files. The state of answers like "apply all for +// rule" or "apply all" is kept between the calls. +class ViolationFixer : public verible::ViolationHandler { + public: + enum class AnswerChoice { + kUnknown, + kApply, // apply fix + kReject, // reject fix + kApplyAllForRule, // apply this and all remaining fixes for violations + // of this rule + kRejectAllForRule, // reject this and all remaining fixes for violations + // of this rule + kApplyAll, // apply this and all remaining fixes + kRejectAll, // reject this and all remaining fixes + kPrintFix, // show fix + kPrintAppliedFixes, // show fixes applied so far + }; + + struct Answer { + AnswerChoice choice; + // If there are multiple alternatives for fixes available, this is + // the one chosen. By default the first one. + size_t alternative = 0; + }; + + using AnswerChooser = + std::function; + + // Violation fixer with user-chosen answer chooser. + ViolationFixer(std::ostream* message_stream, std::ostream* patch_stream, + const AnswerChooser& answer_chooser) + : ViolationFixer(message_stream, patch_stream, answer_chooser, false) {} + + // Violation fixer with interactive answer choice. + ViolationFixer(std::ostream* message_stream, std::ostream* patch_stream) + : ViolationFixer(message_stream, patch_stream, InteractiveAnswerChooser, + true) {} + + void HandleViolations( + const std::set& violations, + absl::string_view base, absl::string_view path) final; + + private: + ViolationFixer(std::ostream* message_stream, std::ostream* patch_stream, + const AnswerChooser& answer_chooser, bool is_interactive) + : message_stream_(message_stream), + patch_stream_(patch_stream), + answer_chooser_(answer_chooser), + is_interactive_(is_interactive), + ultimate_answer_({AnswerChoice::kUnknown, 0}) {} + + void HandleViolation(const verible::LintViolation& violation, + absl::string_view base, absl::string_view path, + absl::string_view url, absl::string_view rule_name, + const verible::LintStatusFormatter& formatter, + verible::AutoFix* fix); + + static Answer InteractiveAnswerChooser( + const verible::LintViolation& violation, absl::string_view rule_name); + + void CommitFixes(absl::string_view source_content, + absl::string_view source_path, + const verible::AutoFix& fix) const; + + std::ostream* const message_stream_; + std::ostream* const patch_stream_; + const AnswerChooser answer_chooser_; + const bool is_interactive_; + + Answer ultimate_answer_; + std::map rule_answers_; +}; + +} // namespace verible + +#endif // VERIBLE_COMMON_ANALYSIS_VIOLATION_HANDLER_H_ diff --git a/verilog/analysis/BUILD b/verilog/analysis/BUILD index c45106264..889f7e328 100644 --- a/verilog/analysis/BUILD +++ b/verilog/analysis/BUILD @@ -227,14 +227,13 @@ cc_library( "//common/analysis:text_structure_linter", "//common/analysis:token_stream_lint_rule", "//common/analysis:token_stream_linter", - "//common/strings:diff", + "//common/analysis:violation_handler", "//common/strings:line_column_map", "//common/text:concrete_syntax_tree", "//common/text:text_structure", "//common/text:token_info", "//common/util:file_util", "//common/util:logging", - "//common/util:user_interaction", "//verilog/parser:verilog_token_classifications", "//verilog/parser:verilog_token_enum", "@com_google_absl//absl/flags:flag", @@ -307,6 +306,7 @@ cc_test( ":verilog_analyzer", ":verilog_linter", ":verilog_linter_configuration", + "//common/analysis:violation_handler", "//common/util:file_util", "//common/util:logging", "@com_google_absl//absl/memory", diff --git a/verilog/analysis/verilog_linter.cc b/verilog/analysis/verilog_linter.cc index bb9e3a7c9..06c22606f 100644 --- a/verilog/analysis/verilog_linter.cc +++ b/verilog/analysis/verilog_linter.cc @@ -39,14 +39,12 @@ #include "common/analysis/text_structure_linter.h" #include "common/analysis/token_stream_lint_rule.h" #include "common/analysis/token_stream_linter.h" -#include "common/strings/diff.h" #include "common/strings/line_column_map.h" #include "common/text/concrete_syntax_tree.h" #include "common/text/text_structure.h" #include "common/text/token_info.h" #include "common/util/file_util.h" #include "common/util/logging.h" -#include "common/util/user_interaction.h" #include "verilog/analysis/default_rules.h" #include "verilog/analysis/lint_rule_registry.h" #include "verilog/analysis/verilog_analyzer.h" @@ -77,6 +75,7 @@ namespace verilog { using verible::LineColumnMap; using verible::LintRuleStatus; +using verible::LintViolationWithStatus; using verible::LintWaiver; using verible::TextStructureView; using verible::TokenInfo; @@ -94,234 +93,13 @@ std::set GetSortedViolations( return violations; } -void ViolationPrinter::HandleViolations( - const std::set& violations, absl::string_view base, - absl::string_view path) { - verible::LintStatusFormatter formatter(base); - for (auto violation : violations) { - formatter.FormatViolation(stream_, *violation.violation, base, path, - violation.status->url, - violation.status->lint_rule_name); - (*stream_) << std::endl; - } -} - -static void PrintFix(std::ostream& stream, absl::string_view text, - const verible::AutoFix& fix) { - std::string after = fix.Apply(text); - verible::LineDiffs diff(text, after); - - verible::LineDiffsToUnifiedDiff(stream, diff, 1); -} - -static void PrintFixAlternatives(std::ostream& stream, absl::string_view text, - const std::vector& fixes) { - const bool print_alternative_number = fixes.size() > 1; - for (size_t i = 0; i < fixes.size(); ++i) { - if (print_alternative_number) { - stream << verible::term::inverse(absl::StrCat( - "[ ", (i + 1), ". Alternative ", fixes[i].Description(), " ]\n")); - } else { - stream << verible::term::inverse( - absl::StrCat("[ ", fixes[i].Description(), " ]\n")); - } - PrintFix(stream, text, fixes[i]); - } -} - -void ViolationFixer::CommitFixes(absl::string_view source_content, - absl::string_view source_path, - const verible::AutoFix& fix) const { - if (fix.Edits().empty()) { - return; - } - std::string fixed_content = fix.Apply(source_content); - - if (patch_stream_) { - verible::LineDiffs diff(source_content, fixed_content); - verible::LineDiffsToUnifiedDiff(*patch_stream_, diff, 1, source_path); - } else { - const absl::Status write_status = - verible::file::SetContents(source_path, fixed_content); - if (!write_status.ok()) { - LOG(ERROR) << "Failed to write fixes to file '" << source_path - << "': " << write_status.ToString(); - return; - } - } -} - -void ViolationFixer::HandleViolations( - const std::set& violations, absl::string_view base, - absl::string_view path) { - verible::AutoFix fix; - verible::LintStatusFormatter formatter(base); - for (auto violation : violations) { - HandleViolation(*violation.violation, base, path, violation.status->url, - violation.status->lint_rule_name, formatter, &fix); - } - - CommitFixes(base, path, fix); -} - -void ViolationFixer::HandleViolation( - const verible::LintViolation& violation, absl::string_view base, - absl::string_view path, absl::string_view url, absl::string_view rule_name, - const verible::LintStatusFormatter& formatter, verible::AutoFix* fix) { - std::stringstream violation_message; - formatter.FormatViolation(&violation_message, violation, base, path, url, - rule_name); - (*message_stream_) << violation_message.str() << std::endl; - - if (violation.autofixes.empty()) { - return; - } - - static std::string_view previous_fix_conflict = - "The fix conflicts with " - "previously applied fixes, rejecting.\n"; - - Answer answer; - for (bool first_round = true; /**/; first_round = false) { - if (ultimate_answer_.choice != AnswerChoice::kUnknown) { - answer = ultimate_answer_; - } else if (auto found = rule_answers_.find(rule_name); - found != rule_answers_.end()) { - answer = found->second; - // If the ApplyAll specifies alternative not available here, use first. - if (answer.alternative >= violation.autofixes.size()) { - answer.alternative = 0; - } - } else { - if (is_interactive_ && first_round) { // Show the user what is available. - PrintFixAlternatives(*message_stream_, base, violation.autofixes); - } - answer = answer_chooser_(violation, rule_name); - } - - switch (answer.choice) { - case AnswerChoice::kApplyAll: - ultimate_answer_ = {AnswerChoice::kApply}; - [[fallthrough]]; - case AnswerChoice::kApplyAllForRule: - rule_answers_[rule_name] = {AnswerChoice::kApply, answer.alternative}; - [[fallthrough]]; - case AnswerChoice::kApply: // Apply fix chosen in the alternatative - if (answer.alternative >= violation.autofixes.size()) - continue; // ask again. - if (!fix->AddEdits(violation.autofixes[answer.alternative].Edits())) { - *message_stream_ << previous_fix_conflict; - } - break; - case AnswerChoice::kRejectAll: - ultimate_answer_ = {AnswerChoice::kReject}; - [[fallthrough]]; - case AnswerChoice::kRejectAllForRule: - rule_answers_[rule_name] = {AnswerChoice::kReject}; - [[fallthrough]]; - case AnswerChoice::kReject: - return; - - case AnswerChoice::kPrintFix: - PrintFixAlternatives(*message_stream_, base, violation.autofixes); - continue; - case AnswerChoice::kPrintAppliedFixes: - PrintFix(*message_stream_, base, *fix); - continue; - - default: - continue; - } - - break; - } -} - -ViolationFixer::Answer ViolationFixer::InteractiveAnswerChooser( - const verible::LintViolation& violation, absl::string_view rule_name) { - static absl::string_view fixed_help_message = - "n - reject fix\n" - "a - apply this and all remaining fixes for violations of this rule\n" - "d - reject this and all remaining fixes for violations of this rule\n" - "A - apply this and all remaining fixes\n" - "D - reject this and all remaining fixes\n" - "p - show fix\n" - "P - show fixes applied in this file so far\n" - "? - print this help and prompt again\n"; - - const size_t fix_count = violation.autofixes.size(); - std::string help_message; - std::string alternative_list; // Show alternatives in the short-menu. - if (fix_count > 1) { - help_message = - absl::StrCat("y - apply first fix\n[1-", fix_count, - "] - apply given alternative\n", fixed_help_message); - for (size_t i = 0; i < fix_count; ++i) - absl::StrAppend(&alternative_list, (i + 1), ","); - } else { - help_message = absl::StrCat("y - apply fix\n", fixed_help_message); - } - - for (;;) { - const char c = verible::ReadCharFromUser( - std::cin, std::cerr, verible::IsInteractiveTerminalSession(), - verible::term::bold("Autofix is available. Apply? [" + - alternative_list + "y,n,a,d,A,D,p,P,?] ")); - - // Single character digit chooses the available alternative. - if (c >= '1' && c <= '9' && - c < static_cast('1' + violation.autofixes.size())) { - return {AnswerChoice::kApply, static_cast(c - '1')}; - } - - switch (c) { - case 'y': - return {AnswerChoice::kApply, 0}; - - // TODO(hzeller): Should we provide a way to choose 'all for rule' - // including an alternative ? Maybe with a two-letter response - // such as 1a, 2a, 3a ? Current assumption of interaction is - // single character. - case 'a': - return {AnswerChoice::kApplyAllForRule}; - case 'A': - return {AnswerChoice::kApplyAll}; // No alternatives - case 'n': - return {AnswerChoice::kReject}; - case 'd': - return {AnswerChoice::kRejectAllForRule}; - case 'D': - return {AnswerChoice::kRejectAll}; - - case '\0': - // EOF: received when too few "answers" have been piped to stdin. - std::cerr << "Received EOF while there are questions left. " - << "Rejecting all remaining fixes." << std::endl; - return {AnswerChoice::kRejectAll}; - - case 'p': - return {AnswerChoice::kPrintFix}; - case 'P': - return {AnswerChoice::kPrintAppliedFixes}; - - case '\n': - continue; - - case '?': - default: - std::cerr << help_message << std::endl; - continue; - } - } -} - // Return code useful to be used in main: // 0: success // 1: linting error (if parse_fatal == true) // 2..: other fatal issues such as file not found. int LintOneFile(std::ostream* stream, absl::string_view filename, const LinterConfiguration& config, - ViolationHandler* violation_handler, bool check_syntax, + verible::ViolationHandler* violation_handler, bool check_syntax, bool parse_fatal, bool lint_fatal, bool show_context) { std::string content; const absl::Status content_status = diff --git a/verilog/analysis/verilog_linter.h b/verilog/analysis/verilog_linter.h index cfbb195ab..8db674793 100644 --- a/verilog/analysis/verilog_linter.h +++ b/verilog/analysis/verilog_linter.h @@ -28,6 +28,7 @@ #include "common/analysis/syntax_tree_linter.h" #include "common/analysis/text_structure_linter.h" #include "common/analysis/token_stream_linter.h" +#include "common/analysis/violation_handler.h" #include "common/strings/line_column_map.h" #include "common/text/text_structure.h" #include "verilog/analysis/lint_rule_registry.h" @@ -35,42 +36,11 @@ namespace verilog { -struct LintViolationWithStatus { - const verible::LintViolation* violation; - const verible::LintRuleStatus* status; - - LintViolationWithStatus(const verible::LintViolation* v, - const verible::LintRuleStatus* s) - : violation(v), status(s) {} - - bool operator<(const LintViolationWithStatus& r) const { - // compares addresses which correspond to locations within the same string - return violation->token.text().data() < r.violation->token.text().data(); - } -}; - // Returns violations from multiple `LintRuleStatus`es sorted by position // of their occurrence in source code. -std::set GetSortedViolations( +std::set GetSortedViolations( const std::vector& statuses); -// Interface for implementing violation handlers. -// -// The linting process produces a list of violations found in source code. Those -// violations are then sorted and passed to `HandleViolations()` method of an -// instance passed to LintOneFile(). -class ViolationHandler { - public: - virtual ~ViolationHandler() = default; - - // This method is called with a list of sorted violations found in file - // located at `path`. It can be called multiple times with statuses generated - // from different files. `base` contains source code from the file. - virtual void HandleViolations( - const std::set& violations, - absl::string_view base, absl::string_view path) = 0; -}; - // Checks a single file for Verilog style lint violations. // This is suitable for calling from main(). // 'stream' is used for printing potential syntax errors (if 'check_syntax' is @@ -86,7 +56,7 @@ class ViolationHandler { // errors were found (syntax, lint), and anything else is a fatal error. int LintOneFile(std::ostream* stream, absl::string_view filename, const LinterConfiguration& config, - ViolationHandler* violation_handler, bool check_syntax, + verible::ViolationHandler* violation_handler, bool check_syntax, bool parse_fatal, bool lint_fatal, bool show_context = false); // VerilogLinter analyzes a TextStructureView of Verilog source code. @@ -134,108 +104,6 @@ absl::StatusOr LinterConfigurationFromFlags( absl::Status AppendLinterConfigurationFromFile( LinterConfiguration* config, absl::string_view config_filename); -// ViolationHandler that prints all violations in a form of user-friendly -// messages. -class ViolationPrinter : public ViolationHandler { - public: - explicit ViolationPrinter(std::ostream* stream) - : stream_(stream), formatter_(nullptr) {} - - void HandleViolations(const std::set& violations, - absl::string_view base, absl::string_view path) final; - - protected: - std::ostream* const stream_; - verible::LintStatusFormatter* formatter_; -}; - -// ViolationHandler that prints all violations and gives an option to fix those -// that have autofixes available. -// -// By default, when violation has an autofix available, ViolationFixer asks an -// user what to do. The answers can be provided by AnswerChooser callback passed -// to the constructor as the answer_chooser parameter. The callback is called -// once for each fixable violation with a current violation object and a -// violated rule name as arguments, and must return one of the values from -// AnswerChoice enum. -// -// When the constructor's patch_stream parameter is not null, the fixes are -// written to specified stream in unified diff format. Otherwise the fixes are -// applied directly to the source file. -// -// The HandleLintRuleStatuses method can be called multiple times with statuses -// generated from different files. The state of answers like "apply all for -// rule" or "apply all" is kept between the calls. -class ViolationFixer : public ViolationHandler { - public: - enum class AnswerChoice { - kUnknown, - kApply, // apply fix - kReject, // reject fix - kApplyAllForRule, // apply this and all remaining fixes for violations - // of this rule - kRejectAllForRule, // reject this and all remaining fixes for violations - // of this rule - kApplyAll, // apply this and all remaining fixes - kRejectAll, // reject this and all remaining fixes - kPrintFix, // show fix - kPrintAppliedFixes, // show fixes applied so far - }; - - struct Answer { - AnswerChoice choice; - // If there are multiple alternatives for fixes available, this is - // the one chosen. By default the first one. - size_t alternative = 0; - }; - - using AnswerChooser = - std::function; - - // Violation fixer with user-chosen answer chooser. - ViolationFixer(std::ostream* message_stream, std::ostream* patch_stream, - const AnswerChooser& answer_chooser) - : ViolationFixer(message_stream, patch_stream, answer_chooser, false) {} - - // Violation fixer with interactive answer choice. - ViolationFixer(std::ostream* message_stream, std::ostream* patch_stream) - : ViolationFixer(message_stream, patch_stream, InteractiveAnswerChooser, - true) {} - - void HandleViolations(const std::set& violations, - absl::string_view base, absl::string_view path) final; - - private: - ViolationFixer(std::ostream* message_stream, std::ostream* patch_stream, - const AnswerChooser& answer_chooser, bool is_interactive) - : message_stream_(message_stream), - patch_stream_(patch_stream), - answer_chooser_(answer_chooser), - is_interactive_(is_interactive), - ultimate_answer_({AnswerChoice::kUnknown, 0}) {} - - void HandleViolation(const verible::LintViolation& violation, - absl::string_view base, absl::string_view path, - absl::string_view url, absl::string_view rule_name, - const verible::LintStatusFormatter& formatter, - verible::AutoFix* fix); - - static Answer InteractiveAnswerChooser( - const verible::LintViolation& violation, absl::string_view rule_name); - - void CommitFixes(absl::string_view source_content, - absl::string_view source_path, - const verible::AutoFix& fix) const; - - std::ostream* const message_stream_; - std::ostream* const patch_stream_; - const AnswerChooser answer_chooser_; - const bool is_interactive_; - - Answer ultimate_answer_; - std::map rule_answers_; -}; - // VerilogLintTextStructure analyzes Verilog syntax tree for style violations // and syntactically detectable pitfalls. // diff --git a/verilog/analysis/verilog_linter_test.cc b/verilog/analysis/verilog_linter_test.cc index c87ad8f37..8a9d3d2b8 100644 --- a/verilog/analysis/verilog_linter_test.cc +++ b/verilog/analysis/verilog_linter_test.cc @@ -34,6 +34,7 @@ #include "absl/status/statusor.h" #include "absl/strings/match.h" #include "absl/strings/string_view.h" +#include "common/analysis/violation_handler.h" #include "common/util/file_util.h" #include "common/util/logging.h" #include "gmock/gmock.h" @@ -47,6 +48,8 @@ namespace { using ::testing::EndsWith; using ::testing::StartsWith; +using verible::ViolationFixer; +using verible::ViolationPrinter; using verible::file::GetContents; using verible::file::testing::ScopedTestFile; @@ -70,7 +73,7 @@ class LintOneFileTest : public DefaultLinterConfigTestFixture, // Tests that nonexistent file is handled as a fatal error. TEST_F(LintOneFileTest, FileNotFound) { std::ostringstream output; - verilog::ViolationPrinter violation_printer(&output); + ViolationPrinter violation_printer(&output); const int exit_code = LintOneFile(&output, "FileNotFound.sv", config_, &violation_printer, true, false, false, false); @@ -89,7 +92,7 @@ TEST_F(LintOneFileTest, LintCleanFiles) { const ScopedTestFile temp_file(testing::TempDir(), test_code); { std::ostringstream output; - verilog::ViolationPrinter violation_printer(&output); + ViolationPrinter violation_printer(&output); const int exit_code = LintOneFile(&output, temp_file.filename(), config_, &violation_printer, true, false, false, false); @@ -98,7 +101,7 @@ TEST_F(LintOneFileTest, LintCleanFiles) { } { // enable additional error context printing std::ostringstream output; - verilog::ViolationPrinter violation_printer(&output); + ViolationPrinter violation_printer(&output); const int exit_code = LintOneFile(&output, temp_file.filename(), config_, &violation_printer, true, false, false, true); @@ -119,7 +122,7 @@ TEST_F(LintOneFileTest, SyntaxError) { const ScopedTestFile temp_file(testing::TempDir(), test_code); { // continue even with syntax error std::ostringstream output; - verilog::ViolationPrinter violation_printer(&output); + ViolationPrinter violation_printer(&output); const int exit_code = LintOneFile(&output, temp_file.filename(), config_, &violation_printer, true, false, false, false); @@ -128,7 +131,7 @@ TEST_F(LintOneFileTest, SyntaxError) { } { // continue even with syntax error with additional error context std::ostringstream output; - verilog::ViolationPrinter violation_printer(&output); + ViolationPrinter violation_printer(&output); const int exit_code = LintOneFile(&output, temp_file.filename(), config_, &violation_printer, true, false, false, true); @@ -137,7 +140,7 @@ TEST_F(LintOneFileTest, SyntaxError) { } { // abort on syntax error std::ostringstream output; - verilog::ViolationPrinter violation_printer(&output); + ViolationPrinter violation_printer(&output); const int exit_code = LintOneFile(&output, temp_file.filename(), config_, &violation_printer, true, true, false, false); @@ -146,7 +149,7 @@ TEST_F(LintOneFileTest, SyntaxError) { } { // ignore syntax error std::ostringstream output; - verilog::ViolationPrinter violation_printer(&output); + ViolationPrinter violation_printer(&output); const int exit_code = LintOneFile(&output, temp_file.filename(), config_, &violation_printer, false, false, false, false); @@ -166,7 +169,7 @@ TEST_F(LintOneFileTest, LintError) { const ScopedTestFile temp_file(testing::TempDir(), test_code); { // continue even with lint error std::ostringstream output; - verilog::ViolationPrinter violation_printer(&output); + ViolationPrinter violation_printer(&output); const int exit_code = LintOneFile(&output, temp_file.filename(), config_, &violation_printer, true, false, false, false); @@ -175,7 +178,7 @@ TEST_F(LintOneFileTest, LintError) { } { // abort on lint error std::ostringstream output; - verilog::ViolationPrinter violation_printer(&output); + ViolationPrinter violation_printer(&output); const int exit_code = LintOneFile(&output, temp_file.filename(), config_, &violation_printer, true, false, true, false); @@ -223,7 +226,7 @@ class VerilogLinterTest : public DefaultLinterConfigTestFixture, const absl::StatusOr> lint_result = VerilogLintTextStructure(filename, config_, text_structure); verilog::ViolationPrinter violation_printer(&diagnostics); - const std::set violations = + const std::set violations = GetSortedViolations(lint_result.value()); violation_printer.HandleViolations(violations, text_structure.Contents(), filename); @@ -460,7 +463,7 @@ class ViolationFixerTest : public testing::Test { const absl::StatusOr> lint_result = VerilogLintTextStructure(temp_file.filename(), config_, text_structure); - const std::set violations = + const std::set violations = GetSortedViolations(lint_result.value()); violation_fixer.HandleViolations(violations, text_structure.Contents(), temp_file.filename()); @@ -517,8 +520,7 @@ class ViolationFixerTest : public testing::Test { choice_it = choices.begin(); // intentionally unopened, diagnostics are discarded std::ofstream diagnostics; - verilog::ViolationFixer violation_fixer(&diagnostics, nullptr, - answer_chooser); + ViolationFixer violation_fixer(&diagnostics, nullptr, answer_chooser); std::vector fixed_sources(input_sources.size()); for (size_t i = 0; i < input_sources.size(); ++i) { @@ -548,8 +550,7 @@ class ViolationFixerTest : public testing::Test { // intentionally unopened, diagnostics are discarded std::ofstream diagnostics; std::ostringstream patch; - verilog::ViolationFixer violation_fixer(&diagnostics, &patch, - answer_chooser); + ViolationFixer violation_fixer(&diagnostics, &patch, answer_chooser); std::vector fixed_sources(input_sources.size()); for (size_t i = 0; i < input_sources.size(); ++i) { diff --git a/verilog/tools/lint/BUILD b/verilog/tools/lint/BUILD index ee2fbf79a..4ee763db7 100644 --- a/verilog/tools/lint/BUILD +++ b/verilog/tools/lint/BUILD @@ -230,6 +230,7 @@ cc_binary( "//common/util:file_util", "//common/util:init_command_line", "//common/util:logging", + "//common/analysis:violation_handler", "//verilog/analysis:verilog_linter", "//verilog/analysis:verilog_linter_configuration", "@com_google_absl//absl/flags:flag", diff --git a/verilog/tools/lint/verilog_lint.cc b/verilog/tools/lint/verilog_lint.cc index 0f0c30ea8..7ce379415 100644 --- a/verilog/tools/lint/verilog_lint.cc +++ b/verilog/tools/lint/verilog_lint.cc @@ -29,6 +29,7 @@ #include "absl/status/status.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" +#include "common/analysis/violation_handler.h" #include "common/util/enum_flags.h" #include "common/util/file_util.h" #include "common/util/init_command_line.h" @@ -157,33 +158,33 @@ int main(int argc, char** argv) { << autofix_mode << std::endl; } - const verilog::ViolationFixer::AnswerChooser applyAllFixes = + const verible::ViolationFixer::AnswerChooser applyAllFixes = [](const verible::LintViolation&, - absl::string_view) -> verilog::ViolationFixer::Answer { - return {verilog::ViolationFixer::AnswerChoice::kApplyAll, 0}; + absl::string_view) -> verible::ViolationFixer::Answer { + return {verible::ViolationFixer::AnswerChoice::kApplyAll, 0}; }; - std::unique_ptr violation_handler; + std::unique_ptr violation_handler; switch (autofix_mode) { case AutofixMode::kNo: - violation_handler.reset(new verilog::ViolationPrinter(&std::cerr)); + violation_handler.reset(new verible::ViolationPrinter(&std::cerr)); break; case AutofixMode::kPatchInteractive: CHECK(autofix_output_stream); violation_handler.reset( - new verilog::ViolationFixer(&std::cerr, autofix_output_stream)); + new verible::ViolationFixer(&std::cerr, autofix_output_stream)); break; case AutofixMode::kPatch: CHECK(autofix_output_stream); - violation_handler.reset(new verilog::ViolationFixer( + violation_handler.reset(new verible::ViolationFixer( &std::cerr, autofix_output_stream, applyAllFixes)); break; case AutofixMode::kInplaceInteractive: - violation_handler.reset(new verilog::ViolationFixer(&std::cerr, nullptr)); + violation_handler.reset(new verible::ViolationFixer(&std::cerr, nullptr)); break; case AutofixMode::kInplace: violation_handler.reset( - new verilog::ViolationFixer(&std::cerr, nullptr, applyAllFixes)); + new verible::ViolationFixer(&std::cerr, nullptr, applyAllFixes)); break; } diff --git a/verilog/tools/ls/verible-lsp-adapter.cc b/verilog/tools/ls/verible-lsp-adapter.cc index 4295654a8..7f20ecddd 100644 --- a/verilog/tools/ls/verible-lsp-adapter.cc +++ b/verilog/tools/ls/verible-lsp-adapter.cc @@ -28,7 +28,7 @@ namespace verilog { // Convert our representation of a linter violation to a LSP-Diagnostic static verible::lsp::Diagnostic ViolationToDiagnostic( - const verilog::LintViolationWithStatus &v, + const verible::LintViolationWithStatus &v, const verible::TextStructureView &text) { const verible::LintViolation &violation = *v.violation; const verible::LineColumnRange range = text.GetRangeForToken(violation.token);