Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename father/son to predecessor/successor #2423

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Rename father/son to predecessor/successor #2423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b90371d
Rename father/son to predecessor/successor
DarkaMaul Apr 12, 2024
a11b730
Merge branch 'dev' into dev-cfg-rename
DarkaMaul Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions examples/scripts/taint_mapping.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
return

visited += [node]
taints = node.function.compilation_unit.context[KEY]

Check failure on line 20 in examples/scripts/taint_mapping.py

View workflow job for this annotation

GitHub Actions / Lint Code Base 

E0601: Using variable 'KEY' before assignment (used-before-assignment)

refs = {}
for ir in node.irs:
Expand Down Expand Up @@ -46,8 +46,8 @@

node.function.compilation_unit.context[KEY] = list(set(taints))

for son in node.sons:
visit_node(son, visited)
for successor in node.successors:
visit_node(successor, visited)


def check_call(func, taints):
Expand All @@ -55,7 +55,7 @@
for ir in node.irs:
if isinstance(ir, HighLevelCall):
if ir.destination in taints:
print(f"Call to tainted address found in {function.name}")

Check failure on line 58 in examples/scripts/taint_mapping.py

View workflow job for this annotation

GitHub Actions / Lint Code Base 

E0601: Using variable 'function' before assignment (used-before-assignment)


