Skip to content

Commit b41588e

Browse files
hzellerhzeller
committed
Don't use down_cast<> with the assumption that type-mismatch returns null.
We typically don't compile-in RTTI so relying on down_cast<> returning nullptr or a correct type will result in subtle issues. Instead, provide MaybeNode() and MaybeLeaf() that use the SymbolKind to provide such casts. This should be expanded: * Ideally we remove _all_ uses of down_cast<> and use type-safe ways of doing the same thing (and then: remove verible::down_cast<>). * There are a few places where we CHECK() fail on type-mismatch, assuming these can't happen; but that depends on how valid our syntax tree is, which depends on the input the the parser to reject such input. These should be reformulated with graceful error handling. Fixes #2419
1 parent 93adb37 commit b41588e

23 files changed

+100
-100
lines changed

‎verible/common/analysis/matcher/BUILD

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ cc_library(
1717
hdrs = ["bound-symbol-manager.h"],
1818
deps = [
1919
"//verible/common/text:symbol",
20-
"//verible/common/util:casts",
20+
"//verible/common/text:tree-utils",
2121
"//verible/common/util:container-util",
2222
"//verible/common/util:logging",
2323
],

‎verible/common/analysis/matcher/bound-symbol-manager.h

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,11 @@
1919
#include <string>
2020

2121
#include "verible/common/text/symbol.h"
22-
#include "verible/common/util/casts.h"
22+
#include "verible/common/text/tree-utils.h"
2323

2424
namespace verible {
25+
class SyntaxTreeNode;
26+
class SyntaxTreeLeaf;
2527
namespace matcher {
2628

2729
// Manages sets of Bound Symbols created when matching against a syntax tree.
@@ -52,9 +54,14 @@ class BoundSymbolManager {
5254
return bound_symbols_;
5355
}
5456

55-
template <typename T>
56-
const T *GetAs(const std::string &key) const {
57-
return down_cast<const T *>(FindSymbol(key));
57+
// Return named node as Node if available of of that type.
58+
const SyntaxTreeNode *GetAsNode(const std::string &key) const {
59+
return verible::MaybeNode(FindSymbol(key));
60+
}
61+
62+
// Return named node as Node if available of of that type.
63+
const SyntaxTreeLeaf *GetAsLeaf(const std::string &key) const {
64+
return verible::MaybeLeaf(FindSymbol(key));
5865
}
5966

6067
private:

‎verible/common/text/symbol.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,6 @@ constexpr SymbolTag NodeTag(EnumType tag) {
5555
}
5656
constexpr SymbolTag LeafTag(int tag) { return {SymbolKind::kLeaf, tag}; }
5757

58-
// forward declare Visitor classes to allow references in Symbol
59-
6058
class Symbol {
6159
public:
6260
virtual ~Symbol() = default;

‎verible/common/text/tree-utils.cc

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,15 @@ const SyntaxTreeLeaf &SymbolCastToLeaf(const Symbol &symbol) {
126126
return down_cast<const SyntaxTreeLeaf &>(symbol);
127127
}
128128

129+
const SyntaxTreeNode *MaybeNode(const Symbol *symbol) {
130+
if (!symbol || symbol->Kind() != SymbolKind::kNode) return nullptr;
131+
return static_cast<const SyntaxTreeNode *>(symbol);
132+
}
133+
const SyntaxTreeLeaf *MaybeLeaf(const Symbol *symbol) {
134+
if (!symbol || symbol->Kind() != SymbolKind::kLeaf) return nullptr;
135+
return static_cast<const SyntaxTreeLeaf *>(symbol);
136+
}
137+
129138
namespace {
130139
// FirstSubtreeFinderMutable is a visitor class that supports the implementation
131140
// of FindFirstSubtreeMutable(). It is derived from

‎verible/common/text/tree-utils.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,9 @@
3030
#include "verible/common/util/logging.h"
3131
#include "verible/common/util/type-traits.h"
3232

33+
// TODO: the functions below that can crash while asserting a type should
34+
// probably return a usable error (std::optional).
35+
3336
namespace verible {
3437

3538
// Returns the leftmost/rightmost leaf contained in Symbol.
@@ -47,6 +50,7 @@ std::string_view StringSpanOfSymbol(const Symbol &symbol);
4750
std::string_view StringSpanOfSymbol(const Symbol &lsym, const Symbol &rsym);
4851

4952
// Returns a SyntaxTreeNode down_casted from a Symbol.
53+
// Will panic if not of that kind.
5054
const SyntaxTreeNode &SymbolCastToNode(const Symbol &);
5155
// Mutable variant.
5256
SyntaxTreeNode &SymbolCastToNode(Symbol &); // NOLINT
@@ -59,8 +63,15 @@ inline const SyntaxTreeNode &SymbolCastToNode(const SyntaxTreeNode &node) {
5963
inline SyntaxTreeNode &SymbolCastToNode(SyntaxTreeNode &node) { return node; }
6064

6165
// Returns a SyntaxTreeLeaf down_casted from a Symbol.
66+
// Will panic if not of that kind.
6267
const SyntaxTreeLeaf &SymbolCastToLeaf(const Symbol &);
6368

69+
// Return Node if symbol is of that kind, otherwise nullptr.
70+
const SyntaxTreeNode *MaybeNode(const Symbol *symbol);
71+
72+
// Return Leaf if symbol is of that kind, otherwise nullptr.
73+
const SyntaxTreeLeaf *MaybeLeaf(const Symbol *symbol);
74+
6475
// Unwrap layers of only-child nodes until reaching a leaf or a node with
6576
// multiple children.
6677
const Symbol *DescendThroughSingletons(const Symbol &symbol);

‎verible/verilog/CST/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ cc_library(
9898
"//verible/common/text:concrete-syntax-tree",
9999
"//verible/common/text:symbol",
100100
"//verible/common/text:token-info",
101+
"//verible/common/text:tree-utils",
101102
"//verible/common/util:logging",
102103
"@abseil-cpp//absl/strings",
103104
],

‎verible/verilog/CST/expression.cc

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,8 +104,9 @@ const verible::TokenInfo *GetUnaryPrefixOperator(
104104
return nullptr;
105105
}
106106
const verible::Symbol *leaf_symbol = node->front().get();
107-
return &verible::down_cast<const verible::SyntaxTreeLeaf *>(leaf_symbol)
108-
->get();
107+
const verible::SyntaxTreeLeaf *leaf = verible::MaybeLeaf(leaf_symbol);
108+
if (!leaf) return nullptr;
109+
return &leaf->get();
109110
}
110111

111112
const verible::Symbol *GetUnaryPrefixOperand(const verible::Symbol &symbol) {

‎verible/verilog/CST/identifier.cc

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,7 @@ const verible::SyntaxTreeLeaf *GetIdentifier(const verible::Symbol &symbol) {
7272
return nullptr;
7373
}
7474
const auto &node = down_cast<const verible::SyntaxTreeNode &>(symbol);
75-
const auto *leaf = down_cast<const verible::SyntaxTreeLeaf *>(node[0].get());
76-
return leaf;
75+
return verible::MaybeLeaf(node[0].get());
7776
}
7877

7978
const verible::SyntaxTreeLeaf *AutoUnwrapIdentifier(

‎verible/verilog/CST/verilog-treebuilder-utils.cc

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,11 @@
2222
#include "verible/common/text/concrete-syntax-tree.h"
2323
#include "verible/common/text/symbol.h"
2424
#include "verible/common/text/token-info.h"
25+
#include "verible/common/text/tree-utils.h"
2526
#include "verible/common/util/logging.h"
2627

2728
namespace verilog {
2829

29-
using verible::down_cast;
30-
3130
// Set of utility functions for embedded a statement into a certain context.
3231
std::string EmbedInClass(std::string_view text) {
3332
return absl::StrCat("class test_class;\n", text, "\nendclass\n");
@@ -47,7 +46,7 @@ std::string EmbedInClassMethod(std::string_view text) {
4746
}
4847

4948
void ExpectString(const verible::SymbolPtr &symbol, std::string_view expected) {
50-
const auto *leaf = down_cast<const verible::SyntaxTreeLeaf *>(symbol.get());
49+
const verible::SyntaxTreeLeaf *leaf = verible::MaybeLeaf(symbol.get());
5150
CHECK(leaf != nullptr) << "expected: " << expected;
5251
CHECK_EQ(leaf->get().text(), expected);
5352
}

‎verible/verilog/analysis/checkers/BUILD

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -931,6 +931,7 @@ cc_library(
931931
"//verible/common/text:symbol",
932932
"//verible/common/text:syntax-tree-context",
933933
"//verible/common/text:token-info",
934+
"//verible/common/text:tree-utils",
934935
"//verible/common/util:logging",
935936
"//verible/verilog/CST:numbers",
936937
"//verible/verilog/CST:verilog-matchers",
@@ -971,6 +972,7 @@ cc_library(
971972
"//verible/common/text:symbol",
972973
"//verible/common/text:syntax-tree-context",
973974
"//verible/common/text:token-info",
975+
"//verible/common/text:tree-utils",
974976
"//verible/verilog/CST:numbers",
975977
"//verible/verilog/CST:verilog-matchers",
976978
"//verible/verilog/analysis:descriptions",
@@ -1010,7 +1012,7 @@ cc_library(
10101012
"//verible/common/text:symbol",
10111013
"//verible/common/text:syntax-tree-context",
10121014
"//verible/common/text:token-info",
1013-
"//verible/common/util:casts",
1015+
"//verible/common/text:tree-utils",
10141016
"//verible/verilog/CST:expression",
10151017
"//verible/verilog/CST:verilog-matchers",
10161018
"//verible/verilog/CST:verilog-nonterminals",
@@ -1156,7 +1158,6 @@ cc_library(
11561158
"//verible/common/text:symbol",
11571159
"//verible/common/text:syntax-tree-context",
11581160
"//verible/common/text:tree-utils",
1159-
"//verible/common/util:casts",
11601161
"//verible/verilog/CST:verilog-matchers",
11611162
"//verible/verilog/CST:verilog-nonterminals",
11621163
"//verible/verilog/analysis:descriptions",
@@ -1195,7 +1196,7 @@ cc_library(
11951196
"//verible/common/text:config-utils",
11961197
"//verible/common/text:symbol",
11971198
"//verible/common/text:syntax-tree-context",
1198-
"//verible/common/util:casts",
1199+
"//verible/common/text:tree-utils",
11991200
"//verible/common/util:logging",
12001201
"//verible/verilog/CST:verilog-matchers",
12011202
"//verible/verilog/CST:verilog-nonterminals",

‎verible/verilog/analysis/checkers/always-comb-blocking-rule.cc

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
#include "verible/common/text/symbol.h"
2828
#include "verible/common/text/syntax-tree-context.h"
2929
#include "verible/common/text/tree-utils.h"
30-
#include "verible/common/util/casts.h"
3130
#include "verible/verilog/CST/verilog-matchers.h" // IWYU pragma: keep
3231
#include "verible/verilog/CST/verilog-nonterminals.h"
3332
#include "verible/verilog/analysis/descriptions.h"
@@ -38,7 +37,6 @@ namespace verilog {
3837
namespace analysis {
3938

4039
using verible::AutoFix;
41-
using verible::down_cast;
4240
using verible::LintRuleStatus;
4341
using verible::LintViolation;
4442
using verible::SearchSyntaxTree;
@@ -77,8 +75,8 @@ void AlwaysCombBlockingRule::HandleSymbol(const verible::Symbol &symbol,
7775
SearchSyntaxTree(symbol, NodekNonblockingAssignmentStatement())) {
7876
if (match.match->Kind() != verible::SymbolKind::kNode) continue;
7977

80-
const auto *node =
81-
down_cast<const verible::SyntaxTreeNode *>(match.match);
78+
const verible::SyntaxTreeNode *node = verible::MaybeNode(match.match);
79+
if (!node) return;
8280

8381
const verible::SyntaxTreeLeaf *leaf = verible::GetSubtreeAsLeaf(
8482
*node, NodeEnum::kNonblockingAssignmentStatement, 1);

‎verible/verilog/analysis/checkers/always-ff-non-blocking-rule.cc

Lines changed: 14 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
#include "verible/common/text/config-utils.h"
3030
#include "verible/common/text/symbol.h"
3131
#include "verible/common/text/syntax-tree-context.h"
32-
#include "verible/common/util/casts.h"
32+
#include "verible/common/text/tree-utils.h"
3333
#include "verible/common/util/logging.h"
3434
#include "verible/verilog/CST/verilog-matchers.h"
3535
#include "verible/verilog/CST/verilog-nonterminals.h"
@@ -108,22 +108,18 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol,
108108

109109
verible::matcher::BoundSymbolManager symbol_man;
110110
if (asgn_blocking_matcher.Matches(symbol, &symbol_man)) {
111-
if (const auto *const node =
112-
verible::down_cast<const verible::SyntaxTreeNode *>(&symbol)) {
113-
check_root =
114-
/* lhs */ verible::down_cast<const verible::SyntaxTreeNode *>(
115-
node->front().get());
111+
if (const verible::SyntaxTreeNode *const node =
112+
verible::MaybeNode(&symbol)) {
113+
check_root = /* lhs */ verible::MaybeNode(node->front().get());
116114
}
117115
} else {
118116
// Not interested in any other blocking assignments unless flagged
119117
if (!catch_modifying_assignments_) return;
120118

121119
if (asgn_modify_matcher.Matches(symbol, &symbol_man)) {
122-
if (const auto *const node =
123-
verible::down_cast<const verible::SyntaxTreeNode *>(&symbol)) {
124-
check_root =
125-
/* lhs */ verible::down_cast<const verible::SyntaxTreeNode *>(
126-
node->front().get());
120+
if (const verible::SyntaxTreeNode *const node =
121+
verible::MaybeNode(&symbol)) {
122+
check_root = /* lhs */ verible::MaybeNode(node->front().get());
127123
}
128124
} else if (asgn_incdec_matcher.Matches(symbol, &symbol_man)) {
129125
check_root = &symbol;
@@ -144,11 +140,9 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol,
144140
if (var.context.IsInside(NodeEnum::kHierarchyExtension)) continue;
145141

146142
bool found = false;
147-
if (const auto *const varn =
148-
verible::down_cast<const verible::SyntaxTreeNode *>(var.match)) {
149-
if (const auto *const ident =
150-
verible::down_cast<const verible::SyntaxTreeLeaf *>(
151-
varn->front().get())) {
143+
if (const verible::SyntaxTreeNode *const varn =
144+
verible::MaybeNode(var.match)) {
145+
if (const auto *const ident = verible::MaybeLeaf(varn->front().get())) {
152146
found = std::find(locals_.begin(), locals_.end(),
153147
ident->get().text()) != locals_.end();
154148
VLOG(4) << "LHS='" << ident->get().text() << "' FOUND=" << found
@@ -210,11 +204,10 @@ bool AlwaysFFNonBlockingRule::LocalDeclaration(const verible::Symbol &symbol) {
210204
if (decl_matcher.Matches(symbol, &symbol_man)) {
211205
auto &count = scopes_.top().inherited_local_count;
212206
for (const auto &var : SearchSyntaxTree(symbol, var_matcher)) {
213-
if (const auto *const node =
214-
verible::down_cast<const verible::SyntaxTreeNode *>(var.match)) {
215-
if (const auto *const ident =
216-
verible::down_cast<const verible::SyntaxTreeLeaf *>(
217-
node->front().get())) {
207+
if (const verible::SyntaxTreeNode *const node =
208+
verible::MaybeNode(var.match)) {
209+
if (const verible::SyntaxTreeLeaf *const ident =
210+
verible::MaybeLeaf(node->front().get())) {
218211
const std::string_view name = ident->get().text();
219212
VLOG(4) << "Registering '" << name << '\'' << std::endl;
220213
locals_.emplace_back(name);

‎verible/verilog/analysis/checkers/create-object-name-match-rule.cc

Lines changed: 13 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
#include "verible/common/text/symbol.h"
3030
#include "verible/common/text/syntax-tree-context.h"
3131
#include "verible/common/text/token-info.h"
32-
#include "verible/common/util/casts.h"
32+
#include "verible/common/text/tree-utils.h"
3333
#include "verible/verilog/CST/expression.h"
3434
#include "verible/verilog/CST/verilog-matchers.h"
3535
#include "verible/verilog/CST/verilog-nonterminals.h"
@@ -40,7 +40,6 @@
4040
namespace verilog {
4141
namespace analysis {
4242

43-
using verible::down_cast;
4443
using verible::LintRuleStatus;
4544
using verible::LintViolation;
4645
using verible::SyntaxTreeContext;
@@ -89,8 +88,7 @@ static bool UnqualifiedIdEquals(const SyntaxTreeNode &node,
8988
if (node.MatchesTag(NodeEnum::kUnqualifiedId)) {
9089
if (!node.empty()) {
9190
// The one-and-only child is the SymbolIdentifier token
92-
const auto &leaf_ptr =
93-
down_cast<const SyntaxTreeLeaf *>(node.front().get());
91+
const SyntaxTreeLeaf *leaf_ptr = verible::MaybeLeaf(node.front().get());
9492
if (leaf_ptr != nullptr) {
9593
const TokenInfo &token = leaf_ptr->get();
9694
return token.token_enum() == SymbolIdentifier && token.text() == name;
@@ -109,10 +107,11 @@ static bool QualifiedCallIsTypeIdCreate(
109107
// my_pkg::class_type::type_id::create.
110108
// 5: 3 segments + 2 separators (in alternation), e.g. A::B::C
111109
if (qualified_id_node.size() >= 5) {
112-
const auto *create_leaf_ptr =
113-
down_cast<const SyntaxTreeNode *>(qualified_id_node.back().get());
114-
const auto *type_id_leaf_ptr = down_cast<const SyntaxTreeNode *>(
115-
qualified_id_node[num_children - 3].get());
110+
// TODO: naming is off. These are not leafs
111+
const SyntaxTreeNode *create_leaf_ptr =
112+
verible::MaybeNode(qualified_id_node.back().get());
113+
const SyntaxTreeNode *type_id_leaf_ptr =
114+
verible::MaybeNode(qualified_id_node[num_children - 3].get());
116115
if (create_leaf_ptr != nullptr && type_id_leaf_ptr != nullptr) {
117116
return UnqualifiedIdEquals(*create_leaf_ptr, "create") &&
118117
UnqualifiedIdEquals(*type_id_leaf_ptr, "type_id");
@@ -138,12 +137,7 @@ static const TokenInfo *ExtractStringLiteralToken(
138137
if (!expr_node.MatchesTag(NodeEnum::kExpression)) return nullptr;
139138

140139
// this check is limited to only checking string literal leaf tokens
141-
if (expr_node.front().get()->Kind() != verible::SymbolKind::kLeaf) {
142-
return nullptr;
143-
}
144-
145-
const auto *leaf_ptr =
146-
down_cast<const SyntaxTreeLeaf *>(expr_node.front().get());
140+
const SyntaxTreeLeaf *leaf_ptr = verible::MaybeLeaf(expr_node.front().get());
147141
if (leaf_ptr != nullptr) {
148142
const TokenInfo &token = leaf_ptr->get();
149143
if (token.token_enum() == TK_StringLiteral) {
@@ -158,8 +152,8 @@ static const SyntaxTreeNode *GetFirstExpressionFromArgs(
158152
const SyntaxTreeNode &args_node) {
159153
if (!args_node.empty()) {
160154
const auto &first_arg = args_node.front();
161-
if (const auto *first_expr =
162-
down_cast<const SyntaxTreeNode *>(first_arg.get())) {
155+
if (const SyntaxTreeNode *first_expr =
156+
verible::MaybeNode(first_arg.get())) {
163157
return first_expr;
164158
}
165159
}
@@ -183,15 +177,15 @@ void CreateObjectNameMatchRule::HandleSymbol(const verible::Symbol &symbol,
183177

184178
// Extract named bindings for matched nodes within this match.
185179

186-
const auto *lval_ref = manager.GetAs<SyntaxTreeNode>("lval_ref");
180+
const SyntaxTreeNode *lval_ref = manager.GetAsNode("lval_ref");
187181
if (lval_ref == nullptr) return;
188182

189183
const TokenInfo *lval_id = ReferenceIsSimpleIdentifier(*lval_ref);
190184
if (lval_id == nullptr) return;
191185
if (lval_id->token_enum() != SymbolIdentifier) return;
192186

193-
const auto *call = manager.GetAs<SyntaxTreeNode>("func");
194-
const auto *args = manager.GetAs<SyntaxTreeNode>("args");
187+
const SyntaxTreeNode *call = manager.GetAsNode("func");
188+
const SyntaxTreeNode *args = manager.GetAsNode("args");
195189
if (call == nullptr) return;
196190
if (args == nullptr) return;
197191
if (!QualifiedCallIsTypeIdCreate(*call)) return;

‎verible/verilog/analysis/checkers/forbidden-macro-rule.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ void ForbiddenMacroRule::HandleSymbol(
7171
const verible::Symbol &symbol, const verible::SyntaxTreeContext &context) {
7272
verible::matcher::BoundSymbolManager manager;
7373
if (MacroCallMatcher().Matches(symbol, &manager)) {
74-
if (const auto *leaf = manager.GetAs<verible::SyntaxTreeLeaf>("name")) {
74+
if (const verible::SyntaxTreeLeaf *leaf = manager.GetAsLeaf("name")) {
7575
const auto &imm = InvalidMacrosMap();
7676
if (imm.find(std::string(leaf->get().text())) != imm.end()) {
7777
violations_.insert(

‎verible/verilog/analysis/checkers/forbidden-symbol-rule.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ void ForbiddenSystemTaskFunctionRule::HandleSymbol(
7979
const verible::Symbol &symbol, const verible::SyntaxTreeContext &context) {
8080
verible::matcher::BoundSymbolManager manager;
8181
if (IdMatcher().Matches(symbol, &manager)) {
82-
if (const auto *leaf = manager.GetAs<verible::SyntaxTreeLeaf>("name")) {
82+
if (const verible::SyntaxTreeLeaf *leaf = manager.GetAsLeaf("name")) {
8383
const auto &ism = InvalidSymbolsMap();
8484
if (ism.find(std::string(leaf->get().text())) != ism.end()) {
8585
violations_.insert(

0 commit comments

Comments
 (0)