Deduplicate changelog entries from generated types module (#48109)

* Deduplicate changelog entries from generated types module

The TypeSpec generated 'types' module contains TypedDict input aliases that shadow the real models in the sibling 'models' module. Newly added models were therefore reported twice in the changelog (e.g. 'Added model Foo'). Add a ShadowTypesModuleChecker post-processing step that drops changelog/breaking-change entries originating from a shadow 'types' module, and add tests.

* Guard shadow types cleanup by sibling models class existence

Address review feedback: only drop a types-module entry when the sibling models module actually contains a class of the same name, so a types-only change (no models counterpart) is preserved in the breaking-change/changelog output. Add regression tests for the class-existence guard.

* Remove unused sys.path.append from shadow types checker

* Match shadow types entries against actual models change tuple

Only drop a types-module entry when the sibling models module reports the same change type and arguments (with member names normalized snake_case to bridge the types wire-name vs models attribute-name difference). This avoids silently dropping an unmatched types-only member change while still deduping the real property/model duplicates. Add member-level regression tests.

* Normalize only member-name position in shadow types match

Compare class names and semantic values exactly and normalize only the member-name position to snake_case, avoiding false collisions on class names, type strings, or defaults. Add a regression test asserting exact class-name matching.

* Index shadow types match keys for O(n) lookup

Pre-index sibling models match keys once so each types entry is checked with a constant-time set lookup instead of rescanning the whole change list (avoids an O(n^2) regression on large diffs). Handle list-valued change args when building keys. Add a scalability test. Also aligns the PR description with the exact class-name matching behavior.

* update test

* update readme

Author: msyyc

scripts/breaking_changes_checker/README.md

@@ -200,3 +200,17 @@ C:\azure-sdk-for-python\sdk\storage\azure-storage-blob> azpysdk breaking . --cha
 
 > **Note:** The apistub-based report is *functionally equivalent* to the import-based report (same namespaces, classes, enums, methods/overloads, properties and module functions) but not byte-identical — enum values reflect member names and the inherited `str`/`Mapping` boilerplate is omitted. Since both sides of a comparison use the same converter, those differences cancel out in the diff.
 
+## Run tests
+
+The default test command runs the full suite, including slow integration tests that generate full code reports for large packages:
+
+```bash
+python -m pytest tests/
+```
+
+Skip those slow tests for a faster local signal:
+
+```bash
+python -m pytest tests/ --fast
+```
+

scripts/breaking_changes_checker/checkers/shadow_types_module_checker.py

@@ -0,0 +1,81 @@
+#!/usr/bin/env python
+
+# --------------------------------------------------------------------------------------------
+# Copyright (c) Microsoft Corporation. All rights reserved.
+# Licensed under the MIT License. See License.txt in the project root for license information.
+# --------------------------------------------------------------------------------------------
+
+# --------------------------------------------------------------------------------------------
+# This checker cleans up duplicate reporting caused by the generated `types` module.
+# TypeSpec generated libraries emit a `types` module containing `TypedDict` input aliases
+# that shadow the real models defined in the sibling `models` module. Because the same class
+# name exists in both modules, changes such as adding a new model are reported twice, e.g.
+# "Added model `Foo`" appears once for `...models` and once for `...types`.
+#
+# A `types` entry is only dropped when the sibling `models` module reports the *same* change
+# (same change type and arguments, with the module substituted). This ensures we never silently
+# drop a change that only exists on the `types` side (which would otherwise disappear from the
+# breaking-change / changelog output). Class names and semantic values (type strings, defaults)
+# are compared exactly; only the member-name position is normalized to snake_case, because the
+# `types` TypedDicts expose wire names (e.g. `serviceTreeId`) while the `models` classes expose
+# the Python attribute names (e.g. `service_tree_id`).
+# --------------------------------------------------------------------------------------------
+
+import re
+
+
+def _to_snake_case(name):
+    if not isinstance(name, str):
+        return name
+    name = re.sub(r"(.)([A-Z][a-z]+)", r"\1_\2", name)
+    name = re.sub(r"([a-z0-9])([A-Z])", r"\1_\2", name)
+    return name.lower()
+
+
+def _hashable(value):
+    # Change tuples may carry list arguments (e.g. parameter-ordering changes); convert them to
+    # tuples so the resulting match key can live in a set.
+    if isinstance(value, list):
+        return tuple(_hashable(item) for item in value)
+    return value
+
+
+class ShadowTypesModuleChecker:
+    def run_check(self, breaking_changes: list, features_added: list, *, diff: dict, stable_nodes: dict, current_nodes: dict, **kwargs) -> tuple[list, list]:
+        def _sibling_models_module(module_name):
+            if not isinstance(module_name, str):
+                return None
+            if module_name != "types" and not module_name.endswith(".types"):
+                return None
+            return module_name[: -len("types")] + "models"
+
+        def _match_key(change, module):
+            # Key is (change_type, module, class_name, [member_name], *rest). The class name
+            # (args[0]) and any trailing semantic values are compared exactly; only the member
+            # name (args[1]) is normalized so that `types` wire names match `models` attribute
+            # names.
+            args = change[3:]
+            normalized_args = tuple(
+                _hashable(_to_snake_case(arg) if index == 1 else arg) for index, arg in enumerate(args)
+            )
+            return (change[1], module) + normalized_args
+
+        def _filter(changes_list):
+            # Pre-index the match key of every change once so each `types` entry can be checked
+            # with a single O(1) set lookup instead of rescanning the whole list (avoids O(n^2)).
+            candidate_keys = {_match_key(change, change[2]) for change in changes_list if len(change) > 3}
+            result = []
+            for change in changes_list:
+                sibling_models = _sibling_models_module(change[2])
+                # Drop a `types` entry only when the sibling `models` module reports the same
+                # change. A module-level change (len <= 3) has no class to match, so keep it.
+                if (
+                    sibling_models is not None
+                    and len(change) > 3
+                    and _match_key(change, sibling_models) in candidate_keys
+                ):
+                    continue
+                result.append(change)
+            return result
+
+        return _filter(breaking_changes), _filter(features_added)

scripts/breaking_changes_checker/supported_checkers.py

@@ -8,6 +8,7 @@
 from checkers.removed_method_overloads_checker import RemovedMethodOverloadChecker
 from checkers.added_method_overloads_checker import AddedMethodOverloadChecker
 from checkers.changed_function_return_type_checker import ChangedFunctionReturnTypeChecker
+from checkers.shadow_types_module_checker import ShadowTypesModuleChecker
 
 CHECKERS = [
     RemovedMethodOverloadChecker(),
@@ -16,5 +17,5 @@
 ]
 
 POST_PROCESSING_CHECKERS = [
-    # Add any post-processing checkers here
+    ShadowTypesModuleChecker(),
 ]

scripts/breaking_changes_checker/tests/conftest.py

@@ -1,2 +1,19 @@
+import pytest
+
+
+def pytest_addoption(parser):
+    parser.addoption("--fast", action="store_true", default=False, help="skip slow integration tests")
+
+
 def pytest_configure(config):
-    config.addinivalue_line("markers", "slow: tests that may take up to 10 minutes")
+    config.addinivalue_line("markers", "slow: tests that may take several minutes")
+
+
+def pytest_collection_modifyitems(config, items):
+    if not config.getoption("--fast"):
+        return
+
+    skip_slow = pytest.mark.skip(reason="skipped by --fast")
+    for item in items:
+        if "slow" in item.keywords:
+            item.add_marker(skip_slow)

scripts/breaking_changes_checker/tests/test_changelog.py

@@ -18,6 +18,7 @@
         sys.path.insert(0, path)
 
 from breaking_changes_checker.checkers.added_method_overloads_checker import AddedMethodOverloadChecker
+from breaking_changes_checker.checkers.shadow_types_module_checker import ShadowTypesModuleChecker
 from breaking_changes_checker.changelog_tracker import ChangelogTracker, BreakingChangesTracker
 from breaking_changes_checker.detect_breaking_changes import main
 from breaking_changes_checker.detect_breaking_changes import test_compare_reports as compare_reports
@@ -151,6 +152,223 @@ def test_async_cleanup_check():
     assert msg == ChangelogTracker.ADDED_CLASS_PROPERTY_MSG
 
 
+def test_shadow_types_module_cleanup():
+    # A TypeSpec generated `types` module holds TypedDict input aliases that shadow the
+    # real models in the sibling `models` module. A newly added model therefore shows up in
+    # both modules and would be reported twice (e.g. "Added model `TrustedHostSubscription`").
+    # The ShadowTypesModuleChecker post-processing step should drop the `types` duplicate.
+    stable = {
+        "azure.mgmt.computelimit.models": {
+            "class_nodes": {
+                "ExistingModel": {"type": None, "methods": {}, "properties": {}},
+            }
+        },
+        "azure.mgmt.computelimit.types": {
+            "class_nodes": {
+                "ExistingModel": {"type": None, "methods": {}, "properties": {}},
+            }
+        },
+    }
+
+    current = {
+        "azure.mgmt.computelimit.models": {
+            "class_nodes": {
+                "ExistingModel": {"type": None, "methods": {}, "properties": {}},
+                "TrustedHostSubscription": {"type": None, "methods": {}, "properties": {}},
+            }
+        },
+        "azure.mgmt.computelimit.types": {
+            "class_nodes": {
+                "ExistingModel": {"type": None, "methods": {}, "properties": {}},
+                "TrustedHostSubscription": {"type": None, "methods": {}, "properties": {}},
+            }
+        },
+    }
+
+    bc = ChangelogTracker(
+        stable, current, "azure-mgmt-computelimit", post_processing_checkers=[ShadowTypesModuleChecker()]
+    )
+    bc.run_checks()
+
+    added_models = [fa for fa in bc.features_added if fa[0] == ChangelogTracker.ADDED_CLASS_MSG]
+    # Should only have 1 "Added model" reported instead of 2 (the `types` duplicate is removed).
+    assert len(added_models) == 1
+    _, _, module_name, class_name = added_models[0]
+    assert module_name == "azure.mgmt.computelimit.models"
+    assert class_name == "TrustedHostSubscription"
+    # No change originating from the shadow `types` module should remain.
+    assert not any(fa[2] == "azure.mgmt.computelimit.types" for fa in bc.features_added)
+
+
+def test_shadow_types_module_kept_without_models_sibling():
+    # If there is no sibling `models` module, a `types` module is a real public module and
+    # its changes should NOT be dropped.
+    stable = {
+        "azure.mgmt.computelimit.types": {
+            "class_nodes": {
+                "ExistingModel": {"type": None, "methods": {}, "properties": {}},
+            }
+        },
+    }
+
+    current = {
+        "azure.mgmt.computelimit.types": {
+            "class_nodes": {
+                "ExistingModel": {"type": None, "methods": {}, "properties": {}},
+                "TrustedHostSubscription": {"type": None, "methods": {}, "properties": {}},
+            }
+        },
+    }
+
+    bc = ChangelogTracker(
+        stable, current, "azure-mgmt-computelimit", post_processing_checkers=[ShadowTypesModuleChecker()]
+    )
+    bc.run_checks()
+
+    added_models = [fa for fa in bc.features_added if fa[0] == ChangelogTracker.ADDED_CLASS_MSG]
+    assert len(added_models) == 1
+    _, _, module_name, class_name = added_models[0]
+    assert module_name == "azure.mgmt.computelimit.types"
+    assert class_name == "TrustedHostSubscription"
+
+
+def test_shadow_types_module_kept_when_class_missing_in_models():
+    # If the sibling `models` module exists but does NOT contain the class, the `types`
+    # class has no `models` counterpart and its change must be preserved.
+    stable = {
+        "azure.mgmt.computelimit.models": {
+            "class_nodes": {
+                "ExistingModel": {"type": None, "methods": {}, "properties": {}},
+            }
+        },
+        "azure.mgmt.computelimit.types": {
+            "class_nodes": {
+                "ExistingModel": {"type": None, "methods": {}, "properties": {}},
+            }
+        },
+    }
+
+    current = {
+        "azure.mgmt.computelimit.models": {
+            "class_nodes": {
+                "ExistingModel": {"type": None, "methods": {}, "properties": {}},
+            }
+        },
+        "azure.mgmt.computelimit.types": {
+            "class_nodes": {
+                "ExistingModel": {"type": None, "methods": {}, "properties": {}},
+                # Only present in the `types` module, no counterpart in `models`.
+                "OrphanInput": {"type": None, "methods": {}, "properties": {}},
+            }
+        },
+    }
+
+    bc = ChangelogTracker(
+        stable, current, "azure-mgmt-computelimit", post_processing_checkers=[ShadowTypesModuleChecker()]
+    )
+    bc.run_checks()
+
+    added_models = [fa for fa in bc.features_added if fa[0] == ChangelogTracker.ADDED_CLASS_MSG]
+    assert len(added_models) == 1
+    _, _, module_name, class_name = added_models[0]
+    assert module_name == "azure.mgmt.computelimit.types"
+    assert class_name == "OrphanInput"
+
+
+def test_shadow_types_module_breaking_change_preserved_for_unmatched_member():
+    # A `types` change is dropped only when the sibling `models` module reports the *same*
+    # change. Here `OrphanInput` is removed only from `types` (no matching `models` entry), so it
+    # must be preserved, while the `RealModel` addition is mirrored in `models` and thus deduped.
+    checker = ShadowTypesModuleChecker()
+    breaking_changes = [
+        # `OrphanInput` only ever existed in `types`, so its removal is a real breaking change.
+        ("Deleted model `{}`", "RemovedOrRenamedClass", "azure.mgmt.computelimit.types", "OrphanInput"),
+    ]
+    features_added = [
+        # `RealModel` is added in both modules, so the `types` copy is a shadow duplicate.
+        ("Added model `{}`", "AddedClass", "azure.mgmt.computelimit.models", "RealModel"),
+        ("Added model `{}`", "AddedClass", "azure.mgmt.computelimit.types", "RealModel"),
+    ]
+
+    bc_out, fa_out = checker.run_check(
+        breaking_changes, features_added, diff={}, stable_nodes={}, current_nodes={}
+    )
+
+    assert bc_out == breaking_changes  # unmatched `types` breaking change preserved
+    assert len(fa_out) == 1  # shadow duplicate removed, `models` entry kept
+    assert fa_out[0][2] == "azure.mgmt.computelimit.models"
+
+
+def test_shadow_types_module_member_change_preserved_when_not_mirrored():
+    # An unmatched *member* change on a class that exists in both modules must be preserved:
+    # `RealModel` loses `orphanProp` only on the `types` side, `models` reports nothing.
+    checker = ShadowTypesModuleChecker()
+    breaking_changes = [
+        (
+            "Model `{}` removed property `{}`",
+            "RemovedOrRenamedInstanceAttribute",
+            "azure.mgmt.computelimit.types",
+            "RealModel",
+            "orphanProp",
+        ),
+    ]
+
+    bc_out, fa_out = checker.run_check(
+        breaking_changes, [], diff={}, stable_nodes={}, current_nodes={}
+    )
+
+    assert bc_out == breaking_changes  # no matching `models` change -> preserved
+    assert fa_out == []
+
+
+def test_shadow_types_module_member_change_deduped_across_naming():
+    # A member change reported in both modules is deduped even though `types` uses the wire name
+    # (`serviceTreeId`) and `models` uses the Python attribute name (`service_tree_id`).
+    checker = ShadowTypesModuleChecker()
+    breaking_changes = [
+        (
+            "Model `{}` removed property `{}`",
+            "RemovedOrRenamedInstanceAttribute",
+            "azure.mgmt.computelimit.models",
+            "FeatureEnableRequest",
+            "service_tree_id",
+        ),
+        (
+            "Model `{}` removed property `{}`",
+            "RemovedOrRenamedInstanceAttribute",
+            "azure.mgmt.computelimit.types",
+            "FeatureEnableRequest",
+            "serviceTreeId",
+        ),
+    ]
+
+    bc_out, _ = checker.run_check(
+        breaking_changes, [], diff={}, stable_nodes={}, current_nodes={}
+    )
+
+    assert len(bc_out) == 1  # `types` duplicate removed despite the camelCase/snake_case names
+    assert bc_out[0][2] == "azure.mgmt.computelimit.models"
+
+
+def test_shadow_types_module_class_name_matched_exactly():
+    # Class names are compared exactly (not snake_case normalized), so a `types` class whose
+    # name only collides with a differently-named `models` class after normalization is kept.
+    checker = ShadowTypesModuleChecker()
+    features_added = [
+        # Different class names that would collide if class names were snake_cased
+        # (`FooBar` -> `foo_bar`, `Foo_Bar` -> `foo_bar`).
+        ("Added model `{}`", "AddedClass", "azure.mgmt.computelimit.models", "Foo_Bar"),
+        ("Added model `{}`", "AddedClass", "azure.mgmt.computelimit.types", "FooBar"),
+    ]
+
+    _, fa_out = checker.run_check(
+        [], features_added, diff={}, stable_nodes={}, current_nodes={}
+    )
+
+    # No exact class-name match in `models`, so the `types` entry is preserved.
+    assert len(fa_out) == 2
+
+
 def test_new_class_property_added_init():
     # Testing if a property is added both in the init and at the class level that we only get 1 report for it
     current = {