if __name__ == "__main__":
Expand Down
8 changes: 4 additions & 4 deletions slither/analyses/write/are_variables_written.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,18 +86,18 @@ def _visit(
lvalue = refs_lvalues

ret: List[Variable] = []
if not node.sons and node.type not in [NodeType.THROW, NodeType.RETURN]:
if not node.successors and node.type not in [NodeType.THROW, NodeType.RETURN]:
ret += [v for v in variables_to_write if v not in variables_written]

# Explore sons if
# Explore successors if
# - Before is none: its the first time we explored the node
# - variables_written is not before: it means that this path has a configuration of set variables
# that we haven't seen yet
before = state.nodes[node] if node in state.nodes else None
if before is None or variables_written not in before:
state.nodes[node].append(variables_written)
for son in node.sons:
ret += _visit(son, state, variables_written, variables_to_write)
for successor in node.successors:
ret += _visit(successor, state, variables_written, variables_to_write)
Comment on lines +89 to +100
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updates to handle successors are correctly implemented. Consider using state.nodes.get(node, None) for more concise and Pythonic access to dictionary elements.

- before = state.nodes[node] if node in state.nodes else None
+ before = state.nodes.get(node, None)
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if not node.successors and node.type not in [NodeType.THROW, NodeType.RETURN]:
ret += [v for v in variables_to_write if v not in variables_written]
# Explore sons if
# Explore successors if
# - Before is none: its the first time we explored the node
# - variables_written is not before: it means that this path has a configuration of set variables
# that we haven't seen yet
before = state.nodes[node] if node in state.nodes else None
if before is None or variables_written not in before:
state.nodes[node].append(variables_written)
for son in node.sons:
ret += _visit(son, state, variables_written, variables_to_write)
for successor in node.successors:
ret += _visit(successor, state, variables_written, variables_to_write)
if not node.successors and node.type not in [NodeType.THROW, NodeType.RETURN]:
ret += [v for v in variables_to_write if v not in variables_written]
# Explore successors if
# - Before is none: its the first time we explored the node
# - variables_written is not before: it means that this path has a configuration of set variables
# that we haven't seen yet
before = state.nodes.get(node, None)
if before is None or variables_written not in before:
state.nodes[node].append(variables_written)
for successor in node.successors:
ret += _visit(successor, state, variables_written, variables_to_write)
Tools
Ruff

96-96: Use state.nodes.get(node, None) instead of an if block (SIM401)

Replace with state.nodes.get(node, None)

return ret


Expand Down
156 changes: 98 additions & 58 deletions slither/core/cfg/node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
Node module
"""
from enum import Enum
import logging
from typing import Optional, List, Set, Dict, Tuple, Union, TYPE_CHECKING

from slither.all_exceptions import SlitherException
Expand Down Expand Up @@ -64,6 +65,8 @@
###################################################################################
###################################################################################

logger = logging.getLogger("SlitherNode")


class NodeType(Enum):
ENTRYPOINT = "ENTRY_POINT" # no expression
Expand Down Expand Up @@ -125,8 +128,8 @@ def __init__(
self._node_type = node_type

# TODO: rename to explicit CFG
self._sons: List["Node"] = []
self._fathers: List["Node"] = []
self._successors: List["Node"] = []
self._predecessors: List["Node"] = []
Comment on lines +131 to +132
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor type annotations by removing quotes.

- self._successors: List["Node"] = []
- self._predecessors: List["Node"] = []
+ self._successors: List[Node] = []
+ self._predecessors: List[Node] = []

- self._dominators: Set["Node"] = set()
- self._immediate_dominator: Optional["Node"] = None
+ self._dominators: Set[Node] = set()
+ self._immediate_dominator: Optional[Node] = None

- self._dom_successors: Set["Node"] = set()
- self._dominance_frontier: Set["Node"] = set()
+ self._dom_successors: Set[Node] = set()
+ self._dominance_frontier: Set[Node] = set()

- self._phi_origins_state_variables: Dict[str, Tuple[StateVariable, Set["Node"]]] = {}
- self._phi_origins_local_variables: Dict[str, Tuple[LocalVariable, Set["Node"]]] = {}
+ self._phi_origins_state_variables: Dict[str, Tuple[StateVariable, Set[Node]]] = {}
+ self._phi_origins_local_variables: Dict[str, Tuple[LocalVariable, Set[Node]]] = {}

- self._ssa_vars_written: List["SlithIRVariable"] = []
- self._ssa_vars_read: List["SlithIRVariable"] = []
+ self._ssa_vars_written: List[SlithIRVariable] = []
+ self._ssa_vars_read: List[SlithIRVariable] = []

- self._ssa_state_vars_written: List[StateIRVariable] = []
- self._ssa_state_vars_read: List[StateIRVariable] = []
+ self._ssa_state_vars_written: List[StateIRVariable] = []
+ self._ssa_state_vars_read: List[StateIRVariable] = []

Also applies to: 136-137, 140-142, 145-146, 156-157, 159-163, 181-181, 197-199, 198-198

Committable suggestion was skipped due to low confidence.

Tools
Ruff

131-131: Remove quotes from type annotation (UP037)

Remove quotes

132-132: Remove quotes from type annotation (UP037)

Remove quotes


## Dominators info
# Dominators nodes
Expand Down Expand Up @@ -225,7 +228,7 @@ def type(self, new_type: NodeType) -> None:

@property
def will_return(self) -> bool:
if not self.sons and self.type != NodeType.THROW:
if not self.successors and self.type != NodeType.THROW:
if SolidityFunction("revert()") not in self.solidity_calls:
if SolidityFunction("revert(string)") not in self.solidity_calls:
Comment on lines +231 to 233
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimize conditional checks by combining them into a single if statement.

- if not self.successors and self.type != NodeType.THROW:
-     if SolidityFunction("revert()") not in self.solidity_calls:
-         if SolidityFunction("revert(string)") not in self.solidity_calls:
+ if not self.successors and self.type != NodeType.THROW and SolidityFunction("revert()") not in self.solidity_calls and SolidityFunction("revert(string)") not in self.solidity_calls:
              return True
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if not self.successors and self.type != NodeType.THROW:
if SolidityFunction("revert()") not in self.solidity_calls:
if SolidityFunction("revert(string)") not in self.solidity_calls:
if not self.successors and self.type != NodeType.THROW and SolidityFunction("revert()") not in self.solidity_calls and SolidityFunction("revert(string)") not in self.solidity_calls:
return True
Tools
Ruff

231-233: Use a single if statement instead of nested if statements (SIM102)

232-233: Use a single if statement instead of nested if statements (SIM102)

return True
Expand Down Expand Up @@ -582,97 +585,97 @@ def add_inline_asm(self, asm: Union[str, Dict]) -> None:
###################################################################################
###################################################################################

def add_father(self, father: "Node") -> None:
"""Add a father node
def add_predecessor(self, predecessor: "Node") -> None:
"""Add a predecessor node

Args:
father: father to add
predecessor: predecessor to add
"""
self._fathers.append(father)
self._predecessors.append(predecessor)

def set_fathers(self, fathers: List["Node"]) -> None:
"""Set the father nodes
def set_predecessors(self, predecessors: List["Node"]) -> None:
"""Set the predecessors nodes

Args:
fathers: list of fathers to add
predecessors: list of predecessors to add
"""
self._fathers = fathers
self._predecessors = predecessors

@property
def fathers(self) -> List["Node"]:
"""Returns the father nodes
def predecessors(self) -> List["Node"]:
"""Returns the predecessor nodes

Returns:
list(Node): list of fathers
list(Node): list of predecessor
"""
return list(self._fathers)
return list(self._predecessors)

def remove_father(self, father: "Node") -> None:
"""Remove the father node. Do nothing if the node is not a father
def remove_predecessor(self, predecessor: "Node") -> None:
"""Remove the predecessor node. Do nothing if the node is not a predecessor

Args:
:param father:
:param predecessor:
"""
self._fathers = [x for x in self._fathers if x.node_id != father.node_id]
self._predecessors = [x for x in self._predecessors if x.node_id != predecessor.node_id]

def remove_son(self, son: "Node") -> None:
"""Remove the son node. Do nothing if the node is not a son
def remove_successor(self, successor: "Node") -> None:
"""Remove the successor node. Do nothing if the node is not a successor

Args:
:param son:
:param successor:
"""
self._sons = [x for x in self._sons if x.node_id != son.node_id]
self._successors = [x for x in self._successors if x.node_id != successor.node_id]

def add_son(self, son: "Node") -> None:
"""Add a son node
def add_successor(self, successor: "Node") -> None:
"""Add a successor node

Args:
son: son to add
successor: successor to add
"""
self._sons.append(son)
self._successors.append(successor)

def replace_son(self, ori_son: "Node", new_son: "Node") -> None:
"""Replace a son node. Do nothing if the node to replace is not a son
def replace_successor(self, old_successor: "Node", new_successor: "Node") -> None:
"""Replace a successor node. Do nothing if the node to replace is not a successor

Args:
ori_son: son to replace
new_son: son to replace with
old_successor: successor to replace
new_successor: successor to replace with
"""
for i, s in enumerate(self._sons):
if s.node_id == ori_son.node_id:
for i, s in enumerate(self._successors):
if s.node_id == old_successor.node_id:
idx = i
break
else:
return
self._sons[idx] = new_son
self._successors[idx] = new_successor

def set_sons(self, sons: List["Node"]) -> None:
"""Set the son nodes
def set_successors(self, successors: List["Node"]) -> None:
"""Set the successor nodes

Args:
sons: list of fathers to add
successors: list of successors to add
"""
self._sons = sons
self._successors = successors

@property
def sons(self) -> List["Node"]:
"""Returns the son nodes
def successors(self) -> List["Node"]:
"""Returns the successors nodes

Returns:
list(Node): list of sons
list(Node): list of successors
"""
return list(self._sons)
return list(self._successors)

@property
def son_true(self) -> Optional["Node"]:
def successor_true(self) -> Optional["Node"]:
if self.type in [NodeType.IF, NodeType.IFLOOP]:
return self._sons[0]
return self._successors[0]
return None

@property
def son_false(self) -> Optional["Node"]:
if self.type in [NodeType.IF, NodeType.IFLOOP] and len(self._sons) >= 1:
return self._sons[1]
def successor_false(self) -> Optional["Node"]:
if self.type in [NodeType.IF, NodeType.IFLOOP] and len(self._successors) >= 1:
return self._successors[1]
return None

# endregion
Expand Down Expand Up @@ -1046,6 +1049,43 @@ def __str__(self) -> str:
txt = str(self._node_type.value) + additional_info
return txt

def __getattr__(self, item: str):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid this, and do a breaking change

Doing the getattr trick will lead to more issues in the long term I think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've attempted to keep the compatibility layer minimal here to avoid future issues - however, I believe that implementing a complete breaking change will significantly simplify the code.

"""Get attribute wrapper.

Used for the breaking change to deprecate the usage of sons/fathers and use predecessors
and successors.
"""
replaced_functions = {
"add_son",
"remove_son",
"replace_son",
"son_false",
"son_true",
"sons",
"add_father",
"fathers",
"remove_father",
"set_fathers",
}

if item not in replaced_functions:
raise AttributeError(item)

if "son" in item:
new_function = item.replace("son", "successor")
elif "father" in item:
new_function = item.replace("father", "predecessor")
else:
raise AttributeError(item)

proxied_function = getattr(self, new_function)
logger.warning(
"Function %s is deprecated and will be removed in a future version. Please use %s instead.",
item,
new_function,
)
return proxied_function


# endregion
###################################################################################
Expand All @@ -1056,18 +1096,18 @@ def __str__(self) -> str:


def link_nodes(node1: Node, node2: Node) -> None:
node1.add_son(node2)
node2.add_father(node1)
node1.add_successor(node2)
node2.add_predecessor(node1)


def insert_node(origin: Node, node_inserted: Node) -> None:
sons = origin.sons
successors = origin.successors
link_nodes(origin, node_inserted)
for son in sons:
son.remove_father(origin)
origin.remove_son(son)
for successor in successors:
successor.remove_predecessor(origin)
origin.remove_successor(successor)

link_nodes(node_inserted, son)
link_nodes(node_inserted, successor)


def recheable(node: Node) -> Set[Node]:
Expand All @@ -1076,16 +1116,16 @@ def recheable(node: Node) -> Set[Node]:
:param node:
:return: set(Node)
"""
nodes = node.sons
nodes = node.successors
visited = set()
while nodes:
next_node = nodes[0]
nodes = nodes[1:]
if next_node not in visited:
visited.add(next_node)
for son in next_node.sons:
if son not in visited:
nodes.append(son)
for successor in next_node.successors:
if successor not in visited:
nodes.append(successor)
return visited


Expand Down
8 changes: 4 additions & 4 deletions slither/core/declarations/contract.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1478,8 +1478,8 @@ def add_constructor_variables(self) -> None:
constructor_variable, counter, v, prev_node.scope
)
v.node_initialization = next_node
prev_node.add_son(next_node)
next_node.add_father(prev_node)
prev_node.add_successor(next_node)
next_node.add_predecessor(prev_node)
prev_node = next_node
counter += 1
break
Expand Down Expand Up @@ -1510,8 +1510,8 @@ def add_constructor_variables(self) -> None:
constructor_variable, counter, v, prev_node.scope
)
v.node_initialization = next_node
prev_node.add_son(next_node)
next_node.add_father(prev_node)
prev_node.add_successor(next_node)
next_node.add_predecessor(prev_node)
prev_node = next_node
counter += 1

Expand Down
16 changes: 8 additions & 8 deletions slither/core/declarations/function.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1394,8 +1394,8 @@ def cfg_to_dot(self, filename: str):
f.write("digraph{\n")
for node in self.nodes:
f.write(f'{node.node_id}[label="{str(node)}"];\n')
for son in node.sons:
f.write(f"{node.node_id}->{son.node_id};\n")
for successor in node.successors:
f.write(f"{node.node_id}->{successor.node_id};\n")
Comment on lines +1397 to +1398
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the method cfg_to_dot and slithir_cfg_to_dot_str to use the new terminology.

-                for successor in node.successors:
-                    f.write(f"{node.node_id}->{successor.node_id};\n")
+                for successor in node.next:
+                    f.write(f"{node.node_id}->{successor.node_id};\n")

This change aligns the method with the new terminology used throughout the codebase.

Also applies to: 1460-1461

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for successor in node.successors:
f.write(f"{node.node_id}->{successor.node_id};\n")
for successor in node.next:
f.write(f"{node.node_id}->{successor.node_id};\n")


f.write("}\n")

Expand Down Expand Up @@ -1450,15 +1450,15 @@ def slithir_cfg_to_dot_str(self, skip_expressions: bool = False) -> str:
label += "\nIRs:\n" + "\n".join([str(ir) for ir in node.irs])
content += f'{node.node_id}[label="{label}"];\n'
if node.type in [NodeType.IF, NodeType.IFLOOP]:
true_node = node.son_true
true_node = node.successor_true
if true_node:
content += f'{node.node_id}->{true_node.node_id}[label="True"];\n'
false_node = node.son_false
false_node = node.successor_false
Comment on lines +1453 to +1456
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the method slithir_cfg_to_dot_str to use the new terminology for conditional branches.

-                true_node = node.successor_true
-                if true_node:
-                    content += f'{node.node_id}->{true_node.node_id}[label="True"];\n'
-                false_node = node.successor_false
-                if false_node:
-                    content += f'{node.node_id}->{false_node.node_id}[label="False"];\n'
+                true_node = node.next_true
+                if true_node:
+                    content += f'{node.node_id}->{true_node.node_id}[label="True"];\n'
+                false_node = node.next_false
+                if false_node:
+                    content += f'{node.node_id}->{false_node.node_id}[label="False"];\n'

This change aligns the method with the new terminology used throughout the codebase.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
true_node = node.successor_true
if true_node:
content += f'{node.node_id}->{true_node.node_id}[label="True"];\n'
false_node = node.son_false
false_node = node.successor_false
true_node = node.next_true
if true_node:
content += f'{node.node_id}->{true_node.node_id}[label="True"];\n'
false_node = node.next_false
if false_node:
content += f'{node.node_id}->{false_node.node_id}[label="False"];\n'

if false_node:
content += f'{node.node_id}->{false_node.node_id}[label="False"];\n'
else:
for son in node.sons:
content += f"{node.node_id}->{son.node_id};\n"
for successor in node.successors:
content += f"{node.node_id}->{successor.node_id};\n"

content += "}\n"
return content
Expand Down Expand Up @@ -1758,8 +1758,8 @@ def _get_last_ssa_variable_instances(
ret[name] = set()
ret[name] |= instances

for son in node.sons:
to_explore.append((son, dict(values)))
for successor in node.successors:
to_explore.append((successor, dict(values)))

return ret

Expand Down
Loading