Add arm-no-replace-inherited-props linting rule#4384
Conversation
Agent-Logs-Url: https://github.com/Azure/typespec-azure/sessions/65193dd6-7c37-41ac-849e-a574be60d7c0 Co-authored-by: markcowl <1054056+markcowl@users.noreply.github.com>
markcowl
left a comment
There was a problem hiding this comment.
There is a merge conflict. Pull the latest from main, resolve the conflict, ensure all properties are correctly formatted and all tests pass and that only the chnages associated with this PR are in the PR diff
…place-inherited-props-linting-rule # Conflicts: # website/src/content/docs/docs/libraries/azure-resource-manager/reference/linter.md Co-authored-by: markcowl <1054056+markcowl@users.noreply.github.com>
Merged latest main and resolved the conflict in the auto-generated |
⚡ Benchmark Results
Full details – comparing
|
| Metric | Baseline | Current | Change |
|---|---|---|---|
| total | 🔴 710.3ms | 🔴 802.8ms | +13.0% 🔴 |
| loader | 🟢 136.0ms | 🟢 165.1ms | +21.3% 🔴 |
| resolver | 🟢 15.9ms | 🟢 16.6ms | +4.3% |
| checker | 🟢 166.7ms | 🟢 189.4ms | +13.6% 🔴 |
| validation | 🟢 39.6ms | 🟢 41.2ms | +4.0% |
| ↳ validation/@azure-tools/typespec-azure-core | 🟢 5.4ms | 🟢 6.4ms | +17.7% |
| ↳ validation/@typespec/http | 🟢 4.8ms | 🟢 5.4ms | +12.4% |
| ↳ validation/@typespec/rest | 🟢 0.5ms | 🟢 0.5ms | +8.3% |
| ↳ validation/@typespec/versioning | 🔴 27.0ms | 🔴 26.9ms | -0.3% |
| ↳ validation/compiler | 🟢 1.4ms | 🟢 1.4ms | -2.3% |
| linter | 🟢 115.9ms | 🟢 136.1ms | +17.5% 🔴 |
| ↳ linter/@azure-tools/typespec-azure-core/auth-required | 🟢 0.0ms | 🟢 0.0ms | +3.5% |
| ↳ linter/@azure-tools/typespec-azure-core/bad-record-type | 🟢 0.2ms | 🟢 0.2ms | -2.0% |
| ↳ linter/@azure-tools/typespec-azure-core/byos | 🟢 5.2ms | 🟢 6.0ms | +15.4% |
| ↳ linter/@azure-tools/typespec-azure-core/casing-style | 🟢 0.5ms | 🟢 0.6ms | +4.8% |
| ↳ linter/@azure-tools/typespec-azure-core/composition-over-inheritance | 🟢 0.1ms | 🟢 0.1ms | -4.5% |
| ↳ linter/@azure-tools/typespec-azure-core/documentation-required | 🟢 0.7ms | 🟢 0.7ms | -4.2% |
| ↳ linter/@azure-tools/typespec-azure-core/friendly-name | 🟢 0.5ms | 🟢 0.6ms | +4.1% |
| ↳ linter/@azure-tools/typespec-azure-core/key-visibility-required | 🟢 0.1ms | 🟢 0.1ms | +1.5% |
| ↳ linter/@azure-tools/typespec-azure-core/known-encoding | 🟢 0.2ms | 🟢 0.2ms | -9.1% |
| ↳ linter/@azure-tools/typespec-azure-core/long-running-polling-operation-required | 🟢 0.3ms | 🟢 0.3ms | +11.2% |
| ↳ linter/@azure-tools/typespec-azure-core/no-case-mismatch | 🟢 0.2ms | 🟢 0.2ms | +1.9% |
| ↳ linter/@azure-tools/typespec-azure-core/no-closed-literal-union | 🟢 0.2ms | 🟢 0.2ms | +4.9% |
| ↳ linter/@azure-tools/typespec-azure-core/no-enum | 🟢 0.0ms | 🟢 0.0ms | +8.2% |
| ↳ linter/@azure-tools/typespec-azure-core/no-error-status-codes | 🟢 0.1ms | 🟢 0.1ms | +14.3% |
| ↳ linter/@azure-tools/typespec-azure-core/no-explicit-routes-resource-ops | 🟢 0.1ms | 🟢 0.1ms | +6.6% |
| ↳ linter/@azure-tools/typespec-azure-core/no-format | 🟢 0.4ms | 🟢 0.4ms | +0.1% |
| ↳ linter/@azure-tools/typespec-azure-core/no-generic-numeric | 🟢 0.4ms | 🟢 0.3ms | -7.5% |
| ↳ linter/@azure-tools/typespec-azure-core/no-header-explode | 🟡 16.1ms | 🔴 20.1ms | +24.6% 🔴 |
| ↳ linter/@azure-tools/typespec-azure-core/no-legacy-usage | 🟢 1.0ms | 🟢 1.0ms | +5.1% |
| ↳ linter/@azure-tools/typespec-azure-core/no-multiple-discriminator | 🟢 0.1ms | 🟢 0.1ms | -5.9% |
| ↳ linter/@azure-tools/typespec-azure-core/no-nullable | 🟢 0.2ms | 🟢 0.2ms | +1.9% |
| ↳ linter/@azure-tools/typespec-azure-core/no-offsetdatetime | 🟢 1.2ms | 🟢 1.2ms | +5.3% |
| ↳ linter/@azure-tools/typespec-azure-core/no-openapi | 🟢 1.7ms | 🟢 1.7ms | +1.8% |
| ↳ linter/@azure-tools/typespec-azure-core/no-private-usage | 🟢 1.7ms | 🟢 1.7ms | +3.2% |
| ↳ linter/@azure-tools/typespec-azure-core/no-query-explode | 🟡 16.7ms | 🔴 20.4ms | +21.9% 🔴 |
| ↳ linter/@azure-tools/typespec-azure-core/no-response-body | 🔴 20.7ms | 🔴 24.7ms | +19.0% 🔴 |
| ↳ linter/@azure-tools/typespec-azure-core/no-rest-library-interfaces | 🟢 0.0ms | 🟢 0.0ms | +4.4% |
| ↳ linter/@azure-tools/typespec-azure-core/no-route-parameter-name-mismatch | 🟢 4.7ms | 🟢 5.7ms | +21.3% |
| ↳ linter/@azure-tools/typespec-azure-core/no-rpc-path-params | 🟢 0.2ms | 🟢 0.2ms | +11.1% |
| ↳ linter/@azure-tools/typespec-azure-core/no-string-discriminator | 🟢 0.0ms | 🟢 0.0ms | -3.2% |
| ↳ linter/@azure-tools/typespec-azure-core/no-unknown | 🟢 0.1ms | 🟢 0.2ms | +12.0% |
| ↳ linter/@azure-tools/typespec-azure-core/no-unnamed-union | 🟢 0.3ms | 🟢 0.3ms | +1.5% |
| ↳ linter/@azure-tools/typespec-azure-core/operation-missing-api-version | 🟢 0.2ms | 🟢 0.1ms | -8.6% |
| ↳ linter/@azure-tools/typespec-azure-core/request-body-problem | 🟢 0.2ms | 🟢 0.2ms | -4.6% |
| ↳ linter/@azure-tools/typespec-azure-core/require-versioned | 🟢 0.0ms | 🟢 0.0ms | -4.5% |
| ↳ linter/@azure-tools/typespec-azure-core/response-schema-problem | 🟡 19.4ms | 🔴 22.5ms | +16.1% 🔴 |
| ↳ linter/@azure-tools/typespec-azure-core/rpc-operation-request-body | 🟢 0.3ms | 🟢 0.3ms | +16.3% |
| ↳ linter/@azure-tools/typespec-azure-core/spread-discriminated-model | 🟢 0.2ms | 🟢 0.2ms | +0.5% |
| ↳ linter/@azure-tools/typespec-azure-core/use-standard-names | 🟢 4.3ms | 🟢 5.3ms | +21.9% |
| ↳ linter/@azure-tools/typespec-azure-core/use-standard-operations | 🟢 0.1ms | 🟢 0.1ms | +17.4% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-common-types-version | 🟢 3.5ms | 🟢 4.3ms | +23.5% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-custom-resource-no-key | 🟢 0.1ms | 🟢 0.1ms | +21.0% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-custom-resource-usage-discourage | 🟢 0.1ms | 🟢 0.1ms | +6.6% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-delete-operation-response-codes | 🟢 4.6ms | 🟢 5.6ms | +21.8% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-no-path-casing-conflicts | 🟢 4.0ms | 🟢 4.9ms | +24.4% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-no-record | 🟢 0.3ms | 🟢 0.3ms | +1.8% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-no-replace-inherited-props | 🟢 0.0ms | 🟢 0.1ms | +100.0% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-post-operation-response-codes | 🟢 0.4ms | 🟢 0.5ms | +35.7% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-put-operation-response-codes | 🟢 0.0ms | 🟢 0.0ms | +6.5% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-action-no-segment | 🟢 0.2ms | 🟢 0.2ms | +10.4% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-duplicate-property | 🟢 0.1ms | 🟢 0.1ms | +16.0% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-interface-requires-decorator | 🟢 0.0ms | 🟢 0.0ms | +9.6% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-invalid-action-verb | 🟢 0.1ms | 🟢 0.1ms | +2.8% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-invalid-envelope-property | 🟢 0.1ms | 🟢 0.1ms | +10.6% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-invalid-version-format | 🟢 0.0ms | 🟢 0.0ms | +8.9% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-key-invalid-chars | 🟢 0.2ms | 🟢 0.2ms | +21.1% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-name-pattern | 🟢 0.0ms | 🟢 0.0ms | -5.8% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-operation | 🟢 0.1ms | 🟢 0.2ms | +3.9% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-operation-response | 🟢 4.7ms | 🟢 5.5ms | +16.5% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-patch | 🟢 0.3ms | 🟢 0.3ms | +14.1% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-path-segment-invalid-chars | 🟢 0.2ms | 🟢 0.2ms | +12.4% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/arm-resource-provisioning-state | 🟢 0.1ms | 🟢 0.1ms | +13.3% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/beyond-nesting-levels | 🟢 0.1ms | 🟢 0.1ms | +12.4% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/empty-updateable-properties | 🟢 0.1ms | 🟢 0.1ms | +13.5% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/improper-subscription-list-operation | 🟢 0.0ms | 🟢 0.0ms | +4.9% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/lro-location-header | 🟡 12.0ms | 🟡 15.4ms | +28.7% 🔴 |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/missing-operations-endpoint | 🟢 0.0ms | 🟢 0.0ms | +10.4% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/missing-x-ms-identifiers | 🟢 0.2ms | 🟢 0.3ms | +12.7% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/no-empty-model | 🟢 0.1ms | 🟢 0.1ms | +12.3% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/no-resource-delete-operation | 🟢 0.1ms | 🟢 0.2ms | +18.4% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/no-response-body | 🟡 18.5ms | 🔴 22.2ms | +20.2% 🔴 |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/patch-envelope | 🟢 0.1ms | 🟢 0.1ms | +13.8% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/resource-name | 🟢 0.1ms | 🟢 0.1ms | +7.5% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/secret-prop | 🟢 2.0ms | 🟢 2.1ms | +5.9% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/unsupported-type | 🟢 0.3ms | 🟢 0.4ms | +10.9% |
| ↳ linter/@azure-tools/typespec-azure-resource-manager/version-progression | 🟢 0.0ms | 🟢 0.0ms | -3.8% |
| ↳ linter/@azure-tools/typespec-client-generator-core/property-name-conflict | 🟢 0.9ms | 🟢 1.0ms | +7.5% |
| ↳ linter/@azure-tools/typespec-client-generator-core/require-client-suffix | 🟢 0.2ms | 🟢 0.2ms | -3.4% |
| emit | 🟡 230.8ms | 🟡 249.1ms | +7.9% 🔴 |
| ↳ emit/@azure-tools/typespec-autorest | 🟢 143.5ms | 🟢 156.0ms | +8.7% 🔴 |
| ↳ emit/@typespec/openapi3 | 🟢 129.6ms | 🟢 138.3ms | +6.7% 🔴 |
| ↳ emit/@typespec/openapi3/compute | 🟢 114.3ms | 🟢 124.6ms | +9.0% 🔴 |
| ↳ emit/@typespec/openapi3/write | 🟢 14.4ms | 🟢 13.7ms | -4.8% |
Averaged across 3 specs (azure-arm-resource-manager, azure-core-dataplane, azure-full).
Threshold: changes > ±5% are highlighted.
🟢 Fast · 🟡 Moderate (stages >200ms, rules >10ms) · 🔴 Slow (stages >400ms, rules >20ms)
commit: |
markcowl
left a comment
There was a problem hiding this comment.
Update the changeset to include your change to the rulesets.
Pull the latest from main, ensure all files are well formatted and only the changes associated with this PR are in the PR diff.
…place-inherited-props-linting-rule Co-authored-by: markcowl <1054056+markcowl@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Azure/typespec-azure/sessions/a463ee62-17e5-45fa-8acd-eb3416b60ccd Co-authored-by: markcowl <1054056+markcowl@users.noreply.github.com>
Updated the changeset to include |
…place-inherited-props-linting-rule # Conflicts: # packages/typespec-azure-resource-manager/README.md # packages/typespec-azure-resource-manager/src/linter.ts # packages/typespec-azure-rulesets/src/rulesets/resource-manager.ts # website/src/content/docs/docs/libraries/azure-resource-manager/reference/linter.md Co-authored-by: markcowl <1054056+markcowl@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Azure/typespec-azure/sessions/081cde0d-41a3-4e99-9080-b24bf0ef7de7 Co-authored-by: markcowl <1054056+markcowl@users.noreply.github.com>
| } | ||
| case "EnumMember": | ||
| return typeof type.value === "number" ? "numeric" : "string"; | ||
| case "Union": { |
There was a problem hiding this comment.
You can use this helper to determine if a union is an open or closed enumeration over a string or numeric type: https://github.com/Azure/typespec-azure/blob/main/packages/typespec-azure-core/src/helpers/union-enums.ts#L83
There was a problem hiding this comment.
Refactored in 826590c to use getUnionAsEnum from @azure-tools/typespec-azure-core for the Union case — it returns either a string- or number-kind enum (open or closed), which collapses directly into the scalar-family check.
|
/azp run typespec-azure - PR Tools |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
All changed packages have been documented.
Show changes
|
|
You can try these changes here
|
markcowl
left a comment
There was a problem hiding this comment.
Pull the latest from main, make these test changes, make any implementation changes required to make these pass, ensure that all files are correctly formatted and all tests pass. Ensure that only the changes associated with this PR are in the PR diff.
| import { Tester } from "#test/tester.js"; | ||
| import { | ||
| createLinterRuleTester, | ||
| LinterRuleTester, |
There was a problem hiding this comment.
We should add tests that allow overriding a scalar type, string union, or enum type inherited from an ancestor with a specific value of the scalar type, a specific variant of the union, or a specific element of the enum.
For example:
// string
model Parent {
prop: string;
}
model Child extends Parent {
prop: "foo";
}
// int32
model Parent {
prop: int32;
}
model Child extends Parent {
prop: 12;
}
// anonymous closed string-typed union
model Parent {
prop: "foo" | "bar";
}
model Child extends Parent {
prop: "foo";
}
// open union with named variants
union PropType {
Foo: "foo",
Bar: "bar",
string
}
model Parent {
prop: PropType;
}
model Child extends Parent {
prop: PropType.Foo;
}
There was a problem hiding this comment.
Added all four test cases in 826590c (string → "foo", int32 → 12, "foo" | "bar" → "foo", and named open union PropType → PropType.Foo). The implementation needed one small addition — handling the UnionVariant kind in getScalarFamily so that PropType.Foo (a named variant reference) reduces to its underlying string type. All 14 rule tests and 170 ARM rule tests pass.
…place-inherited-props-linting-rule Co-authored-by: markcowl <1054056+markcowl@users.noreply.github.com>
…errides Agent-Logs-Url: https://github.com/Azure/typespec-azure/sessions/e17694c7-bbb6-4162-961f-041bb6fcf475 Co-authored-by: markcowl <1054056+markcowl@users.noreply.github.com>
Done in 826590c — pulled latest from main, added the 4 new test cases, made the small implementation change required (UnionVariant handling in the scalar-family reducer), and confirmed |
Redefining a property already declared on a base model is an Azure API anti-pattern and currently crashes the breaking-change tool (see azure-rest-api-specs#42903). This PR adds a new ARM linter rule to flag the pattern at design time.
Changes
arm-no-replace-inherited-props(packages/typespec-azure-resource-manager/src/rules/arm-no-replace-inherited-props.ts)baseModelchain and emits a warning for any property in the current model whose name also appears in any ancestor.string,numeric, orboolean). The scalar family is computed by walkingbaseScalarchains, classifying enum members by value type, reducing unions via thegetUnionAsEnumhelper from@azure-tools/typespec-azure-core, reducing named union variants to their underlying type, and treating string/numeric/boolean literals as their underlying scalar. This naturally covers:stringoverridden bystring, by a scalar that extendsstring, by a string literal, by a string-valued enum, or by an open or closed string union.string→"foo",int32→12,"foo" | "bar"→"foo", named open unionPropType→PropType.Foo).nameproperty (string ↔ string).@discriminatorproperties overridden with a string literal in derived models.nameor@discriminator; both fall out of the scalar-compatibility rule.The property '${propertyName}' is also defined in the base model. Redefining inherited properties can cause problems with OpenAPI tooling and some language representations of the models.src/linter.ts(included in theallruleset).packages/typespec-azure-rulesets/src/rulesets/resource-manager.ts).name, discriminator literal override, string→string literal value, int32→numeric literal value, anonymous closed string union→specific variant, and named open string union→named variant.linter.mdreference and packageREADME.md..chronus/changes/covering both@azure-tools/typespec-azure-resource-managerand@azure-tools/typespec-azure-rulesets.Example