diff --git a/buildscripts/check_for_noexcept.py b/buildscripts/check_for_noexcept.py index b6f5999c935..5a3e052e1f4 100644 --- a/buildscripts/check_for_noexcept.py +++ b/buildscripts/check_for_noexcept.py @@ -3,13 +3,16 @@ Check for changes in noexcept usage between the given revisions and output a file that can be used by evergreen to post a github commit check annotating a PR with the results. -The algorithm here basically handles two cases: (1) straightforward cases where -you're "just" adding or removing noexcept from an existing function; and (2) -some cases where you're adding a new noexcept function or functions. Case (1) -works like this: if we have a diff block (that is, a block of addition or -deletion) that's a single line and mentions noexcept on only one side, count -it. Case (2) works like this: if noexcept is mentioned on the b-side of the -diff and not the a-side, assume new noexcept code has been added, and count it. +The algorithm here handles a few cases: (1) straightforward cases where you're "just" +adding or removing noexcept from an existing function; (2) some cases where you're adding +a new noexcept function or functions; (3) cases where you're adding a non-noexcept move +constructor or assignment operation. Case (1) works like this: if we have a diff block +(that is, a block of addition or deletion) that's only mentions noexcept on only one side, +those mentions are not detected as move operations, and the only diff between the two +sides is related to the noexcept(s), count it. Case (2) works like this: if noexcept is +mentioned more times on the b-side of the diff than the a-side (excluding move +operations), assume new noexcept code has been added, and count it. Case (3) simply +detects added code that looks like a move operation and alerts if it isn't noexcept. """ import difflib @@ -45,8 +48,8 @@ SUMMARY_TEXT = ( # These fields go in the annotations in the "Files Changed" tab. These fields are rendered # verbatim (i.e., markdown doesn't work). -ANNOTATION_TITLE_TEXT = TITLE_TEXT -ANNOTATION_MESSAGE_TEXT = ( +ANNOTATION_TITLE_TEXT_NOEXCEPT_CHANGE = TITLE_TEXT +ANNOTATION_MESSAGE_TEXT_NOEXCEPT_CHANGE = ( "A change in noexcept usage has been detected.\n" "As noexcept is a frequently-misunderstood feature,\n" "please refer to the documentation on noexcept usage\n" @@ -61,6 +64,17 @@ ANNOTATION_MESSAGE_TEXT = ( "Documentation can be found here: " + DOCUMENTATION_URL ) +ANNOTATION_TITLE_TEXT_NON_NOEXCEPT_MOVE = "Non-noexcept move operation added" +ANNOTATION_MESSAGE_TEXT_NON_NOEXCEPT_MOVE = ( + "A non-noexcept move operation has been added in this change.\n" + "Move operations should generally be marked noexcept to\n" + "avoid unnecessary copies during standard library operations.\n" + "If the move operation truly can throw and is intentionally not\n" + "noexcept, please disregard this message. " + "\n\n" + "Documentation can be found here: " + DOCUMENTATION_URL +) + def configure_logging() -> None: logging.basicConfig( @@ -82,43 +96,45 @@ def get_git_diff(repo: git.Repo, base: git.Commit, rev) -> git.DiffIndex: return base.diff(rev) -class ChangeKind(Enum): - ADDITION = auto() - REMOVAL = auto() +def get_matching_blocks(lhs: str, rhs: str) -> list[tuple[int, int, int, int]]: + """ + Find the (line-level) matching blocks between the two given strings, representing the left-hand + side (lhs) and right-hand side (rhs) of a file with a diff. That is, they represent the file's + contents before (lhs) and after (rhs) the change. Return value is a tuple of (lhs_start, + lhs_end, rhs_start, rhs_end) for each matching block. Unlike difflib's + SequenceMatcher.get_matching_blocks, we do not return the dummy size-0 block at the end. + """ + lhs_lines = lhs.splitlines() + rhs_lines = rhs.splitlines() + matcher = difflib.SequenceMatcher(None, lhs_lines, rhs_lines, autojunk=False) + blocks = matcher.get_matching_blocks() + line_diffs = [] + for block in blocks: + if block.size == 0: + # The last block is a dummy block a and b set to the sequence lengths and size 0. + # We don't return such a block since we don't need it. + break + lhs_start = block.a + rhs_start = block.b + line_diffs.append((lhs_start, lhs_start + block.size, rhs_start, rhs_start + block.size)) + return line_diffs -@dataclass -class UnboundNoexceptChange: - kind: ChangeKind - is_certain: bool - index: int - - def bind_to_line(self, line: int): - return LineBoundNoexceptChange(**asdict(self), line=line) - - -@dataclass -class LineBoundNoexceptChange(UnboundNoexceptChange): - line: int - - def bind_to_file(self, file: str): - return FileBoundNoexceptChange(**asdict(self), file=file) - - -@dataclass -class FileBoundNoexceptChange(LineBoundNoexceptChange): - file: str - - def make_annotation(self) -> dict[str, Any]: - return { - "path": self.file, - "annotation_level": "notice", - "title": ANNOTATION_TITLE_TEXT, - "message": ANNOTATION_MESSAGE_TEXT, - "raw_details": "", - "start_line": self.line, - "end_line": self.line, - } +def get_nonmatching_blocks(lhs: str, rhs: str) -> list[tuple[int, int, int, int]]: + matches = get_matching_blocks(lhs, rhs) + lhs_index = 0 + rhs_index = 0 + blocks = [] + lhs_line_count = len(lhs.splitlines()) + rhs_line_count = len(rhs.splitlines()) + for lhs_start, lhs_end, rhs_start, rhs_end in matches: + if lhs_index < lhs_start or rhs_index < rhs_start: + blocks.append((lhs_index, lhs_start, rhs_index, rhs_start)) + lhs_index = lhs_end + rhs_index = rhs_end + if lhs_index < lhs_line_count or rhs_index < rhs_line_count: + blocks.append((lhs_index, lhs_line_count, rhs_index, rhs_line_count)) + return blocks def get_line_for_char(text: str, i: int) -> int: @@ -126,148 +142,280 @@ def get_line_for_char(text: str, i: int) -> int: return text[:i].count("\n") + 1 -def analyze_uncertain_insdel(insdel: str, kind: ChangeKind) -> list[UnboundNoexceptChange]: - return [ - UnboundNoexceptChange(kind=kind, is_certain=False, index=match.start()) - for match in re.finditer("noexcept", insdel) +def strip_noexcept(s: str) -> str: + return re.sub(r"\snoexcept(\s*\(.*\))?(?=\s|;|{|$)", "", s) + + +class SnippetKind(Enum): + # Noexcept is the most general kind of snippet, it simply refers to an instance of noexcept + # that wasn't classified as more specific type of snippet. + Noexcept = auto() + # A NoexceptAddition is a snippet representing a definite addition of noexcept that wasn't + # classified as a move. + NoexceptAddition = auto() + # A NoexceptRemoval is a snippet representing a definite removal of noexcept that wasn't + # classified as a move. + NoexceptRemoval = auto() + # A NoexceptMove is a move operation that is marked noexcept. + NoexceptMove = auto() + # A NonNoexceptMove is a move operation that is not marked noexcept. + NonNoexceptMove = auto() + + +@dataclass +class UnboundSnippet: + kind: SnippetKind + start: int + end: int + identifier: str | None + + def is_move(self) -> bool: + return self.kind in (SnippetKind.NoexceptMove, SnippetKind.NonNoexceptMove) + + # We take a separate alert line since snippets on the LHS that are alertable need to be attached + # to a line on the RHS. If it's not provided, we just use the start line of the snippet, + # under the assumption that it's from the RHS. + def _bind_to_text(self, s: str, alert_line: int | None = None): + start = get_line_for_char(s, self.start) + end = get_line_for_char(s, self.end) + if alert_line is None: + alert_line = start + return LineBoundSnippet( + **asdict(self), line_start=start, line_end=end, alert_line=alert_line + ) + + def bind_to_rhs(self, s: str): + return self._bind_to_text(s) + + def bind_to_lhs(self, s: str, alert_line: int): + return self._bind_to_text(s, alert_line=alert_line) + + +@dataclass +class LineBoundSnippet(UnboundSnippet): + line_start: int + line_end: int + alert_line: int + + def bind_to_file(self, file: str): + return FileBoundSnippet(**asdict(self), file=file) + + +@dataclass +class FileBoundSnippet(LineBoundSnippet): + file: str + + +class AlertKind(Enum): + NonNoexceptMove = auto() + Noexcept = auto() + + +@dataclass +class Alert: + file: str + line: int + kind: AlertKind + identifier: str | None + + def get_title(self) -> str: + if self.kind == AlertKind.Noexcept: + return ANNOTATION_TITLE_TEXT_NOEXCEPT_CHANGE + else: + assert self.kind == AlertKind.NonNoexceptMove + return ANNOTATION_TITLE_TEXT_NON_NOEXCEPT_MOVE + + def get_message(self) -> str: + if self.kind == AlertKind.Noexcept: + return ANNOTATION_MESSAGE_TEXT_NOEXCEPT_CHANGE + else: + assert self.kind == AlertKind.NonNoexceptMove + return ANNOTATION_MESSAGE_TEXT_NON_NOEXCEPT_MOVE + + def make_annotation(self) -> dict[str, Any]: + return { + "path": self.file, + "annotation_level": "notice", + "title": self.get_title(), + "message": self.get_message(), + "raw_details": "", + "start_line": self.line, + "end_line": self.line, + } + + +def get_noexcepts(s: str) -> list[UnboundSnippet]: + noexcept_patterns = [ + re.compile(r"\bnoexcept\b"), + ] + noexcepts = [] + for pattern in noexcept_patterns: + for match in pattern.finditer(s): + snippet = UnboundSnippet( + start=match.start(), + end=match.end(), + identifier=None, + kind=SnippetKind.Noexcept, + ) + noexcepts.append(snippet) + return noexcepts + + +def _get_all_move_operations(s: str) -> list[tuple[int, int, str]]: + """ + Find all move constructors and assignment operators in the given string. Returns a list of + (start_char, end_char, identifier) triples, where identifier is what we detected as the + name of the move operation (e.g., TypeName::TypeName for a move constructor or + TypeName::operator= for a move assignment). + """ + move_constructor_patterns = [ + # TypeName(TypeName&&...) - TypeName can be anything capitalized and we don't check the two + # instances of Typename are the same. We also don't check that TypeName is the name of the + # type, so this could be a normal function with a bad name. We do try to check that there + # are no additional parameters beyond the move parameter. + re.compile(r"\b[A-Z]\w*\s*\(([A-Z]\w*)\s*&&[^,)]*\)", re.MULTILINE), + ] + move_assignment_patterns = [ + # operator=(Typename&&...) - TypeName can be anything capitalized and we do try to check + # that there are no additional parameters beyond the move parameter. + re.compile(r"\boperator=\s*\(([A-Z]\w*)\s*&&[^,)]*\)", re.MULTILINE), ] + move_operations = [] -def analyze_insdel(insdel: str, kind: ChangeKind) -> list[UnboundNoexceptChange]: - # If the inserted or deleted string is a single line, we'll assume the presence of noexcept - # indicates a true insertion or deletion of noexcept. - if insdel.count("noexcept") == 1 and insdel.count("\n") == 0: - return [UnboundNoexceptChange(kind=kind, is_certain=True, index=insdel.find("noexcept"))] - return analyze_uncertain_insdel(insdel, kind) + for pattern in move_constructor_patterns: + for match in pattern.finditer(s): + move_operations.append( + (match.start(), match.end(), match.group(1) + "::" + match.group(1)) + ) + + for pattern in move_assignment_patterns: + for match in pattern.finditer(s): + move_operations.append((match.start(), match.end(), match.group(1) + "::operator=")) + + return move_operations -def analyze_deletion(deleted: str) -> list[UnboundNoexceptChange]: - return analyze_insdel(deleted, ChangeKind.REMOVAL) +def get_move_operations(s: str) -> list[UnboundSnippet]: + """ + Find and return all move operations (constructors or assignment operators) in the given string. + """ + matches = _get_all_move_operations(s) + snippets = [] + for start, end, identifier in matches: + # end is the closing bracket of the move operation's parameter list. + # We look for the noexcept token between there and the next ;, {, or end of string, + # and we'll also extend the snippet to that point if noexcept is found. + move_op_end = min([s.find(c, end) if c in s else len(s) for c in (";", "{")]) + trailing_text = s[end:move_op_end] + is_noexcept = "noexcept" in trailing_text + snippet_kind = SnippetKind.NoexceptMove if is_noexcept else SnippetKind.NonNoexceptMove + snippet_end = move_op_end if is_noexcept else end + snippet = UnboundSnippet( + start=start, + end=snippet_end, + identifier=identifier, + kind=snippet_kind, + ) + snippets.append(snippet) + + return snippets -def analyze_insertion(inserted: str) -> list[UnboundNoexceptChange]: - return analyze_insdel(inserted, ChangeKind.ADDITION) +def filter_snippets( + snippets: list[LineBoundSnippet], start: int, end: int +) -> list[LineBoundSnippet]: + def is_in_range(snippet: LineBoundSnippet, start: int, end: int) -> bool: + return start <= snippet.line_start <= end or start <= snippet.line_end <= end + + return [s for s in snippets if is_in_range(s, start, end)] -def analyze_edit(deleted: str, inserted: str) -> list[UnboundNoexceptChange]: - if "noexcept" not in inserted: - return analyze_deletion(deleted) - if "noexcept" not in deleted: - return analyze_insertion(inserted) +def drop_covered_noexcept_snippets(snippets: list[LineBoundSnippet]) -> list[LineBoundSnippet]: + """ + Remove any snippets of less-specific types (Noexcept, NoexceptAddition, NoexceptRemoval) that + are fully covered by snippets of more-specific types (NoexceptMove). + """ - # Both sides mention noexcept - we have far less certainty. - deletions = analyze_uncertain_insdel(deleted, ChangeKind.REMOVAL) - insertions = analyze_uncertain_insdel(inserted, ChangeKind.ADDITION) - - return insertions + deletions + noexcept_moves = [s for s in snippets if s.kind == SnippetKind.NoexceptMove] + filtered = [] + for snippet in snippets: + if snippet.is_move(): + filtered.append(snippet) + continue + is_covered = any( + move_op.start <= snippet.start <= move_op.end for move_op in noexcept_moves + ) + if not is_covered: + filtered.append(snippet) + return filtered -# This is a wrapper around difflib.SeqeuenceMatcher's get_matching_blocks. -# SequenceMatcher synchronizes the sequences quite aggressively when diffing a character sequence, -# often inserting a very small match in the middle of what we'd like to consider a single edit. -# For example, considering the strings "int f() noexcept const" and "int f() const", -# it might align them like this: -# int f() noexcept const -# int f() c onst -# producing two insertions of "noex" and "ept", making it hard for us to analyze individual edits. -# This wrapper performs the match at the level of words and translates back to a character-based -# match sequence. The matching ignores whitespace, so matching regions may be of different sizes -# on each side and may indeed differ in whitespace. -def get_matching_blocks(lhs: str, rhs: str) -> list[tuple[int, int, int, int]]: - def skip_whitespace(s: str, i: int) -> int: - while i < len(s) and s[i].isspace(): - i += 1 - return i +def analyze_text_diff(lhs: str, rhs: str) -> tuple[list[LineBoundSnippet], list[LineBoundSnippet]]: + """ + Analyze the text diff between lhs and rhs, returning snippets of noexcept-relevant changes. + Returns two lists: snippets from lhs and snippets from rhs. + """ - def skip_word(s: str, word: str, i: int) -> int: - for char in word: - assert i < len(s) - assert s[i] == char - i += 1 - return i + def normalize(s: str) -> str: + return re.sub(r"\s+", "", s) - def advance(s: str, word: str, i: int) -> int: - i = skip_whitespace(s, i) - i = skip_word(s, word, i) - i = skip_whitespace(s, i) - return i + line_diff = get_nonmatching_blocks(lhs, rhs) - lhs_words = lhs.split() - rhs_words = rhs.split() - s = difflib.SequenceMatcher(None, lhs_words, rhs_words, autojunk=False) - matching_blocks = s.get_matching_blocks() - lhs_word_index = 0 - lhs_char_index = 0 - rhs_word_index = 0 - rhs_char_index = 0 - new_blocks = [] - for block in matching_blocks: - if block.size == 0: - # The last block is a dummy block a and b set to the sequence lengths and size 0. - # We don't return such a block since we don't need it. - break + # Get all snippets, considering the whole file. We'll filter down to those that overlap with + # each diff block later. This ensures that when finding snippets, we have enough context. + # For example, if there's code like: + # TypeName& operator=(TypeName&& other) + # noexcept { + # and the diff only covers the "noexcept {" line, we still want to look at the line before + # to realize it's a move assignment operator. + all_pre_snippets = drop_covered_noexcept_snippets(get_noexcepts(lhs) + get_move_operations(lhs)) + all_post_snippets = drop_covered_noexcept_snippets( + get_noexcepts(rhs) + get_move_operations(rhs) + ) - while lhs_word_index < block.a: - lhs_char_index = advance(lhs, lhs_words[lhs_word_index], lhs_char_index) - lhs_word_index += 1 - while rhs_word_index < block.b: - rhs_char_index = advance(rhs, rhs_words[rhs_word_index], rhs_char_index) - rhs_word_index += 1 + pre, post = [], [] + for lstart, lend, rstart, rend in line_diff: + # Ensure snippets on the LHS generate alerts on the corresponding RHS line, as alerts + # can only appear on the RHS. Snippets already on the RHS just use their own line. + pre_snippets = [s.bind_to_lhs(lhs, rstart) for s in all_pre_snippets] + post_snippets = [s.bind_to_rhs(rhs) for s in all_post_snippets] - block_lhs_start = lhs_char_index - while lhs_word_index < block.a + block.size: - lhs_char_index = advance(lhs, lhs_words[lhs_word_index], lhs_char_index) - lhs_word_index += 1 - block_rhs_start = rhs_char_index - while rhs_word_index < block.b + block.size: - rhs_char_index = advance(rhs, rhs_words[rhs_word_index], rhs_char_index) - rhs_word_index += 1 + # Filter snippets down to those that overlap with this diff block. + pre_snippets = filter_snippets(pre_snippets, lstart, lend) + post_snippets = filter_snippets(post_snippets, rstart, rend) - new_blocks.append((block_lhs_start, lhs_char_index, block_rhs_start, rhs_char_index)) + rhs_text = " ".join(rhs.splitlines()[rstart:rend]) + lhs_text = " ".join(lhs.splitlines()[lstart:lend]) - return new_blocks + # Special case: if there are snippets on only one side, they're all of type Noexcept, + # and the only textual difference between the two sides is the presence or absence of + # noexcept, we can promote those snippets to NoexceptAddition or NoexceptRemoval. + # This is the case where we can definitively tell that noexcept was added to or removed + # from existing code, and that code isn't a move operation (probably, up to the accuracy + # of our move detection). + if ( + len(pre_snippets) == 0 + and len(post_snippets) > 0 + and all(s.kind == SnippetKind.Noexcept for s in post_snippets) + and normalize(strip_noexcept(rhs_text)) == normalize(lhs_text) + ): + for s in post_snippets: + s.kind = SnippetKind.NoexceptAddition + if ( + len(post_snippets) == 0 + and len(pre_snippets) > 0 + and all(s.kind == SnippetKind.Noexcept for s in pre_snippets) + and normalize(strip_noexcept(lhs_text)) == normalize(rhs_text) + ): + for s in pre_snippets: + s.kind = SnippetKind.NoexceptRemoval -def get_nonmatching_blocks(lhs: str, rhs: str) -> list[tuple[int, int, int, int]]: - matches = get_matching_blocks(lhs, rhs) - lhs_i = 0 - rhs_i = 0 - blocks = [] - for lhs_start, lhs_end, rhs_start, rhs_end in matches: - if lhs_i < lhs_start or rhs_i < rhs_start: - blocks.append((lhs_i, lhs_start, rhs_i, rhs_start)) - lhs_i = lhs_end - rhs_i = rhs_end - if lhs_i < len(lhs) or rhs_i < len(rhs): - blocks.append((lhs_i, len(lhs), rhs_i, len(rhs))) - return blocks + pre += pre_snippets + post += post_snippets - -def analyze_text_diff(lhs: str, rhs: str) -> list[LineBoundNoexceptChange]: - blocks = get_nonmatching_blocks(lhs, rhs) - changes = [] - for lhs_start, lhs_end, rhs_start, rhs_end in blocks: - deleted = lhs[lhs_start:lhs_end] - inserted = rhs[rhs_start:rhs_end] - block_changes = analyze_edit(deleted, inserted) - - # Line number always refers to the rhs of the diff (post-changes). Annotations can only - # be put on that side. - rhs_start_line = get_line_for_char(rhs, rhs_start) - - # Indices are relative to the start of inserted/deleted, convert them to be relative - # to lhs/rhs. - for change in block_changes: - if change.kind == ChangeKind.ADDITION: - # For additions, we bind to the line where noexcept appears. - change.index += rhs_start - changes += [change.bind_to_line(get_line_for_char(rhs, change.index))] - elif change.kind == ChangeKind.REMOVAL: - # For removals, we bind to the start of the diff block due to the constraint that - # annotations go on the right-hand side. - change.index += lhs_start - changes += [change.bind_to_line(rhs_start_line)] - return changes + return pre, post def is_allowed_path(path: str) -> bool: @@ -278,7 +426,9 @@ def is_allowed_path(path: str) -> bool: return True -def analyze_single_git_diff(diff: git.Diff) -> list[LineBoundNoexceptChange]: +def analyze_single_git_diff( + diff: git.Diff, +) -> tuple[list[LineBoundSnippet], list[LineBoundSnippet]]: # Blobs can be None. For example, if the diff is a file deletion, the b_blob will be None. # In that case, we want the None to be treated as empty string for our text diff. def decode(blob: git.Blob | None) -> str: @@ -287,24 +437,108 @@ def analyze_single_git_diff(diff: git.Diff) -> list[LineBoundNoexceptChange]: return blob.data_stream.read().decode("utf-8") if not is_allowed_path(diff.a_path): - return [] + return [], [] if not is_allowed_path(diff.b_path): - return [] + return [], [] return analyze_text_diff(decode(diff.a_blob), decode(diff.b_blob)) -def analyze_git_diff(diffs: list[git.Diff]) -> list[FileBoundNoexceptChange]: - changes = [] +def analyze_git_diff( + diffs: list[git.Diff], +) -> tuple[list[FileBoundSnippet], list[FileBoundSnippet]]: + pre, post = [], [] for diff in diffs: - diff_changes = analyze_single_git_diff(diff) - changes += [change.bind_to_file(diff.b_path) for change in diff_changes] + file_pre, file_post = analyze_single_git_diff(diff) + pre += [change.bind_to_file(diff.a_path) for change in file_pre] + post += [change.bind_to_file(diff.b_path) for change in file_post] - return changes + return pre, post -def make_check_result(changes: list[FileBoundNoexceptChange]) -> dict[str, Any]: - annotations = [change.make_annotation() for change in changes] +def analyze_changes(pre: list[FileBoundSnippet], post: list[FileBoundSnippet]) -> list[Alert]: + """ + Given pre- and post-change snippets, analyze the, and return any alerts that should be raised. + """ + alerts = [] + + # First, check for non-noexcept moves being added (including the case where we remove noexcept + # from an existing noexcept move). + non_noexcept_moves_post = [s for s in post if s.kind == SnippetKind.NonNoexceptMove] + non_noexcept_move_identifiers_pre = { + s.identifier for s in pre if s.kind == SnippetKind.NonNoexceptMove + } + for s in non_noexcept_moves_post: + if s.identifier in non_noexcept_move_identifiers_pre: + # This was already a non-noexcept move, it's not the PR's fault. + LOGGER.info("Skipping alert for existing non-noexcept move", identifier=s.identifier) + continue + # This is a new non-noexcept move (or noexcept was removed from an existing move) - alert. + LOGGER.info("Alerting for non-noexcept move", identifier=s.identifier) + alerts.append( + Alert( + file=s.file, + line=s.alert_line, + kind=AlertKind.NonNoexceptMove, + identifier=s.identifier, + ) + ) + + # Ignore any move operations, noexcept or not. We've already checked for the bad case related + # to moves, so now we just need to look at other non-move noexcept changes. + non_move_pre = [s for s in pre if not s.is_move()] + non_move_post = [s for s in post if not s.is_move()] + + # Now we should be left with snippets for non-move noexcept changes only. First, check for the + # case where we can definitively detect adding or removing noexcept from existing code. + alerted_for_noexcept_change = False + for s in non_move_pre + non_move_post: + if s.kind in (SnippetKind.NoexceptAddition, SnippetKind.NoexceptRemoval): + if alerted_for_noexcept_change: + LOGGER.info( + "Not alerting for definitive noexcept change (already alerted once)", + file=s.file, + line=s.alert_line, + ) + else: + LOGGER.info( + "Alerting for defintive noexcept change", file=s.file, line=s.alert_line + ) + alerts.append( + Alert( + file=s.file, + line=s.alert_line, + kind=AlertKind.Noexcept, + identifier=s.identifier, + ) + ) + alerted_for_noexcept_change = True + + # If the count increased, then we're adding new noexcept usage somewhere (either in new code or + # in making old code noexcept). We may not have been able to detect these as definitive + # additions above. This could happen if e.g., code is both moved to a new file and made + # noexcept, or if new code is written using noexcept. If the count descreased, we might just be + # deleting code that happens to contain noexcept, so we won't alert in that case. + if not alerted_for_noexcept_change and len(non_move_post) > len(non_move_pre): + if alerted_for_noexcept_change: + LOGGER.info("Not alerting for new noexcept usage (already alerted once)") + else: + LOGGER.info("Alerting for net new noexcept usage") + alerts.append( + Alert( + file=non_move_post[0].file, + line=non_move_post[0].alert_line, + kind=AlertKind.Noexcept, + identifier=s.identifier, + ) + ) + alerted_for_noexcept_change = True + + return alerts + + +def make_check_result(alerts: list[Alert]) -> dict[str, Any]: + annotations = [alert.make_annotation() for alert in alerts] return { "title": TITLE_TEXT, "summary": SUMMARY_TEXT, @@ -314,28 +548,9 @@ def make_check_result(changes: list[FileBoundNoexceptChange]) -> dict[str, Any]: def check_for_noexcept_changes(diffs: list[git.Diff]) -> dict[str, Any]: - changes = analyze_git_diff(diffs) - - # If we're certain that noexcept was added or removed in some places, make annotations based - # on the changes we're sure of. - certain_changes = [change for change in changes if change.is_certain] - if len(certain_changes) > 0: - return make_check_result(certain_changes) - - # We've detected changes involving noexcept, but we're not certain that they're meaningful. - # For example, removing a noexcept function altogether would be picked up as a noexcept - # deletion, but it's not one we want to report on. - # Conservatively, we'll only report on uncertain noexcept changes if there are additions of - # noexcept with _no_ removals. This is still not perfect - we might pick up comments, or - # someone declaring a noexcept function that already has a definition elsewhere, for example. - additions = [change for change in changes if change.kind == ChangeKind.ADDITION] - removals = [change for change in changes if change.kind == ChangeKind.REMOVAL] - if len(additions) > 0 and len(removals) == 0: - return make_check_result(additions) - - # There are either no changes to noexcept or a mix of added and removed noexcepts. We'd need a - # deeper analysis to figure out the mixed case. - return make_check_result([]) + pre, post = analyze_git_diff(diffs) + alerts = analyze_changes(pre, post) + return make_check_result(alerts) def main( @@ -348,9 +563,6 @@ def main( output_path: Annotated[ str, typer.Option("--output", "-o", help="Path at which to output github annotations") ] = "../github_annotations.json", - max_annotations: Annotated[ - int, typer.Option("--limit", help="Maximum number of annotations to output") - ] = 1, ): """ Check for changes in noexcept usage between the given revisions and output a file that @@ -373,12 +585,6 @@ def main( check_result = check_for_noexcept_changes(diffs) - # Cap annotations to the given limit - we only want to output one in the commit queue, but - # for testing it's useful to see all of them. - if "annotations" in check_result: - annotations = check_result.get("annotations", []) - if max_annotations > 0 and len(annotations) > max_annotations: - check_result["annotations"] = annotations[:max_annotations] with open(output_path, "w") as f: json.dump(check_result, f, indent=4) diff --git a/buildscripts/tests/test_check_for_noexcept.py b/buildscripts/tests/test_check_for_noexcept.py index 6c2adcde637..c2c0e83e621 100644 --- a/buildscripts/tests/test_check_for_noexcept.py +++ b/buildscripts/tests/test_check_for_noexcept.py @@ -1,47 +1,49 @@ import unittest from buildscripts.check_for_noexcept import ( - ChangeKind, + AlertKind, + FileBoundSnippet, + SnippetKind, + analyze_changes, analyze_text_diff, get_matching_blocks, + get_move_operations, + get_noexcepts, get_nonmatching_blocks, + strip_noexcept, ) -def _remove_whitespace(s: str) -> str: - return "".join(s.split()) - - class TestGetMatchingBlocks(unittest.TestCase): def _check_blocks_cover_string(self, blocks: list[tuple[int, int]], s: str): + lines = s.splitlines() if len(blocks) == 0: - self.assertEqual(len(s), 0) + self.assertEqual(len(lines), 0) return prev_end = 0 for block_start, block_end in sorted(blocks): self.assertEqual(block_start, prev_end) prev_end = block_end - self.assertEqual(prev_end, len(s)) + self.assertEqual(prev_end, len(lines)) def test_matches_and_nonmatches_are_accurate(self): cases = ( - ("const noexcept try {", "const try {"), - ("a b c d", "a b c d e"), - ("b c d e", "a b c d e"), - ("int f(SomeType x) noexcept const {", "int f(SomeType x) const {"), - ("int f(SomeType x) noexcept const\n{", "int\nf(SomeType x) const {"), + ("abc\nbcd\ncde\ndef", "abc\nwxy\ncde\ndef"), + ("abc\nbcd\ncde\ndef", "abc\ncde\ndef"), + ("abc\nbcd\ncde\ndef", "abc\nbcd\ncde\ndef\nxyz"), ) for lhs, rhs in cases: matching = get_matching_blocks(lhs, rhs) nonmatching = get_nonmatching_blocks(lhs, rhs) self.assertGreater(len(matching), 0) + self.assertGreater(len(nonmatching), 0) + + lhs_lines, rhs_lines = lhs.splitlines(), rhs.splitlines() for lhs_start, lhs_end, rhs_start, rhs_end in matching: - block_lhs = lhs[lhs_start:lhs_end] - block_rhs = rhs[rhs_start:rhs_end] - # Note that the matching ignores whitespace, so "matching" sequences may - # differ in whitespace. - self.assertEqual(_remove_whitespace(block_lhs), _remove_whitespace(block_rhs)) + block_lhs = "\n".join(lhs_lines[lhs_start:lhs_end]) + block_rhs = "\n".join(rhs_lines[rhs_start:rhs_end]) + self.assertEqual(block_lhs, block_rhs) lhs_blocks = [] rhs_blocks = [] for lhs_start, lhs_end, rhs_start, rhs_end in matching + nonmatching: @@ -55,37 +57,226 @@ class TestGetMatchingBlocks(unittest.TestCase): self.assertEqual(get_matching_blocks("nonempty", ""), []) self.assertEqual(get_matching_blocks("", ""), []) + self.assertEqual(get_nonmatching_blocks("", "nonempty"), [(0, 0, 0, 1)]) + self.assertEqual(get_nonmatching_blocks("nonempty", ""), [(0, 1, 0, 0)]) + self.assertEqual(get_nonmatching_blocks("", ""), []) + def test_no_match(self): self.assertEqual(get_matching_blocks("a b c d", "e f g h"), []) + self.assertEqual(get_nonmatching_blocks("a b c d", "e f g h"), [(0, 1, 0, 1)]) def test_equal_strings(self): s = "a b c d" - self.assertEqual(get_matching_blocks(s, s), [(0, len(s), 0, len(s))]) + self.assertEqual(get_matching_blocks(s, s), [(0, 1, 0, 1)]) + self.assertEqual(get_nonmatching_blocks(s, s), []) + + def test_basic_multiline_edit(self): + lhs = "a\nb\nc\nd" + rhs = "a\nw\nx\nd" + self.assertEqual(get_matching_blocks(lhs, rhs), [(0, 1, 0, 1), (3, 4, 3, 4)]) + self.assertEqual(get_nonmatching_blocks(lhs, rhs), [(1, 3, 1, 3)]) + + def test_basic_multiline_delete(self): + lhs = "a\nb\nc\nd" + rhs = "a\nd" + self.assertEqual(get_matching_blocks(lhs, rhs), [(0, 1, 0, 1), (3, 4, 1, 2)]) + self.assertEqual(get_nonmatching_blocks(lhs, rhs), [(1, 3, 1, 1)]) + + def test_basic_multiline_insert(self): + lhs = "a\nd" + rhs = "a\nb\nc\nd" + self.assertEqual(get_matching_blocks(lhs, rhs), [(0, 1, 0, 1), (1, 2, 3, 4)]) + self.assertEqual(get_nonmatching_blocks(lhs, rhs), [(1, 1, 1, 3)]) + + +# The basic cases below end with semicolons (generally). We can generate similar cases ending with { +# or just without the semicolon to cover those cases. +def _generate_additional_cases(cases: list[str]) -> tuple[str]: + new_cases = list(cases) + for case in cases: + if case.endswith(";"): + new_cases.append(case[:-1] + " {") + new_cases.append(case[:-1]) + return tuple(set(new_cases)) + + +# The hardcoded cases all contain noexcept. We can generate non-noexcept cases by removing any +# noexcept tokens. +def _generate_non_noexcept_cases(cases: list[str]) -> tuple[str]: + new_cases = [strip_noexcept(case) for case in cases] + return tuple(set(new_cases)) + + +_BASIC_NOEXCEPT_MOVE_CONSTRUCTORS = _generate_additional_cases( + [ + "TypeName(TypeName&& other) const noexcept;", + "TypeName(TypeName&& other) const noexcept(std::is_nothrow_constructible_v);", + "TypeName(TypeName&& other) noexcept;", + "TypeName(TypeName&& other) noexcept const;", + "TypeName(TypeName&& other) noexcept(std::is_nothrow_constructible_v) const;", + "Typenoexcept(Typenoexcept&& other) const noexcept;", + "Typenoexcept(Typenoexcept&& other) noexcept;", + "Typenoexcept(Typenoexcept&& other) noexcept const;", + ] +) + + +_BASIC_NOEXCEPT_MOVE_ASSIGNMENTS = _generate_additional_cases( + [ + "TypeName& operator=(TypeName&&) noexcept;", + "TypeName& operator=(TypeName&&) noexcept(std::is_nothrow_constructible_v);", + "TypeName& operator=(TypeName&&) noexcept { return *this; }", + "TypeName& operator=(TypeName&&) noexcept const;", + "TypeName& operator=(TypeName&&) noexcept const { return *this; }", + "TypeName& operator=(TypeName&&) noexcept(std::is_nothrow_constructible_v) const { return *this; }", + "TypeName& operator=(TypeName&&) const noexcept;", + "TypeName& operator=(TypeName&&) const noexcept { return *this; }", + "Typenoexcept& operator=(Typenoexcept&&) noexcept { return *this; }", + "Typenoexcept& operator=(Typenoexcept&&) noexcept(std::is_nothrow_constructible_v) { return *this; }", + ] +) + + +_BASIC_NON_NOEXCEPT_MOVE_CONSTRUCTORS = _generate_non_noexcept_cases( + _BASIC_NOEXCEPT_MOVE_CONSTRUCTORS +) + + +_BASIC_NON_NOEXCEPT_MOVE_ASSIGNMENTS = _generate_non_noexcept_cases( + _BASIC_NOEXCEPT_MOVE_ASSIGNMENTS +) + + +_BASIC_NOEXCEPT_MOVE_OPERATIONS = ( + _BASIC_NOEXCEPT_MOVE_CONSTRUCTORS + _BASIC_NOEXCEPT_MOVE_ASSIGNMENTS +) + + +_BASIC_NON_NOEXCEPT_MOVE_OPERATIONS = ( + _BASIC_NON_NOEXCEPT_MOVE_CONSTRUCTORS + _BASIC_NON_NOEXCEPT_MOVE_ASSIGNMENTS +) + + +# Tricky statements that look similar-ish to move operations but are not. +_BASIC_NOEXCEPT_NON_MOVE_OPERATIONS = _generate_additional_cases( + [ + "functionName(TypeName&&) noexcept;", + "functionName(TypeName&& other) noexcept;", + "functionName(TypeName&&) const noexcept;", + "functionName(TypeName&&, int) const noexcept;", + "functionName(TypeName&& other, int otherParam) noexcept;", + "functionName() noexcept;", + "TypeName() noexcept;", + "TypeName(const TypeName&) noexcept;", + "TypeName(const TypeName&) noexcept(std::is_trivially_constructible_v);", + "TypeName(TypeName) noexcept;", + "functionName(TypeName val) noexcept;", + "functionName(TypeName& val) noexcept;", + "functionName(TypeName&& val) noexcept;", + "auto lambda = [x = std::move(y)]() noexcept { return x; }", + "auto lambda = [](TypeName&& x) noexcept { return x; }", + "[](TypeName&& x) noexcept { return x; }", + "([](TypeName&& x) noexcept { return x; })()", + "TypeName(TypeName&&, int otherParam) noexcept;", + "TypeName operator+(const TypeName& other) noexcept;", + "TypeName operator+(const TypeName& other) const noexcept;", + "TypeName& operator*() const noexcept;", + ] +) + + +_BASIC_NON_NOEXCEPT_NON_MOVE_OPERATIONS = _generate_non_noexcept_cases( + _BASIC_NOEXCEPT_NON_MOVE_OPERATIONS +) + + +def _noexcept_moves_only(snippets): + return [s for s in snippets if s.kind == SnippetKind.NoexceptMove] + + +def _nonnoexcept_moves_only(snippets): + return [s for s in snippets if s.kind == SnippetKind.NonNoexceptMove] + + +def _make_snippet( + *, + file: str, + kind: SnippetKind, + identifier: str | None = None, + line: int | None = None, + line_start: int | None = None, + line_end: int | None = None, + alert_line: int | None = None, +): + if line is not None: + line_start = line + line_end = line + else: + line_start = line_start + line_end = line_end + + if alert_line is None: + alert_line = line_start + + return FileBoundSnippet( + file=file, + kind=kind, + start=0, + end=0, + identifier=identifier, + line_start=line_start, + line_end=line_end, + alert_line=alert_line, + ) class TestCheckForNoexcept(unittest.TestCase): - def test_no_text(self): - changes = analyze_text_diff("", "") - self.assertEqual(changes, []) + def test_strip_noexcept(self): + no_noexcept_cases = _BASIC_NON_NOEXCEPT_MOVE_OPERATIONS + for case in no_noexcept_cases: + self.assertEqual(strip_noexcept(case), case) - def test_simple_additions(self): + self.assertEqual( + strip_noexcept( + "int f(SomeType x) noexcept(std::is_nothrow_constructible_v>);" + ), + "int f(SomeType x);", + ) + self.assertEqual( + strip_noexcept("int f(SomeType x) noexcept const;"), + "int f(SomeType x) const;", + ) + self.assertEqual( + strip_noexcept("int f(SomeType x) noexcept;"), + "int f(SomeType x);", + ) + + def test_no_text(self): + pre, post = analyze_text_diff("", "") + self.assertEqual(pre, []) + self.assertEqual(post, []) + + def test_simple_noexcept_additions(self): pre = "int f(SomeType x) const {" posts = ( "int f(SomeType x) noexcept const {", "int f(SomeType x) noexcept {", + "int f(SomeType x) noexcept(std::is_nothrow_constructible_v>) {", "int func(SomeType x) noexcept const {", "int f(SomeType x) noexcept const {", "int f(SomeType y) noexcept const {", "int f(SomeOtherType x) noexcept const {", ) for post in posts: - changes = analyze_text_diff(pre, post) - self.assertEqual(len(changes), 1) - change = changes[0] - self.assertEqual(change.kind, ChangeKind.ADDITION) - self.assertTrue(change.is_certain) - self.assertEqual(change.index, post.find("noexcept")) - self.assertEqual(change.line, 1) + before, after = analyze_text_diff(pre, post) + self.assertEqual(len(before), 0) + self.assertEqual(len(after), 1) + snippet = after[0] + self.assertFalse(snippet.is_move()) + self.assertEqual(snippet.start, post.find("noexcept")) + self.assertEqual(snippet.end, snippet.start + len("noexcept")) + self.assertEqual(snippet.line_start, 1) + self.assertEqual(snippet.line_end, 1) def test_simple_removals(self): pre = "int f(SomeType x) noexcept const {" @@ -98,19 +289,61 @@ class TestCheckForNoexcept(unittest.TestCase): "int f(SomeOtherType x) const {", ) for post in posts: - changes = analyze_text_diff(pre, post) - self.assertEqual(len(changes), 1) - change = changes[0] - self.assertEqual(change.kind, ChangeKind.REMOVAL) - self.assertTrue(change.is_certain) - self.assertEqual(change.index, pre.find("noexcept")) - self.assertEqual(change.line, 1) + before, after = analyze_text_diff(pre, post) + self.assertEqual(len(before), 1) + self.assertEqual(len(after), 0) + snippet = before[0] + self.assertFalse(snippet.is_move()) + self.assertEqual(snippet.start, pre.find("noexcept")) + self.assertEqual(snippet.end, snippet.start + len("noexcept")) + self.assertEqual(snippet.line_start, 1) + self.assertEqual(snippet.line_end, 1) - def test_simple_unchanged(self): + def test_definite_noexcept_addition_removal(self): + with_noexcept = """ +int f() { + return x * y; +} +int g(int z) const noexcept(std::is_nothrow_constructible_v) { + return z + 1; +} +void q() noexcept { + // do nothing +} +""" + without_noexcept = """ +int f() { + return x * y; +} +int g(int z) const { + return z + 1; +} +void q() { + // do nothing +} +""" + # Case where noexcept was definitively removed. + before, after = analyze_text_diff(with_noexcept, without_noexcept) + self.assertEqual(len(before), 2) + self.assertEqual(len(after), 0) + for snippet in before: + self.assertEqual(snippet.kind, SnippetKind.NoexceptRemoval) + + # Case where noexcept was definitively added. + before, after = analyze_text_diff(without_noexcept, with_noexcept) + self.assertEqual(len(before), 0) + self.assertEqual(len(after), 2) + for snippet in before: + self.assertEqual(snippet.kind, SnippetKind.NoexceptRemoval) + + def test_simple_unchanged_with_noexcept(self): pre = "int f(SomeType x) noexcept {" - post = "int f(SomeType x) noexcept {" - changes = analyze_text_diff(pre, post) - self.assertEqual(changes, []) + post = "float f(SomeOtherType y) noexcept {" + before, after = analyze_text_diff(pre, post) + self.assertEqual(len(before), 1) + self.assertEqual(before[0].kind, SnippetKind.Noexcept) + self.assertEqual(len(after), 1) + self.assertEqual(after[0].kind, SnippetKind.Noexcept) def test_simple_unrelated_changes(self): pre = "int f(SomeType x) noexcept const {" @@ -122,70 +355,362 @@ class TestCheckForNoexcept(unittest.TestCase): "int f(SomeOtherType x) noexcept const {", ) for post in posts: - changes = analyze_text_diff(pre, post) - self.assertEqual(changes, []) + before, after = analyze_text_diff(pre, post) + self.assertEqual(len(before), 1) + self.assertEqual(before[0].kind, SnippetKind.Noexcept) + self.assertEqual(len(after), 1) + self.assertEqual(after[0].kind, SnippetKind.Noexcept) - def test_complex_change(self): - pre = """ -class SomeClass { - template - auto memberFunction(std::unique_ptr x) const { - thisFunctionCanThrow(); - return x.get(); - } - void otherMemberFunction() const noexcept try { - thisFunctionCanThrow() - } catch (DBException& e) { - } -}; -""" - post = """ -class SomeClass { - template - int memberFunctionNoThrow(std::unique_ptr x) const noexcept { - return x.get(); - } - void otherMemberFunction() const { - thisFunctionCanThrow() - } -}; -""" - changes = analyze_text_diff(pre, post) - additions = [change for change in changes if change.kind == ChangeKind.ADDITION] - removals = [change for change in changes if change.kind == ChangeKind.REMOVAL] - self.assertEqual(len(additions), 1) - self.assertEqual(len(removals), 1) - addition = additions[0] - removal = removals[0] - # Note that lines are all relative to the "post" side and the "SomeClass" is line 2 - # (lines are 1-indexed and line 1 is a blank line). - self.assertEqual(addition.line, 4) - self.assertEqual(removal.line, 7) + def test_simple_noexcept_move_addition(self): + pre = "\n\nTypeName(TypeName&& other);" + post = "\n\nTypeName(TypeName&& other)\nnoexcept;" + before, after = analyze_text_diff(pre, post) + self.assertEqual(len(before), 1) + snippet = before[0] + self.assertEqual(snippet.kind, SnippetKind.NonNoexceptMove) + self.assertEqual(snippet.start, 2) + self.assertEqual(snippet.end, pre.find(")") + 1) + self.assertEqual(snippet.line_start, 3) + self.assertEqual(snippet.line_end, 3) - def test_rewrite_change(self): - pre = """ -class SomeClass { - auto memberFunction(std::unique_ptr x) const { return x.get(); } - void otherMemberFunction() const { return 7; } - template - T anotherMemberFunction(T x) const { return x; } -}; -""" - post = """ -class SomeClass { - template - void anotherNewNoexceptFunction() noexcept { doSomething(); } - auto someNewNoexceptFunction(std::shared_ptr x) const noexcept { return *x; } - int anotherNewNoexceptFunction2() const noexcept { return 7; } -}; -""" - changes = analyze_text_diff(pre, post) - additions = [change for change in changes if change.kind == ChangeKind.ADDITION] - removals = [change for change in changes if change.kind == ChangeKind.REMOVAL] - self.assertEqual(len(additions), 3) - self.assertEqual(len(removals), 0) - lines = sorted([addition.line for addition in additions]) - self.assertEqual(lines, [4, 5, 6]) + self.assertEqual(len(after), 1) + snippet = after[0] + self.assertEqual(snippet.kind, SnippetKind.NoexceptMove) + self.assertEqual(snippet.start, 2) + self.assertEqual(snippet.end, len(post) - 1) + self.assertEqual(snippet.line_start, 3) + self.assertEqual(snippet.line_end, 4) + + def test_simple_noexcept_move_removal(self): + pre = "\n\nTypeName(TypeName&& other) noexcept {\n /* code */ }" + post = "\n\nTypeName(TypeName&& other) {\n /* code */ }" + before, after = analyze_text_diff(pre, post) + self.assertEqual(len(before), 1) + snippet = before[0] + self.assertEqual(snippet.kind, SnippetKind.NoexceptMove) + self.assertEqual(snippet.start, 2) + self.assertEqual(snippet.end, pre.find("{")) + self.assertEqual(snippet.line_start, 3) + self.assertEqual(snippet.line_end, 3) + + self.assertEqual(len(after), 1) + snippet = after[0] + self.assertEqual(snippet.kind, SnippetKind.NonNoexceptMove) + self.assertEqual(snippet.start, 2) + self.assertEqual(snippet.end, post.find(")") + 1) + self.assertEqual(snippet.line_start, 3) + self.assertEqual(snippet.line_end, 3) + + def test_get_move_operations_noexcept_independent(self): + # Basic cases + with_move_operation = _BASIC_NOEXCEPT_MOVE_OPERATIONS + _BASIC_NON_NOEXCEPT_MOVE_OPERATIONS + without_move_operation = ( + _BASIC_NOEXCEPT_NON_MOVE_OPERATIONS + _BASIC_NON_NOEXCEPT_NON_MOVE_OPERATIONS + ) + + # Trickier cases + with_move_operation += ( + """ + class TypeName { + TypeName(TypeName&&) noexcept; + }; + """, + """ + TypeName& operator=(TypeName&& other) noexcept { + auto lambda = [x = std::move(y)]() noexcept { return x; };", + z = lambda(); + return *this; + } + """, + "TypeName(TypeName&&) { /* noexcept */ }", + "TypeName(TypeName&&); // noexcept", + "Typenoexcept(Typenoexcept&& varnoexcept);", + "TypeName(TypeName&&); TypeName() noexcept;", + "Typenoexcept& operator=(Typenoexcept&&) { return *this; }", + """ + TypeName& operator=(TypeName&& other) { + auto lambda = [x = std::move(y)]() noexcept { return x; };", + z = lambda(); + return *this; + } + """, + ) + + without_move_operation += ( + """ + class TypeName { + TypeName(const TypeName&); + TypeName(TypeName); + functionName(TypeName&&); + }; + """, + ) + + for snippet in with_move_operation: + all_moves = get_move_operations(snippet) + self.assertEqual(len(all_moves), 1, msg=f"Expected move operation: {snippet}") + + for snippet in without_move_operation: + all_moves = get_move_operations(snippet) + self.assertEqual(len(all_moves), 0, msg=f"Expected no move operation in {snippet}") + + def test_get_move_operations_noexcept_matters(self): + # Basic cases + with_noexcept = _BASIC_NOEXCEPT_MOVE_OPERATIONS + without_noexcept = _BASIC_NON_NOEXCEPT_MOVE_OPERATIONS + + # Trickier cases + with_noexcept += ( + "Typenoexcept& operator=(Typenoexcept&&) noexcept { return *this; }", + """ + TypeName& operator=(TypeName&& other) noexcept { + auto lambda = [x = std::move(y)]() noexcept { return x; };", + z = lambda(); + return *this; + } + """, + ) + + without_noexcept += ( + "TypeName(TypeName&&) { /* noexcept */ }", + "TypeName(TypeName&&); // noexcept", + "Typenoexcept(Typenoexcept&& varnoexcept);", + "TypeName(TypeName&&); TypeName() noexcept;", + "Typenoexcept& operator=(Typenoexcept&&) { return *this; }", + """ + TypeName& operator=(TypeName&& other) { + auto lambda = [x = std::move(y)]() noexcept { return x; };", + z = lambda(); + return *this; + } + """, + ) + + for snippet in with_noexcept: + all_moves = get_move_operations(snippet) + noexcept = _noexcept_moves_only(all_moves) + non_noexcept = _nonnoexcept_moves_only(all_moves) + self.assertEqual(len(noexcept), 1, msg=f"Expected noexcept move operation: {snippet}") + self.assertEqual( + len(non_noexcept), 0, msg=f"Expected no non-noexcept move operation: {snippet}" + ) + + for snippet in without_noexcept: + all_moves = get_move_operations(snippet) + noexcept = _noexcept_moves_only(all_moves) + non_noexcept = _nonnoexcept_moves_only(all_moves) + self.assertEqual( + len(noexcept), 0, msg=f"Expected no noexcept move operation: {snippet}" + ) + self.assertEqual( + len(non_noexcept), 1, msg=f"Expected non-noexcept move operation: {snippet}" + ) + + def test_get_move_operations_negative_cases(self): + negative_cases = ( + _BASIC_NOEXCEPT_NON_MOVE_OPERATIONS + _BASIC_NON_NOEXCEPT_NON_MOVE_OPERATIONS + ) + + for snippet in negative_cases: + all_moves = get_move_operations(snippet) + self.assertEqual(len(all_moves), 0, msg=f"Expected no move operation: {snippet}") + + def test_get_noexcepts(self): + with_noexcept = _BASIC_NOEXCEPT_MOVE_OPERATIONS + _BASIC_NOEXCEPT_NON_MOVE_OPERATIONS + without_noexcept = ( + _BASIC_NON_NOEXCEPT_MOVE_OPERATIONS + _BASIC_NON_NOEXCEPT_NON_MOVE_OPERATIONS + ) + + for snippet in with_noexcept: + self.assertGreater( + len(get_noexcepts(snippet)), 0, msg=f"Expected noexcept in snippet: {snippet}" + ) + + for snippet in without_noexcept: + self.assertEqual( + len(get_noexcepts(snippet)), 0, msg=f"Expected no noexcept in snippet: {snippet}" + ) + + def test_move_identifiers(self): + templates = ( + "{t}({t}&&){c}{n};", + "{t}({t}&& {v}){c}{n};", + "operator=({t}&&){c}{n};", + "operator=({t}&& {v}){c}{n};", + "{t}& operator=({t}&&){c}{n};", + "{t}& operator=({t}&& {v}){c}{n};", + "{t}({t}&&)\n{c}\n{n};", + "operator=({t}&&)\n{c}{n};", + ) + + for template in templates: + for const in (" const", ""): + for noexcept in (" noexcept", "noexcept(condition)", ""): + for varname in ("other", "varnoexcept", ""): + for typename in ("TypeName", "Typenoexcept"): + case = template.format(t=typename, v=varname, c=const, n=noexcept) + all_moves = get_move_operations(case) + self.assertEqual( + len(all_moves), 1, msg=f"Expected move operation: {case}" + ) + move = all_moves[0] + if "operator=" in template: + identifier = typename + "::operator=" + else: + identifier = typename + "::" + typename + self.assertEqual(move.identifier, identifier) + + def test_analyze_changes_simple_addition_removal(self): + # No alert on noexcept deletion. + snippets = [ + _make_snippet(file="file1.cpp", kind=SnippetKind.Noexcept, line_start=10, line_end=10), + ] + alerts = analyze_changes(snippets, []) + self.assertEqual(len(alerts), 0) + + # Alert on noexcept addition. + alerts = analyze_changes([], snippets) + self.assertEqual(len(alerts), 1) + alert = alerts[0] + self.assertEqual(alert.file, "file1.cpp") + self.assertEqual(alert.kind, AlertKind.Noexcept) + self.assertEqual(alert.line, 10) + + def test_analyze_changes_non_noexcept_move(self): + other_snippets = [ + _make_snippet( + file="file1.cpp", kind=SnippetKind.NoexceptMove, identifier="irrelevant", line=5 + ), + _make_snippet(file="file2.cpp", kind=SnippetKind.Noexcept, line=10), + ] + + non_noexcept_move_snippet = [ + _make_snippet( + file="file3.cpp", + kind=SnippetKind.NonNoexceptMove, + identifier="TypeName::TypeName", + line=15, + ), + ] + + # The non-noexcept move is present on both sides, no alert. + alerts = analyze_changes(non_noexcept_move_snippet, non_noexcept_move_snippet) + self.assertEqual(len(alerts), 0) + alerts = analyze_changes( + other_snippets + non_noexcept_move_snippet, other_snippets + non_noexcept_move_snippet + ) + self.assertEqual(len(alerts), 0) + + # Other stuff is changing, but the non-noexcept move is not changing, no alert. + alerts = analyze_changes( + other_snippets + non_noexcept_move_snippet, non_noexcept_move_snippet + ) + self.assertFalse(any(alert.kind == AlertKind.NonNoexceptMove for alert in alerts)) + alerts = analyze_changes( + non_noexcept_move_snippet, other_snippets + non_noexcept_move_snippet + ) + self.assertFalse(any(alert.kind == AlertKind.NonNoexceptMove for alert in alerts)) + + # The non-noexcept move is removed, this is fine, no alert. + alerts = analyze_changes(other_snippets + non_noexcept_move_snippet, other_snippets) + self.assertEqual(len(alerts), 0) + + # The non-noexcept move is added, this is an alert. + alerts = analyze_changes(other_snippets, other_snippets + non_noexcept_move_snippet) + self.assertEqual(len(alerts), 1) + alert = alerts[0] + self.assertEqual(alert.file, "file3.cpp") + self.assertEqual(alert.line, 15) + self.assertEqual(alert.identifier, "TypeName::TypeName") + self.assertEqual(alert.kind, AlertKind.NonNoexceptMove) + + def test_analyze_changes_complex_case(self): + # We have a non-noexcept move (TypeName) in both pre and post, so no alert for that. + # We have a net addition of noexcept, so alert for that. + # We have a new non-noexcept move (OtherType), so alert for that. + pre_snippets = [ + _make_snippet(file="file1.cpp", kind=SnippetKind.Noexcept, line=10), + _make_snippet( + file="file2.cpp", + kind=SnippetKind.NonNoexceptMove, + identifier="TypeName::TypeName", + line=20, + ), + ] + post_snippets = [ + _make_snippet(file="file1.cpp", kind=SnippetKind.Noexcept, line=10), + _make_snippet(file="file1.cpp", kind=SnippetKind.Noexcept, line=30), + _make_snippet( + file="file2.cpp", + kind=SnippetKind.NoexceptMove, + identifier="TypeName::TypeName", + line=20, + ), + _make_snippet( + file="file3.cpp", + kind=SnippetKind.NonNoexceptMove, + identifier="OtherType::OtherType", + line=30, + ), + _make_snippet( + file="file4.cpp", + kind=SnippetKind.NonNoexceptMove, + identifier="TypeName::TypeName", + line=40, + ), + ] + + alerts = analyze_changes(pre_snippets, post_snippets) + self.assertEqual(len(alerts), 2) + + noexcept_move_alerts = [ + alert for alert in alerts if alert.kind == AlertKind.NonNoexceptMove + ] + self.assertEqual(len(noexcept_move_alerts), 1) + noexcept_move_alert = noexcept_move_alerts[0] + self.assertEqual(noexcept_move_alert.file, "file3.cpp") + self.assertEqual(noexcept_move_alert.line, 30) + self.assertEqual(noexcept_move_alert.identifier, "OtherType::OtherType") + + noexcept_change_alerts = [alert for alert in alerts if alert.kind == AlertKind.Noexcept] + self.assertEqual(len(noexcept_change_alerts), 1) + noexcept_change_alert = noexcept_change_alerts[0] + self.assertEqual(noexcept_change_alert.file, "file1.cpp") + + def test_analyze_changes_definitive_additions(self): + post_snippets = [ + _make_snippet(file="file1.cpp", kind=SnippetKind.Noexcept, line=10), + _make_snippet(file="file1.cpp", kind=SnippetKind.NoexceptAddition, line=20), + _make_snippet(file="file1.cpp", kind=SnippetKind.NoexceptAddition, line=30), + _make_snippet(file="file1.cpp", kind=SnippetKind.Noexcept, line=40), + ] + alerts = analyze_changes([], post_snippets) + # Only alert once for a definitive addition, even if there are multiple snippets. + # Also don't alert for the net additions of noexcept since it would be repetitive. + self.assertEqual(len(alerts), 1) + alert = alerts[0] + self.assertEqual(alert.file, "file1.cpp") + self.assertEqual(alert.kind, AlertKind.Noexcept) + self.assertEqual(alert.line, 20) + + def test_analyze_changes_definitive_removals(self): + pre_snippets = [ + _make_snippet(file="file1.cpp", kind=SnippetKind.Noexcept, line=10, alert_line=510), + _make_snippet( + file="file1.cpp", kind=SnippetKind.NoexceptAddition, line=20, alert_line=520 + ), + _make_snippet( + file="file1.cpp", kind=SnippetKind.NoexceptAddition, line=30, alert_line=530 + ), + _make_snippet(file="file1.cpp", kind=SnippetKind.Noexcept, line=40, alert_line=540), + ] + alerts = analyze_changes(pre_snippets, []) + # Only alert once for a definitive removal, even if there are multiple snippets. + self.assertEqual(len(alerts), 1) + alert = alerts[0] + self.assertEqual(alert.file, "file1.cpp") + self.assertEqual(alert.kind, AlertKind.Noexcept) + self.assertEqual(alert.line, 520) if __name__ == "__main__":