Skip to content

Commit

Permalink
Update ForbidImplicitDeclarations lint rule
Browse files Browse the repository at this point in the history 
Signed-off-by: Lukasz Dalek <[email protected]>
  • Loading branch information
Lukasz Dalek committed Apr 24, 2020
1 parent 54f8762 commit 6754c23
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 211 deletions.
8 changes: 5 additions & 3 deletions verilog/analysis/checkers/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -315,17 +315,19 @@ cc_library(
deps = [
"//common/analysis:citation",
"//common/analysis:lint_rule_status",
"//common/analysis:syntax_tree_lint_rule",
"//common/analysis:text_structure_lint_rule",
"//common/analysis/matcher",
"//common/analysis/matcher:bound_symbol_manager",
"//common/analysis/matcher:core_matchers",
"//common/analysis/matcher:matcher_builders",
"//common/text:symbol",
"//common/text:syntax_tree_context",
"//common/util:auto_pop_stack",
#"//common/text:syntax_tree_context",
"//verilog/CST:identifier",
"//verilog/CST:verilog_matchers",
"//verilog/analysis:descriptions",
"//verilog/analysis:lint_rule_registry",
"//verilog/analysis:scope_tree_visitor",
"@com_google_absl//absl/strings",
],
alwayslink = 1,
Expand All @@ -337,7 +339,7 @@ cc_test(
deps = [
":forbid_implicit_declarations_rule",
"//common/analysis:linter_test_utils",
"//common/analysis:syntax_tree_linter_test_utils",
"//common/analysis:text_structure_linter_test_utils",
"//common/text:symbol",
"//verilog/CST:verilog_nonterminals",
"//verilog/CST:verilog_treebuilder_utils",
Expand Down
154 changes: 22 additions & 132 deletions verilog/analysis/checkers/forbid_implicit_declarations_rule.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,146 +52,36 @@ std::string ForbidImplicitDeclarationsRule::GetDescription(DescriptionType descr
"implicitly declared nets.");
}

const absl::string_view ForbidImplicitDeclarationsRule::GetIdentifier(
const verible::matcher::BoundSymbolManager& manager,
const std::string& id) const {
const auto* identifier_symbol = manager.FindSymbol(id);
const auto* identifier_leaf = AutoUnwrapIdentifier(*ABSL_DIE_IF_NULL(identifier_symbol));
return ABSL_DIE_IF_NULL(identifier_leaf)->get().text;
void ForbidImplicitDeclarationsRule::Lint(const verible::TextStructureView& text_structure,
absl::string_view filename) {
const verible::ConcreteSyntaxTree& syntax_tree = text_structure.SyntaxTree();
CHECK_NOTNULL(syntax_tree);
syntax_tree->Accept(this);
}

// checks that we are in correct scope
void ForbidImplicitDeclarationsRule::CheckAndPopScope(const verible::Symbol& symbol,
const SyntaxTreeContext& context) {
while ((scopes_.size() > 0) &&
(!ContainsAncestor(scopes_.back().symbol_, context))) {
scopes_.pop_back();
}
}

void ForbidImplicitDeclarationsRule::DetectScope(const verible::Symbol& symbol,
const SyntaxTreeContext& context) {
verible::matcher::BoundSymbolManager manager;

if (net_scope_matcher_.Matches(symbol, &manager)) {
CheckAndPopScope(symbol, context);
scopes_.push_back(ScopeType(&symbol));
}
}

void ForbidImplicitDeclarationsRule::DetectDeclarations(const verible::Symbol& symbol,
const SyntaxTreeContext& context) {
verible::matcher::BoundSymbolManager manager;
// If matched variable in scope
if (net_declaration_matcher_.Matches(symbol, &manager)) {
// Top of the matched tree stack
CHECK_GE(context.size(), 1);
const auto& top = context.top(); // scope
const auto top_tag = static_cast<verilog::NodeEnum>(top.Tag().tag);

CHECK((top_tag == NodeEnum::kModuleItemList) ||
(top_tag == NodeEnum::kGenerateItemList));

// This could be omitted if we had matchers for begining and ending of scopes
// e.g. something like kBegin/kEnd. But we would have to modify parser grammar for
// some of them, e.g. "module". Leaving like this for now.
CheckAndPopScope(symbol, context);

const auto* decls = ABSL_DIE_IF_NULL(manager.GetAs<verible::SyntaxTreeNode>("decls"));
for (const auto& itr : decls->children()) {
const auto& kind = itr.get()->Kind();
if (kind != verible::SymbolKind::kNode) continue;
const auto& tag = static_cast<verilog::NodeEnum>(itr.get()->Tag().tag);
if ((tag != verilog::NodeEnum::kNetVariable) &&
(tag != verilog::NodeEnum::kNetDeclarationAssignment)) continue;
const auto& leaf = ABSL_DIE_IF_NULL(GetLeftmostLeaf(*itr))->get();
CHECK_EQ(leaf.token_enum, SymbolIdentifier);
const auto& identifier = leaf.text;

// TODO: check parent scope for net names overlap?
CHECK_GE(scopes_.size(), 1);
auto& scope = scopes_.back().declared_nets_;
scope[identifier] = &top;
}
}
}

void ForbidImplicitDeclarationsRule::AnalyzeLHS(const verible::SyntaxTreeLeaf& lval,
const verible::SyntaxTreeContext& context) {
const verible::TokenInfo& lval_token = lval.get();
CHECK_EQ(lval_token.token_enum, SymbolIdentifier);
const auto& identifier = lval_token.text;

for (auto itr = scopes_.rbegin() ; itr != scopes_.rend() ; ++itr) {
const auto& scope = itr->declared_nets_;
//const auto* node = scope.at(identifier);
const auto& node = scope.find(identifier);

// If there's decleration or exists common ancestor then it's explicit declaration.
// TODO: Check necessity of call to ContainsAncestor().
// CheckAndPopScope() is propably enough.
if ((node != scope.end()) && (ContainsAncestor(node->second, context))) {
// Found declaration, stop searching
return ;
}
}

violations_.insert(LintViolation(lval, kMessage, context));
LintRuleStatus ForbidImplicitDeclarationsRule::Report() const {
return LintRuleStatus(violations_, Name(), GetStyleGuideCitation(kTopic));
}

void ForbidImplicitDeclarationsRule::DetectReference(const verible::Symbol& symbol,
const SyntaxTreeContext& context) {
verible::matcher::BoundSymbolManager manager;

if (net_assignment_matcher_.Matches(symbol, &manager)) {
if (!context.IsInside(NodeEnum::kContinuousAssignmentStatement)) {
void ForbidImplicitDeclarationsRule::HandleNetReference(
const verible::Symbol& symbol, const verible::SyntaxTreeContext& context,
const ScopeTreeContext& scope) {
const auto& leaf = *ABSL_DIE_IF_NULL(
dynamic_cast<const verible::SyntaxTreeLeaf*>(&symbol));
const auto identifier = leaf.get().text;

// Search upward stack and for declared nets.
for (auto itr = scope.rbegin() ; itr != scope.rend() ; ++itr) {
const auto& declared_nets = (*itr)->declared_nets_;
const auto& net = declared_nets.find(identifier);
if (net != declared_nets.end()) {
// Found net declaration
return ;
}

const auto* lpval = ABSL_DIE_IF_NULL(
manager.GetAs<verible::SyntaxTreeNode>("lpval"));

const auto& children = lpval->children();
CHECK_EQ(children.size(), 1);
const auto& lhs = *ABSL_DIE_IF_NULL(children[0]);

CheckAndPopScope(symbol, context);

if (net_reference_matcher_.Matches(lhs, &manager)) {
// Matches simple LPvalue, e.g. assign a = 1'b0;
const auto& lval = *ABSL_DIE_IF_NULL(
manager.GetAs<verible::SyntaxTreeLeaf>("lval"));
AnalyzeLHS(lval, context);
} else if (net_openrange_matcher_.Matches(lhs, &manager)) {
// Matches concatenated LPvalue, e.g. assign {a,b,c} = 3'b101;
const auto* clist = ABSL_DIE_IF_NULL(
manager.GetAs<verible::SyntaxTreeNode>("clist"));

for (const auto& itr : clist->children()) {
if (net_expression_reference_matcher_.Matches(*itr, &manager)) {
const auto& lval = *ABSL_DIE_IF_NULL(
manager.GetAs<verible::SyntaxTreeLeaf>("lval"));
AnalyzeLHS(lval, context);
}
}
} // TODO: Should log if we don't get any matches?
}
}

void ForbidImplicitDeclarationsRule::HandleSymbol(const verible::Symbol& symbol,
const SyntaxTreeContext& context) {
// Detects and pushes on stack new scopes
DetectScope(symbol, context);

// Detects nets declarations
DetectDeclarations(symbol, context);

// Detect referenced nets and checks for declarations
DetectReference(symbol, context);
}

LintRuleStatus ForbidImplicitDeclarationsRule::Report() const {
return LintRuleStatus(violations_, Name(), GetStyleGuideCitation(kTopic));
// No one net declaration was found. Report violation
violations_.insert(LintViolation(leaf, kMessage, context));
}

} // namespace analysis
Expand Down
90 changes: 15 additions & 75 deletions verilog/analysis/checkers/forbid_implicit_declarations_rule.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,109 +22,49 @@
#include "common/analysis/matcher/core_matchers.h"
#include "common/analysis/matcher/matcher.h"
#include "common/analysis/matcher/matcher_builders.h"
#include "common/analysis/syntax_tree_lint_rule.h"
#include "common/analysis/text_structure_lint_rule.h"
#include "common/text/symbol.h"
#include "common/text/syntax_tree_context.h"
#include "common/text/tree_context_visitor.h"
#include "common/util/auto_pop_stack.h"
#include "verilog/CST/verilog_matchers.h" // IWYU pragma: keep
#include "verilog/analysis/descriptions.h"
#include "verilog/analysis/scope_tree_visitor.h"

namespace verilog {
namespace analysis {

// ForbidImplicitDeclarationsRule detect implicitly declared nets
class ForbidImplicitDeclarationsRule : public verible::SyntaxTreeLintRule {
class ForbidImplicitDeclarationsRule : public verible::TextStructureLintRule,
public ScopeTreeVisitor {
public:
using rule_type = verible::SyntaxTreeLintRule;
using rule_type = verible::TextStructureLintRule;
static absl::string_view Name();

// Returns the description of the rule implemented formatted for either the
// helper flag or markdown depending on the parameter type.
static std::string GetDescription(DescriptionType);

void HandleSymbol(const verible::Symbol& symbol,
const verible::SyntaxTreeContext& context) override;
// Analyze text structure for violations.
void Lint(const verible::TextStructureView& text_structure,
absl::string_view filename) override;

verible::LintRuleStatus Report() const override;

void HandleNetReference(const verible::Symbol& symbol,
const verible::SyntaxTreeContext& context,
const ScopeTreeContext& scope) override;

private:
// Link to style guide rule.
static const char kTopic[];

// Diagnostic message.
static const char kMessage[];

using Matcher = verible::matcher::Matcher;

// Checks that top of the stack is our ancestor. If not
// pops until find one
void CheckAndPopScope(const verible::Symbol& symbol,
const verible::SyntaxTreeContext& context);

void AnalyzeLHS(const verible::SyntaxTreeLeaf& lval,
const verible::SyntaxTreeContext& context);

// Pushes new scopes on stack
void DetectScope(const verible::Symbol& symbol,
const verible::SyntaxTreeContext& context);

// Detects nets declarations adds them to map
void DetectDeclarations(const verible::Symbol& symbol,
const verible::SyntaxTreeContext& context);

void DetectReference(const verible::Symbol& symbol,
const verible::SyntaxTreeContext& context);

bool ContainsAncestor(const verible::Symbol* const symbol,
const verible::SyntaxTreeContext& context) const {
CHECK_NOTNULL(symbol);

// check for common ancestors
for (const auto& iter : context) {
if (iter == symbol) {
return true;
}
}
return false;
}

const absl::string_view GetIdentifier(
const verible::matcher::BoundSymbolManager& manager,
const std::string& id) const;

// Matches scopes with nets
// TODO: description, other scopes
const Matcher net_scope_matcher_ = verible::matcher::AnyOf(
NodekModuleItemList(),
NodekGenerateItemList());

// Matches nets declarations
const Matcher net_declaration_matcher_ =
NodekNetDeclaration(PathkNetVariableDeclarationAssign().Bind("decls"));

const Matcher net_assignment_matcher_ =
NodekNetVariableAssignment(PathkLPValue().Bind("lpval"));

const Matcher net_openrange_matcher_ =
NodekBraceGroup(PathkOpenRangeList().Bind("clist"));

// TODO: Any smart way to merge those two?
const Matcher net_reference_matcher_ = NodekReferenceCallBase(
PathkReference(UnqualifiedReferenceHasId().Bind("lval")));
const Matcher net_expression_reference_matcher_ = NodekExpression(
PathkReferenceCallBase(PathkReference(
UnqualifiedReferenceHasId().Bind("lval"))));

std::set<verible::LintViolation> violations_;

using DeclType = std::map<absl::string_view, const verible::Symbol*>;

struct ScopeType {
ScopeType(const verible::Symbol* symbol) : symbol_(symbol) {};
const verible::Symbol* const symbol_;
DeclType declared_nets_;
};

std::vector<ScopeType> scopes_;
ScopeTreeVisitor visitor_;
};

} // namespace analysis
Expand Down
2 changes: 1 addition & 1 deletion verilog/analysis/checkers/forbid_implicit_declarations_rule_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

#include "gtest/gtest.h"
#include "common/analysis/linter_test_utils.h"
#include "common/analysis/syntax_tree_linter_test_utils.h"
#include "common/analysis/text_structure_linter_test_utils.h"
#include "common/text/symbol.h"
#include "verilog/CST/verilog_nonterminals.h"
#include "verilog/CST/verilog_treebuilder_utils.h"
Expand Down

0 comments on commit 6754c23

Please sign in to comment.