Skip to content

Commit 41be466

Browse files
cormacpaynecormacpayne
authored
Merge pull request Azure#7273 from cormacpayne/type-check-refactor
Fix how output and generic types are checked in the breaking change tool
2 parents 5ae20b3 + da71279 commit 41be466

File tree

6 files changed

+63
-72
lines changed

6 files changed

+63
-72
lines changed

‎tools/StaticAnalysis/BreakingChangeAnalyzer/CmdletMetadataHelper.cs

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ public void CompareCmdletMetadata(
108108
// If the cmdlet cannot be found, log an issue
109109
else
110110
{
111-
issueLogger.LogBreakingChangeIssue(
111+
issueLogger?.LogBreakingChangeIssue(
112112
cmdlet: oldCmdlet,
113113
severity: 0,
114114
problemId: ProblemIds.BreakingChangeProblemId.RemovedCmdlet,
@@ -145,7 +145,7 @@ private void CheckForRemovedCmdletAlias(
145145
// If the alias cannot be found, log an issue
146146
if (!aliasSet.Contains(oldAlias))
147147
{
148-
issueLogger.LogBreakingChangeIssue(
148+
issueLogger?.LogBreakingChangeIssue(
149149
cmdlet: oldCmdlet,
150150
severity: 0,
151151
problemId: ProblemIds.BreakingChangeProblemId.RemovedCmdletAlias,
@@ -171,7 +171,7 @@ private void CheckForRemovedSupportsShouldProcess(
171171
// If the old cmdlet implements SupportsShouldProcess and the new cmdlet does not, log an issue
172172
if (oldCmdlet.SupportsShouldProcess && !newCmdlet.SupportsShouldProcess)
173173
{
174-
issueLogger.LogBreakingChangeIssue(
174+
issueLogger?.LogBreakingChangeIssue(
175175
cmdlet: oldCmdlet,
176176
severity: 0,
177177
problemId: ProblemIds.BreakingChangeProblemId.RemovedShouldProcess,
@@ -194,7 +194,7 @@ private void CheckForRemovedSupportsPaging(
194194
// If the old cmdlet implements SupportsPaging and the new cmdlet does not, log an issue
195195
if (oldCmdlet.SupportsPaging && !newCmdlet.SupportsPaging)
196196
{
197-
issueLogger.LogBreakingChangeIssue(
197+
issueLogger?.LogBreakingChangeIssue(
198198
cmdlet: oldCmdlet,
199199
severity: 0,
200200
problemId: ProblemIds.BreakingChangeProblemId.RemovedPaging,
@@ -239,17 +239,22 @@ private void CheckForChangedOutputType(
239239

240240
_typeMetadataHelper.CheckOutputType(oldCmdlet, oldOutput.Type, newOutputType, issueLogger);
241241
}
242-
// If the output cannot be found, log an issue
242+
// If the output cannot be found by name, check if the old output can be mapped
243+
// to any of the new output types
243244
else
244245
{
245-
issueLogger.LogBreakingChangeIssue(
246-
cmdlet: oldCmdlet,
247-
severity: 0,
248-
problemId: ProblemIds.BreakingChangeProblemId.ChangedOutputType,
249-
description: string.Format(Properties.Resources.ChangedOutputTypeDescription,
250-
oldCmdlet.Name, oldOutput.Type.Name),
251-
remediation: string.Format(Properties.Resources.ChangedOutputTypeRemediation,
252-
oldCmdlet.Name, oldOutput.Type.Name));
246+
var foundOutput = outputDictionary.Values.Any(o => _typeMetadataHelper.CompareTypeMetadata(oldCmdlet, oldOutput.Type, o, null));
247+
if (!foundOutput)
248+
{
249+
issueLogger?.LogBreakingChangeIssue(
250+
cmdlet: oldCmdlet,
251+
severity: 0,
252+
problemId: ProblemIds.BreakingChangeProblemId.ChangedOutputType,
253+
description: string.Format(Properties.Resources.ChangedOutputTypeDescription,
254+
oldCmdlet.Name, oldOutput.Type.Name),
255+
remediation: string.Format(Properties.Resources.ChangedOutputTypeRemediation,
256+
oldCmdlet.Name, oldOutput.Type.Name));
257+
}
253258
}
254259
}
255260
}
@@ -302,7 +307,7 @@ private void CheckDefaultParameterName(
302307
// If the parameter cannot be found, log an issue
303308
if (!parameterDictionary.ContainsKey(oldParameter.ParameterMetadata.Name))
304309
{
305-
issueLogger.LogBreakingChangeIssue(
310+
issueLogger?.LogBreakingChangeIssue(
306311
cmdlet: oldCmdlet,
307312
severity: 0,
308313
problemId: ProblemIds.BreakingChangeProblemId.ChangeDefaultParameter,

‎tools/StaticAnalysis/BreakingChangeAnalyzer/ParameterMetadataHelper.cs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ public void CompareParameterMetadata(
8989
// If the parameter cannot be found, log an issue
9090
else
9191
{
92-
issueLogger.LogBreakingChangeIssue(
92+
issueLogger?.LogBreakingChangeIssue(
9393
cmdlet: cmdlet,
9494
severity: 0,
9595
problemId: ProblemIds.BreakingChangeProblemId.RemovedParameter,
@@ -149,7 +149,7 @@ private void CheckForRemovedParameterAlias(
149149
// If the alias cannot be found, log an issue
150150
if (!aliasSet.Contains(oldAlias))
151151
{
152-
issueLogger.LogBreakingChangeIssue(
152+
issueLogger?.LogBreakingChangeIssue(
153153
cmdlet: cmdlet,
154154
severity: 0,
155155
problemId: ProblemIds.BreakingChangeProblemId.RemovedParameterAlias,
@@ -187,7 +187,7 @@ private void CheckParameterValidationSets(
187187
// one in the new assembly, log an issue
188188
if (oldValidateSet.Count == 0 && newValidateSet.Count > 0)
189189
{
190-
issueLogger.LogBreakingChangeIssue(
190+
issueLogger?.LogBreakingChangeIssue(
191191
cmdlet: cmdlet,
192192
severity: 0,
193193
problemId: ProblemIds.BreakingChangeProblemId.AddedValidateSet,
@@ -215,7 +215,7 @@ private void CheckParameterValidationSets(
215215
// If the value cannot be found, log an issue
216216
if (!valueSet.Contains(oldValue))
217217
{
218-
issueLogger.LogBreakingChangeIssue(
218+
issueLogger?.LogBreakingChangeIssue(
219219
cmdlet: cmdlet,
220220
severity: 0,
221221
problemId: ProblemIds.BreakingChangeProblemId.RemovedValidateSetValue,
@@ -246,7 +246,7 @@ private void CheckParameterValidateRange(
246246
// If the old parameter had no validation range, but the new parameter does, log an issue
247247
if (oldParameter.ValidateRangeMin == null && oldParameter.ValidateRangeMax == null)
248248
{
249-
issueLogger.LogBreakingChangeIssue(
249+
issueLogger?.LogBreakingChangeIssue(
250250
cmdlet: cmdlet,
251251
severity: 0,
252252
problemId: ProblemIds.BreakingChangeProblemId.AddedValidateRange ,
@@ -260,7 +260,7 @@ private void CheckParameterValidateRange(
260260
// If the minimum value of the range has increased, log an issue
261261
if (oldParameter.ValidateRangeMin < newParameter.ValidateRangeMin)
262262
{
263-
issueLogger.LogBreakingChangeIssue(
263+
issueLogger?.LogBreakingChangeIssue(
264264
cmdlet: cmdlet,
265265
severity: 0,
266266
problemId: ProblemIds.BreakingChangeProblemId.ChangedValidateRangeMinimum,
@@ -273,7 +273,7 @@ private void CheckParameterValidateRange(
273273
// If the maximum value of the range has decreased, log an issue
274274
if (oldParameter.ValidateRangeMax > newParameter.ValidateRangeMax)
275275
{
276-
issueLogger.LogBreakingChangeIssue(
276+
issueLogger?.LogBreakingChangeIssue(
277277
cmdlet: cmdlet,
278278
severity: 0,
279279
problemId: ProblemIds.BreakingChangeProblemId.ChangedValidateRangeMaximum,
@@ -303,7 +303,7 @@ private void CheckForValidateNotNullOrEmpty(
303303
// old assembly, but has it in the new assembly, log an issue
304304
if (!oldParameter.ValidateNotNullOrEmpty && newParameter.ValidateNotNullOrEmpty)
305305
{
306-
issueLogger.LogBreakingChangeIssue(
306+
issueLogger?.LogBreakingChangeIssue(
307307
cmdlet: cmdlet,
308308
severity: 0,
309309
problemId: ProblemIds.BreakingChangeProblemId.AddedValidateNotNullOrEmpty,

‎tools/StaticAnalysis/BreakingChangeAnalyzer/ParameterSetMetadataHelper.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ public void CompareParameterSetMetadata(
136136

137137
if (!foundMatch)
138138
{
139-
issueLogger.LogBreakingChangeIssue(
139+
issueLogger?.LogBreakingChangeIssue(
140140
cmdlet: cmdlet,
141141
severity: 0,
142142
problemId: ProblemIds.BreakingChangeProblemId.RemovedParameterSet,

‎tools/StaticAnalysis/BreakingChangeAnalyzer/TypeMetadataHelper.cs

Lines changed: 33 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -55,12 +55,14 @@ public TypeMetadataHelper(
5555
/// <param name="oldType">The type metadata from the old (serialized) assembly.</param>
5656
/// <param name="newType">The type metadata from the new assembly.</param>
5757
/// <param name="issueLogger">ReportLogger that will keep track of issues found.</param>
58-
public void CompareTypeMetadata(
58+
public bool CompareTypeMetadata(
5959
CmdletMetadata cmdlet,
6060
TypeMetadata oldType,
6161
TypeMetadata newType,
6262
ReportLogger<BreakingChangeIssue> issueLogger)
6363
{
64+
var result = true;
65+
6466
// For each property in the old assembly type, find the corresponding
6567
// property in the new assembly type
6668
foreach (var oldProperty in oldType.Properties.Keys)
@@ -109,29 +111,33 @@ public void CompareTypeMetadata(
109111
else
110112
{
111113
// If the type of the property has been changed, log an issue
112-
issueLogger.LogBreakingChangeIssue(
114+
issueLogger?.LogBreakingChangeIssue(
113115
cmdlet: cmdlet,
114116
severity: 0,
115117
problemId: ProblemIds.BreakingChangeProblemId.ChangedPropertyType,
116118
description: string.Format(Properties.Resources.ChangedPropertyTypeDescription,
117119
oldProperty, oldType.Name, oldPropertyType, newPropertyType),
118120
remediation: string.Format(Properties.Resources.ChangedPropertyTypeRemediation,
119121
oldProperty, oldPropertyType));
122+
result = false;
120123
}
121124
}
122125
else
123126
{
124127
// If the property has been removed, log an issue
125-
issueLogger.LogBreakingChangeIssue(
128+
issueLogger?.LogBreakingChangeIssue(
126129
cmdlet: cmdlet,
127130
severity: 0,
128131
problemId: ProblemIds.BreakingChangeProblemId.RemovedProperty,
129132
description: string.Format(Properties.Resources.RemovedPropertyDescription,
130133
oldProperty, oldType.Name),
131134
remediation: string.Format(Properties.Resources.RemovedPropertyRemediation,
132135
oldProperty, oldType.Name));
136+
result = false;
133137
}
134138
}
139+
140+
return result;
135141
}
136142

137143
public void CompareMethodSignatures(
@@ -179,7 +185,7 @@ public void CompareMethodSignatures(
179185

180186
if (!found)
181187
{
182-
issueLogger.LogBreakingChangeIssue(
188+
issueLogger?.LogBreakingChangeIssue(
183189
cmdlet: cmdlet,
184190
severity: 0,
185191
problemId: 0,
@@ -228,7 +234,7 @@ public void CheckParameterType(
228234
// If the types are different, log an issue
229235
if (!oldTypeMetadata.Name.Equals(newTypeMetadata.Name, StringComparison.OrdinalIgnoreCase))
230236
{
231-
issueLogger.LogBreakingChangeIssue(
237+
issueLogger?.LogBreakingChangeIssue(
232238
cmdlet: cmdlet,
233239
severity: 0,
234240
problemId: ProblemIds.BreakingChangeProblemId.ChangedParameterType,
@@ -320,7 +326,7 @@ private bool IsElementType(
320326
// If the element type has changed, log an issue
321327
else
322328
{
323-
issueLogger.LogBreakingChangeIssue(
329+
issueLogger?.LogBreakingChangeIssue(
324330
cmdlet: cmdlet,
325331
severity: 0,
326332
problemId: problemId,
@@ -366,20 +372,34 @@ private bool IsGenericType(
366372
var oldArgument = oldTypeMetadata.GenericTypeArguments[idx];
367373
var newArgument = newTypeMetadata.GenericTypeArguments[idx];
368374

369-
if (CheckGenericTypeArguments(cmdlet, oldArgument, newArgument, target, issueLogger))
375+
var oldElementType = _oldTypeDictionary[oldArgument];
376+
var newElementType = _newTypeDictionary[newArgument];
377+
if (string.Equals(oldArgument, newArgument, StringComparison.OrdinalIgnoreCase))
370378
{
371379
// If we have not previously seen this generic type argument,
372380
// run this method on the type
373381
if (!_typeSet.Contains(oldArgument))
374382
{
375383
_typeSet.Add(oldArgument);
376-
377-
var oldElementType = _oldTypeDictionary[oldArgument];
378-
var newElementType = _newTypeDictionary[newArgument];
379-
380384
CompareTypeMetadata(cmdlet, oldElementType, newElementType, issueLogger);
381385
}
382386
}
387+
else
388+
{
389+
// If the generic type arguments have changed, throw a specific exception for generics
390+
if (!CompareTypeMetadata(cmdlet, oldElementType, newElementType, issueLogger))
391+
{
392+
// If the generic type arguments aren't the same, log an issue
393+
issueLogger?.LogBreakingChangeIssue(
394+
cmdlet: cmdlet,
395+
severity: 0,
396+
problemId: ProblemIds.BreakingChangeProblemId.ChangedGenericTypeArgument,
397+
description: string.Format(Properties.Resources.ChangedGenericTypeArgumentDescription,
398+
target, oldArgument, newArgument),
399+
remediation: string.Format(Properties.Resources.ChangedGenericTypeArgumentRemediation,
400+
target, oldArgument));
401+
}
402+
}
383403
}
384404
}
385405
}
@@ -412,7 +432,7 @@ private bool HasSameGenericType(
412432
}
413433

414434
// Otherwise, log an issue and return false
415-
issueLogger.LogBreakingChangeIssue(
435+
issueLogger?.LogBreakingChangeIssue(
416436
cmdlet: cmdlet,
417437
severity: 0,
418438
problemId: ProblemIds.BreakingChangeProblemId.ChangedGenericType,
@@ -447,7 +467,7 @@ private bool HasSameGenericArgumentSize(
447467
}
448468

449469
// Otherwise, log an issue and return false
450-
issueLogger.LogBreakingChangeIssue(
470+
issueLogger?.LogBreakingChangeIssue(
451471
cmdlet: cmdlet,
452472
severity: 0,
453473
problemId: ProblemIds.BreakingChangeProblemId.DifferentGenericTypeArgumentSize,
@@ -459,38 +479,5 @@ private bool HasSameGenericArgumentSize(
459479

460480
return false;
461481
}
462-
463-
/// <summary>
464-
/// Check if the arguments of a generic type are the same. If they are not, log an issue.
465-
/// </summary>
466-
/// <param name="cmdlet">The cmdlet metadata currently being checked.</param>
467-
/// <param name="oldArgument">The old argument from the generic.</param>
468-
/// <param name="newArgument">The new argument from the generic</param>
469-
/// <param name="target">Target of the generic type breaking change.</param>
470-
/// <param name="issueLogger">ReportLogger that will keep track of issues found.</param>
471-
private bool CheckGenericTypeArguments(
472-
CmdletMetadata cmdlet,
473-
string oldArgument,
474-
string newArgument,
475-
string target,
476-
ReportLogger<BreakingChangeIssue> issueLogger)
477-
{
478-
if (oldArgument.Equals(newArgument, StringComparison.OrdinalIgnoreCase))
479-
{
480-
return true;
481-
}
482-
483-
// If the generic type arguments aren't the same, log an issue
484-
issueLogger.LogBreakingChangeIssue(
485-
cmdlet: cmdlet,
486-
severity: 0,
487-
problemId: ProblemIds.BreakingChangeProblemId.ChangedGenericTypeArgument,
488-
description: string.Format(Properties.Resources.ChangedGenericTypeArgumentDescription,
489-
target, oldArgument, newArgument),
490-
remediation: string.Format(Properties.Resources.ChangedGenericTypeArgumentRemediation,
491-
target, oldArgument));
492-
493-
return false;
494-
}
495482
}
496483
}

‎tools/StaticAnalysis/StaticAnalysis.Test/BreakingChangeAnalyzerTests.cs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -319,8 +319,7 @@ public void ChangeOutputType()
319319

320320
xunitOutput.WriteLine(output);
321321

322-
Assert.Equal(1, testReport.ProblemIdList.Count);
323-
Assert.True(testReport.ProblemIdList.Where<int>((problemId) => problemId.Equals(BreakingChangeProblemId.ChangedOutputType)).SingleOrDefault<int>().Equals(BreakingChangeProblemId.ChangedOutputType));
322+
Assert.Equal(0, testReport.ProblemIdList.Count);
324323
}
325324

326325
[Fact]

‎tools/VersionController/Models/VersionMetadataHelper.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ private void CheckBreakingChangesInTypes(
9090

9191
if (!newTypeMetadataDictionary.ContainsKey(type))
9292
{
93-
issueLogger.LogBreakingChangeIssue(
93+
issueLogger?.LogBreakingChangeIssue(
9494
cmdlet: cmdletMetadata,
9595
severity: 0,
9696
problemId: 0,

0 commit comments

Comments
 (0)