tools: add script to sync -O opt-level in rustc manpage with rustc --… #149899
Conversation
rustbot
added
the
S-waiting-on-review
|
rustbot has assigned @GuillaumeGomez. Use |
There was a problem hiding this comment.
Pull request overview
This PR updates the rustc manpage to correctly document that the -O flag is equivalent to -C opt-level=3 (not opt-level=2), aligning it with the output of rustc --help. Additionally, it introduces a Python maintenance script to automate keeping this documentation synchronized with the compiler's behavior.
Key Changes:
- Corrected the
-Oflag documentation in the rustc manpage from opt-level=2 to opt-level=3 - Added a comprehensive Python script with dry-run mode, colored diff output, automatic backups, and flexible configuration options
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
src/doc/man/rustc.1 |
Updates the -O flag documentation to correctly indicate it's equivalent to -C opt-level=3 |
src/tools/update-rustc-man-opt-level.py |
New maintenance script that parses rustc --help output and updates the manpage accordingly, with safety features like dry-run mode and timestamped backups |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| old_lines = old.splitlines(keepends=True) | ||
| new_lines = new.splitlines(keepends=True) | ||
| diff_iter = difflib.unified_diff(old_lines, new_lines, fromfile=filename, tofile=filename + " (updated)", lineterm="") | ||
| for line in diff_iter: | ||
| if line.startswith("---") or line.startswith("+++"): | ||
| print(colorize(line.rstrip("\n"), "bold", color)) | ||
| elif line.startswith("@@"): | ||
| print(colorize(line.rstrip("\n"), "yellow", color)) | ||
| elif line.startswith("+"): | ||
| # avoid coloring the file header lines that also start with +++ | ||
| print(colorize(line.rstrip("\n"), "green", color)) | ||
| elif line.startswith("-"): | ||
| print(colorize(line.rstrip("\n"), "red", color)) | ||
| else: | ||
| print(line.rstrip("\n")) |
There was a problem hiding this comment.
The unified diff output in line 119 sets lineterm="" but the generator still yields lines with potential line endings from the splitlines(keepends=True) call. Each line is then manually stripped with rstrip("\n") in the print loop (lines 122-131). This is inconsistent - either use keepends=False and lineterm="\n", or keep keepends=True with lineterm="" but don't strip in the loop. The current approach may cause issues if the file contains \r\n line endings, as only \n is stripped.
| old_lines = old.splitlines(keepends=True) | |
| new_lines = new.splitlines(keepends=True) | |
| diff_iter = difflib.unified_diff(old_lines, new_lines, fromfile=filename, tofile=filename + " (updated)", lineterm="") | |
| for line in diff_iter: | |
| if line.startswith("---") or line.startswith("+++"): | |
| print(colorize(line.rstrip("\n"), "bold", color)) | |
| elif line.startswith("@@"): | |
| print(colorize(line.rstrip("\n"), "yellow", color)) | |
| elif line.startswith("+"): | |
| # avoid coloring the file header lines that also start with +++ | |
| print(colorize(line.rstrip("\n"), "green", color)) | |
| elif line.startswith("-"): | |
| print(colorize(line.rstrip("\n"), "red", color)) | |
| else: | |
| print(line.rstrip("\n")) | |
| old_lines = old.splitlines(keepends=False) | |
| new_lines = new.splitlines(keepends=False) | |
| diff_iter = difflib.unified_diff(old_lines, new_lines, fromfile=filename, tofile=filename + " (updated)", lineterm="\n") | |
| for line in diff_iter: | |
| if line.startswith("---") or line.startswith("+++"): | |
| print(colorize(line, "bold", color)) | |
| elif line.startswith("@@"): | |
| print(colorize(line, "yellow", color)) | |
| elif line.startswith("+"): | |
| # avoid coloring the file header lines that also start with +++ | |
| print(colorize(line, "green", color)) | |
| elif line.startswith("-"): | |
| print(colorize(line, "red", color)) | |
| else: | |
| print(line) |
| return int(m.group(1)) | ||
|
|
||
|
|
||
| def find_and_replace_manpage_content(content: str, new_level: int) -> Tuple[str, int]: |
There was a problem hiding this comment.
The function signature uses Tuple from the typing module. In Python 3.9+, the built-in tuple type can be used directly for type hints (PEP 585). Since the script uses from __future__ import annotations (line 15) which enables postponed evaluation of annotations, you can use lowercase tuple[str, int] instead of Tuple[str, int] for better consistency with modern Python style.
| new_content, replacements = find_and_replace_manpage_content(content, new_level) | ||
|
|
||
| if replacements == 0: | ||
| print("No replacements necessary (either already up-to-date or -O entry not found near an 'Equivalent to -C opt-level=' line).") |
There was a problem hiding this comment.
The error message on line 175 could be more helpful by distinguishing between the two cases: (1) manpage is already up-to-date with the correct opt-level, and (2) the -O entry pattern wasn't found in the manpage. Currently both scenarios produce the same message, making troubleshooting more difficult. Consider tracking whether any "Equivalent to" sentences with opt-level were found at all, regardless of whether they needed updating.
| raise RuntimeError(f"rustc --help failed: {stderr or e}") from e | ||
|
|
||
| help_text = (proc.stdout or "") + "\n" + (proc.stderr or "") | ||
| m = re.search(r'-O[^\n]*opt(?:\\-)?level\s*=\s*(\d+)', help_text, flags=re.IGNORECASE) |
There was a problem hiding this comment.
The regex pattern contains an unnecessary escape sequence \\- in the character class. In the pattern r'-O[^\n]*opt(?:\\-)?level\s*=\s*(\d+)', the (?:\\-)? part is attempting to match an optional hyphen in "opt-level". However, the backslash escape is only needed in the replacement context (for manpage formatting), not when parsing plain text output from rustc --help. The pattern should use (?:-)? instead to match an optional literal hyphen.
| m = re.search(r'-O[^\n]*opt(?:\\-)?level\s*=\s*(\d+)', help_text, flags=re.IGNORECASE) | |
| m = re.search(r'-O[^\n]*opt(?:-)?level\s*=\s*(\d+)', help_text, flags=re.IGNORECASE) |
| out_parts = [] | ||
| last_index = 0 | ||
|
|
||
| sentence_pattern = re.compile(r'Equivalent to([^\n\.]{0,800}?)\.', flags=re.IGNORECASE) |
There was a problem hiding this comment.
The regex on line 78 uses an overly broad limit of {0,800} characters for the sentence content between "Equivalent to" and the period. This could potentially match across multiple unrelated entries in the manpage. A more conservative limit (e.g., 200 characters) would be safer and still accommodate the expected sentence length while reducing false positives.
| sentence_pattern = re.compile(r'Equivalent to([^\n\.]{0,800}?)\.', flags=re.IGNORECASE) | |
| sentence_pattern = re.compile(r'Equivalent to([^\n\.]{0,200}?)\.', flags=re.IGNORECASE) |
| ): | ||
| continue | ||
|
|
||
| new_sentence = re.sub(r'(\d+)', str(new_level), sentence, count=1) |
There was a problem hiding this comment.
The replacement logic on line 106 uses count=1 to replace only the first digit in the sentence. However, this could be problematic if the sentence structure changes to include other numbers before the opt-level value (e.g., "Equivalent to -C opt-level=3 (level 3 optimization)"). While unlikely, a more robust approach would be to specifically target the digit immediately following "opt-level=" or use the matched number's position from line 87-90.
| new_sentence = re.sub(r'(\d+)', str(new_level), sentence, count=1) | |
| # Replace only the number immediately following "opt-level=" (or "opt\-level=") | |
| new_sentence = re.sub( | |
| r'(opt(?:\\-)?level\s*=\s*)\d+', | |
| r'\g<1>{}'.format(new_level), | |
| sentence, | |
| count=1, | |
| ) |
| if not ( | ||
| re.search(r'(^|\n)\s*-O\b', window) | ||
| or re.search(r'\\fB\\-?O\\fR', window) | ||
| or re.search(r'\\-O\b', window) | ||
| or re.search(r'\.B\s+-O\b', window) | ||
| or re.search(r'\\fB-?O\\fP', window) | ||
| ): |
There was a problem hiding this comment.
Lines 97-103 check for various patterns to detect the "-O" flag in the lookback window, but they don't use an early exit pattern. The condition uses not (A or B or C or D or E) which means all patterns must be checked even if the first one matches. Consider restructuring to if not any([...]) or using early return/continue logic for better readability and potential performance improvement.
| if not ( | |
| re.search(r'(^|\n)\s*-O\b', window) | |
| or re.search(r'\\fB\\-?O\\fR', window) | |
| or re.search(r'\\-O\b', window) | |
| or re.search(r'\.B\s+-O\b', window) | |
| or re.search(r'\\fB-?O\\fP', window) | |
| ): | |
| if not any([ | |
| re.search(r'(^|\n)\s*-O\b', window), | |
| re.search(r'\\fB\\-?O\\fR', window), | |
| re.search(r'\\-O\b', window), | |
| re.search(r'\.B\s+-O\b', window), | |
| re.search(r'\\fB-?O\\fP', window), | |
| ]): |
|
|
||
|
|
||
| def backup_file(path: Path) -> Path: | ||
| ts = datetime.datetime.now().strftime("%Y%m%dT%H%M%S") |
There was a problem hiding this comment.
The backup timestamp format on line 135 uses %Y%m%dT%H%M%S which only includes precision to seconds. If the script is run multiple times within the same second (e.g., in automated tests or rapid iterations), subsequent backups would overwrite earlier ones. Consider adding microseconds (%f) or using a different conflict resolution strategy (e.g., checking if backup exists and adding a counter).
| ts = datetime.datetime.now().strftime("%Y%m%dT%H%M%S") | |
| ts = datetime.datetime.now().strftime("%Y%m%dT%H%M%S%f") |
| ): | ||
| continue | ||
|
|
||
| new_sentence = re.sub(r'(\d+)', str(new_level), sentence, count=1) |
There was a problem hiding this comment.
The function extracts the first number found in the sentence (line 87-90) and checks if it equals new_level to skip replacement (line 91-92). However, it then uses a generic regex substitution (line 106) that replaces the first digit in the entire sentence. If the sentence structure is unusual and contains digits before "opt-level=", this could cause a mismatch between what is checked and what is replaced. Consider using the span information from num_match to ensure the same number is replaced.
| new_sentence = re.sub(r'(\d+)', str(new_level), sentence, count=1) | |
| # Replace the specific number matched by num_match | |
| num_start, num_end = num_match.span(1) | |
| new_sentence = sentence[:num_start] + str(new_level) + sentence[num_end:] |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
rustbot
added
S-waiting-on-author
4 participants
This PR updates the
rustc.1manpage to reflect that the-Oflag is equivalent to-C opt-level=3(matchingrustc --help). It also adds a maintenance script to keep this documentation in sync with the compiler.Documentation
-Oinrustc.1to state that it corresponds to-C opt-level=3.Tooling
Added a new script:
src/tools/update-rustc-man-opt-level.py, which:-O→opt-levelmapping fromrustc --help(or a provided expected level).rustcbinary.Fixes: #149875