Reviewers should probably either review the entire result of running the scripted diff or review the script itself thoroughly.
Made an attempt at reviewing the script. Leaned on mypy contrib/devtools/reg-settings.py --check-untyped-defs and ruff check to help elucidate types and find issues.
<details><summary>Suggestion diff</summary>
diff --git a/contrib/devtools/reg-settings.py b/contrib/devtools/reg-settings.py
index f006fd6b1f..40d5ff07af 100644
--- a/contrib/devtools/reg-settings.py
+++ b/contrib/devtools/reg-settings.py
@@ -37,7 +37,7 @@ DefaultValue = str | bool | None
class SettingType:
name: str
primary: bool = False
- defaults: set[str | None] = field(default_factory=set)
+ defaults: set[str | bool | None] = field(default_factory=set)
default_value: DefaultValue = False
includes: set[str] = field(default_factory=set)
"""Include paths needed in setting file for default expressions"""
@@ -90,22 +90,26 @@ class Setting: [@dataclass](/bitcoin-bitcoin/contributor/dataclass/)
class Range:
start: int
- end: int
+ end: int | None
-def get_files_with_args(src_dir):
+def get_files_with_args(src_dir: str) -> list[str]:
# Run git grep to find files containing AddArg/GetArg/GetIntArg/GetBoolArg/GetArgs
result = subprocess.run(
[
- "git", "grep", "-l", "AddArg(\|AddHiddenArgs(\|GetArg(\|GetPathArg(\|GetIntArg(\|GetBoolArg(\|GetArgs(\|IsArgSet(\|IsArgNegated(", "--", src_dir
+ "git", "grep", "-l", r"AddArg(\|AddHiddenArgs(\|GetArg(\|GetPathArg(\|GetIntArg(\|GetBoolArg(\|GetArgs(\|IsArgSet(\|IsArgNegated(", "--", src_dir
],
capture_output=True,
text=True
)
return result.stdout.splitlines()
-def parse_function_args(arg_str):
- args = []
- stack = []
+# Converts something like:
+# '("-foo", strprintf("helptext %i", DEFAULT), ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::OPTIONS)'
+# into:
+# (125, ['"-foo"', ' strprintf("helptext %i", DEFAULT', ' ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION', ' OptionsCategory::OPTIONS'])
+def parse_function_args(arg_str: str) -> tuple[int, list[str]]:
+ args: list[str] = []
+ stack: list[str] = []
quot = False
for pos, c in enumerate(arg_str):
if c == '"':
@@ -128,26 +132,28 @@ def parse_function_args(arg_str):
args[-1] += c
return pos, args
-def parse_calls(file_path):
- adds = []
- gets = []
+def parse_calls(file_path: str) -> tuple[list[AddArg], list[GetArg]]:
+ adds: list[AddArg] = []
+ gets: list[GetArg] = []
context = get_file_context(file_path)
namespace = get_file_namespace(file_path)
with open(file_path, 'r') as f:
content = f.read()
for match in re.finditer(r'\b(\w+)\.AddArg\((")', content):
call_len, (summary, help_text, flags, category) = parse_function_args(content[match.start(2)-1:])
+ arg_name_match = re.match(r'"([^"=(]+).*', summary)
+ assert arg_name_match
call = Call(
file=file_path,
position=match.start(),
call_text=content[match.start():match.start(2)+call_len],
obj_name=match.group(1),
- arg_name=re.match(r'"([^"=(]+).*', summary).group(1),
+ arg_name=arg_name_match.group(1),
context=context,
namespace=namespace,
)
help_text=help_text.strip()
- help_args = []
+ help_args: list[str] = []
if m := re.match(r"strprintf\(", help_text):
_, help_args = parse_function_args(help_text[m.end()-1:])
help_text = help_args[0].strip()
@@ -162,17 +168,18 @@ def parse_calls(file_path):
))
for match in re.finditer(r'( *)\b(\w+)\.AddHiddenArgs\((\{)', content):
call_len, call_args = parse_function_args(content[match.start(3):])
- hidden_args = []
+ hidden_args: list[AddArg] = []
for summary in call_args:
summary = summary.strip()
if not summary: continue
- call_text=content[match.start():match.start(3)+call_len+1]
+ arg_name_match = re.match(r'"([^"=(]+).*', summary)
+ assert arg_name_match
call = Call(
file=file_path,
position=match.start(),
call_text=content[match.start():match.start(3)+call_len+2],
obj_name=match.group(2),
- arg_name=re.match(r'"([^"=(]+).*', summary).group(1),
+ arg_name=arg_name_match.group(1),
context=context,
namespace=namespace,
)
@@ -215,6 +222,7 @@ def parse_calls(file_path):
DataType.DISABLED if function_name == "IsArgNegated" else
DataType.UNSET if function_name == "IsArgSet" else
None)
+ assert data_type
default_arg = call_args[1].strip() if len(call_args) > 1 else None
default_value = (
True if data_type == DataType.STRING and default_arg == '""' else
@@ -227,7 +235,7 @@ def parse_calls(file_path):
gets.append(GetArg(call=call, function_name=function_name, data_type=data_type, default_value=default_value, nots=len(match.group(1))))
return adds, gets
-def make_setting(settings, call):
+def make_setting(settings: dict[str, Setting], call: Call) -> Setting:
name = call.arg_name.lstrip("-")
if name in settings:
setting = settings[name]
@@ -235,7 +243,7 @@ def make_setting(settings, call):
setting = settings[name] = Setting(name)
return setting
-def flags_to_options(flag_str):
+def flags_to_options(flag_str: str) -> list[str]:
flags = set()
for flag in flag_str.split("|"):
flag = flag.strip()
@@ -263,9 +271,9 @@ def flags_to_options(flag_str):
raise Exception(f"Unknown flags {flags!r}")
return options
-def collect_argument_information(src_dir):
+def collect_argument_information(src_dir: str) -> dict[str, Setting]:
files = get_files_with_args(src_dir)
- settings: Dict[str, Setting] = {}
+ settings: dict[str, Setting] = {}
for file in files:
adds, gets = parse_calls(file)
for add in adds:
@@ -290,7 +298,7 @@ def collect_argument_information(src_dir):
for get in setting.gets:
if get.add is None and same_context(add, get):
get.add = add
- fake_adds = {} # map of (file path, arg name) -> AddArg
+ fake_adds: dict[tuple[str, str], AddArg] = {} # map of (file path, arg name) -> AddArg
for get in setting.gets:
if get.add is None and get.call.arg_name:
key = get.call.file, get.call.arg_name
@@ -303,7 +311,7 @@ def collect_argument_information(src_dir):
# Figure out setting types and default values.
setting_name = ''.join(NAME_MAP.get(word) or word.capitalize() for word in arg_name.split('-')) + "Setting"
- counter = collections.Counter()
+ counter: collections.Counter = collections.Counter()
for add in setting.adds:
add.include_path = add.call.file.replace(".cpp", "_settings.h")
add.rel_include_path = re.sub("^src/", "", add.include_path)
@@ -387,7 +395,7 @@ def collect_argument_information(src_dir):
return settings
-def setting_definitions(add):
+def setting_definitions(add: AddArg) -> tuple[list[str], set[str]]:
defs = []
includes = set()
# If this is hidden argument and there is non-hidden argument with the same
@@ -405,6 +413,7 @@ def setting_definitions(add):
"common::Disabled" if data_type == DataType.DISABLED else
"common::Unset" if data_type == DataType.UNSET else
None)
+ assert ctype
if None in setting_type.defaults:
ctype = f"std::optional<{ctype}>"
help_str = ""
@@ -447,26 +456,28 @@ def setting_definitions(add):
defs.append(f"\nusing {setting_type.name} = common::Setting<\n {add.summary}, {ctype}, {options_str}{help_str}>{extra};\n")
return defs, includes
-def addarg_replacement(add):
+def addarg_replacement(add: AddArg) -> str:
new_call = ""
if add.hidden_args is None:
new_call = register_str(add)
else:
for hadd in add.hidden_args:
if new_call: new_call += ";\n"
- spaces=re.match(" *", hadd.call.call_text).group()
+ hidden_match = re.match(" *", hadd.call.call_text)
+ assert hidden_match
+ spaces=hidden_match.group()
new_call += f"{spaces}{register_str(hadd.nonhidden_arg or hadd, hidden=hadd.nonhidden_arg is not None)}"
return new_call
-def arg_name(add):
+def arg_name(add: AddArg) -> str:
default_data_type = min(add.data_types.keys())
return add.data_types[default_data_type].name
-def register_str(add, hidden=False):
+def register_str(add: AddArg, hidden: bool=False) -> str:
register_args = ", ".join([add.call.obj_name] + add.extern_args)
return f"{arg_name(add)}{'::Hidden' if hidden else ''}::Register({register_args})"
-def getarg_replacement(get):
+def getarg_replacement(get: GetArg) -> str:
if get.data_type == DataType.UNSET:
method = "Value"
suffix = ".isNull()"
@@ -476,6 +487,7 @@ def getarg_replacement(get):
else:
method = "Get"
suffix = ""
+ assert get.add
setting_type = get.add.data_types.get(get.data_type) or get.add.data_types[min(get.add.data_types.keys())]
default_arg = ""
if get.default_value and not setting_type.default_value:
@@ -490,7 +502,7 @@ def getarg_replacement(get):
new_call += f"{setting_type.name}::{method}({get.call.obj_name}{default_arg}){suffix}"
return new_call
-def add_to_file(file_path, local_include, system_include=(), defs=()):
+def add_to_file(file_path: str, local_include: list[str], system_include: list[str]=[], defs: list[str]=[]):
if os.path.exists(file_path):
with open(file_path, 'r') as f:
lines = f.readlines()
@@ -510,17 +522,16 @@ def add_to_file(file_path, local_include, system_include=(), defs=()):
lines.extend(["\n", f"#endif // {guard}\n"])
# Identify include blocks and their positions
- blocks = []
+ blocks: list[Range] = []
self_include = f"#include <{file_path.replace('src/', '').replace('.cpp', '.h')}>"
- first = last = self = None
+ first = last = None
for i, line in enumerate(lines):
if line.startswith('#include') and "IWYU pragma: keep" not in line and not line.startswith(self_include):
if not blocks or blocks[-1].end is not None:
blocks.append(Range(i, None))
elif blocks and blocks[-1].end is None:
blocks[-1].end = i
- elif line.startswith('#include'):
- self = True
+
if first is None and not line.startswith("//") and not line.startswith("#ifndef") and not line.startswith("#define") and line != "\n":
first = i
if line != "\n" and not line.startswith("#endif") and not line.startswith("} // namespace "):
@@ -528,6 +539,8 @@ def add_to_file(file_path, local_include, system_include=(), defs=()):
if len(blocks) == 0:
# If there are no include blocks, add an empty one where includes should go.
+ assert first
+ assert last
m = min(first, last)
blocks.append(Range(m, m))
if len(blocks) == 1:
@@ -535,6 +548,7 @@ def add_to_file(file_path, local_include, system_include=(), defs=()):
# local or system includes, but reasonably heuristic is to assume it
# contains local includes in .cpp files and system includes in .h files.
if file_path.endswith(".cpp"):
+ assert blocks[0].end
blocks.append(Range(blocks[0].end, blocks[0].end))
else:
blocks.insert(0, Range(blocks[0].start, blocks[0].start))
@@ -558,7 +572,7 @@ def add_to_file(file_path, local_include, system_include=(), defs=()):
with open(file_path, 'w') as f:
f.writelines(lines)
-def replace_in_file(file_path, old, new, replacements=None):
+def replace_in_file(file_path: str, old: str, new: str, replacements: list[tuple[str, str]] | None=None):
with open(file_path, 'r') as f:
content = f.read()
if replacements is not None:
@@ -572,29 +586,33 @@ def replace_in_file(file_path, old, new, replacements=None):
with open(file_path, 'w') as f:
f.write(new_content)
-def modify_source_files(settings, git_commit=False):
+def modify_source_files(settings: dict[str, Setting], git_commit: bool=False):
# map file path->list of (old, new) tuples with GetArg->Get replacements made so far
- replacements = collections.defaultdict(list)
+ replacements: dict[str, list[tuple[str, str]]] = collections.defaultdict(list)
for setting in settings.values():
for add in setting.adds:
if not add.fake:
replace_in_file(add.call.file, add.call.call_text, addarg_replacement(add))
+ assert add.rel_include_path
add_to_file(add.call.file, [add.rel_include_path])
for get in setting.gets:
if get.add is add:
replace_in_file(get.call.file, get.call.call_text, getarg_replacement(get),
replacements[get.call.file])
+ assert get.add.rel_include_path
add_to_file(get.call.file, [get.add.rel_include_path])
defs, def_includes = setting_definitions(add)
if defs:
+ assert add.include_path
add_to_file(add.include_path,
[include for include in def_includes | {"common/setting.h"}],
["string", "vector"], defs)
if git_commit and subprocess.run(["git", "status", "--porcelain", "--untracked-files=no"], stdout=subprocess.PIPE, check=True).stdout:
+ assert add.include_path
subprocess.run(["git", "add", "-N", add.include_path], check=True)
msg = f"{add.rel_include_path}: Add {arg_name(add.nonhidden_arg or add)}"
subprocess.run(["git", "commit", "-am", msg], check=True)
@@ -644,7 +662,6 @@ ARG_PATTERNS = {
"DEFAULT_ASMAP_FILENAME": PatternOptions(include_path="init.h", runtime=True),
"DEFAULT_AVOIDPARTIALSPENDS": PatternOptions(include_path="wallet/coincontrol.h", runtime=True),
"DEFAULT_BENCH_FILTER": PatternOptions(runtime=True),
- "DEFAULT_BLOCKFILTERINDEX": PatternOptions(include_path="index/blockfilterindex.h"),
"DEFAULT_BLOCKFILTERINDEX": PatternOptions(include_path="index/blockfilterindex.h", runtime=True),
"DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN": PatternOptions(include_path="net_processing.h"),
"DEFAULT_CHOOSE_DATADIR": PatternOptions(include_path="qt/intro.h"),
</details>
