Skip to content

Commit 6834307

Browse files
hzellerhzeller
authored
Merge pull request #2423 from hzeller/feature-20250531-limit-down-cast
Don't use down_cast<> with the assumption that type-mismatch returns …
2 parents 93adb37 + b41588e commit 6834307

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)