diff --git a/src/Internal.AspNetCore.Sdk/build/ApiCheck.targets b/src/Internal.AspNetCore.Sdk/build/ApiCheck.targets index be0ec4fe3..d9ced9ac6 100644 --- a/src/Internal.AspNetCore.Sdk/build/ApiCheck.targets +++ b/src/Internal.AspNetCore.Sdk/build/ApiCheck.targets @@ -5,10 +5,10 @@ - <_ApiListingFileSuffix Condition=" '$(TargetFrameworkIdentifier)' == '.NETFramework' ">net45.json + <_ApiListingFileSuffix Condition=" '$(TargetFrameworkIdentifier)' == '.NETFramework' ">netframework.json <_ApiListingFileSuffix Condition=" '$(_ApiListingFileSuffix)' == '' ">netcore.json <_ApiListingFilePath>$(MSBuildProjectDirectory)\baseline.$(_ApiListingFileSuffix) - <_ApiExclusionsFilePath>$(MSBuildProjectDirectory)\exceptions.$(_ApiListingFileSuffix) + <_ApiExclusionsFilePath>$(MSBuildProjectDirectory)\breakingchanges.$(_ApiListingFileSuffix) <_ApiListingFile Condition=" Exists('$(_ApiListingFilePath)') ">$(_ApiListingFilePath) <_ApiExclusionsFile Condition=" Exists('$(_ApiExclusionsFilePath)') ">$(_ApiExclusionsFilePath) diff --git a/src/Microsoft.AspNetCore.BuildTools.ApiCheck.Task/ApiCheckTask.cs b/src/Microsoft.AspNetCore.BuildTools.ApiCheck.Task/ApiCheckTask.cs index 366fd64d0..4b67fa707 100644 --- a/src/Microsoft.AspNetCore.BuildTools.ApiCheck.Task/ApiCheckTask.cs +++ b/src/Microsoft.AspNetCore.BuildTools.ApiCheck.Task/ApiCheckTask.cs @@ -123,7 +123,7 @@ protected override string GenerateFullPathToTool() if (Framework.StartsWith("net4", StringComparison.OrdinalIgnoreCase)) { var taskAssemblyFolder = Path.GetDirectoryName(GetType().GetTypeInfo().Assembly.Location); - return Path.GetFullPath(Path.Combine(taskAssemblyFolder, "..", "net452", ToolExe)); + return Path.GetFullPath(Path.Combine(taskAssemblyFolder, "..", "net46", ToolExe)); } // If muxer does not find dotnet, fall back to system PATH and hope for the best. @@ -141,7 +141,7 @@ protected override string GenerateCommandLineCommands() arguments = $@"""{Path.GetFullPath(toolPath)}"" "; } - arguments += "compare --compact-output"; + arguments += "compare"; if (ExcludePublicInternalTypes) { arguments += " --exclude-public-internal"; @@ -167,14 +167,15 @@ protected override string GetWorkingDirectory() /// protected override void LogEventsFromTextOutput(string singleLine, MessageImportance messageImportance) { - if (string.IsNullOrEmpty(singleLine) || singleLine.StartsWith(" ", StringComparison.Ordinal)) + // Since tool prints out formatted list of breaking changes, + // anything that starts with Error considered an error; the rest is user information. + if (singleLine.StartsWith("Error", StringComparison.OrdinalIgnoreCase)) { - // Almost everything the tool writes indicates an error. But, blank and indented lines are informative. - base.LogEventsFromTextOutput(singleLine, messageImportance); + Log.LogError(singleLine, Array.Empty()); } else { - Log.LogError(singleLine, Array.Empty()); + base.LogEventsFromTextOutput(singleLine, messageImportance); } } } diff --git a/src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiChangeExclusion.cs b/src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiChangeExclusion.cs deleted file mode 100644 index faf70f604..000000000 --- a/src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiChangeExclusion.cs +++ /dev/null @@ -1,27 +0,0 @@ -// Copyright (c) .NET Foundation. All rights reserved. -// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. - -namespace ApiCheck -{ - /// - /// Exclusion list for API changes. It contains the missing element id, (type or type + member) - /// which acts as a unique identifier for the exclusion. The new type id and the new member id - /// that replace them in case there's any. And the type of change performed, whether we changed - /// something on the signature of the type or member of we just deleted the entire element. - /// - /// Exclusions are only considered valid if the old elements on the baseline and the new elements - /// described in the exclusion match perfectly. No additional exclusion should be left out after - /// doing a successful api listing comparison. - /// - public class ApiChangeExclusion - { - public string OldTypeId { get; set; } - public string OldMemberId { get; set; } - public string NewTypeId { get; set; } - public string NewMemberId { get; set; } - public ChangeKind Kind { get; set; } - - public bool IsExclusionFor(string oldTypeId, string oldMemberId) => - oldTypeId != null && OldTypeId == oldTypeId && OldMemberId == oldMemberId; - } -} diff --git a/src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiComparisonResult.cs b/src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiComparisonResult.cs deleted file mode 100644 index 99a99dd13..000000000 --- a/src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiComparisonResult.cs +++ /dev/null @@ -1,19 +0,0 @@ -// Copyright (c) .NET Foundation. All rights reserved. -// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. - -using System.Collections.Generic; - -namespace ApiCheck -{ - public class ApiComparisonResult - { - public ApiComparisonResult(IList breakingChanges, IList exclusions) - { - BreakingChanges = breakingChanges; - RemainingExclusions = exclusions; - } - - public IList BreakingChanges { get; } - public IList RemainingExclusions { get; } - } -} diff --git a/src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiListingComparer.cs b/src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiListingComparer.cs index ace4af9ba..bc215093e 100644 --- a/src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiListingComparer.cs +++ b/src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiListingComparer.cs @@ -13,128 +13,108 @@ public class ApiListingComparer { private readonly ApiListing _newApiListing; private readonly ApiListing _oldApiListing; - private readonly IList _exclusions; public ApiListingComparer( ApiListing oldApiListing, - ApiListing newApiListing, - IList exclusions = null) + ApiListing newApiListing) { _oldApiListing = oldApiListing; _newApiListing = newApiListing; - _exclusions = exclusions ?? new List(); } - public ApiComparisonResult GetDifferences() + public IList GetDifferences() { - var exclusions = _exclusions.ToList(); - var breakingChanges = new List(); + var newTypes = _newApiListing.Types; + foreach (var type in _oldApiListing.Types) { var newType = _newApiListing.FindType(type.Name); - if (newType == null || !string.Equals(type.Id, newType.Id, StringComparison.Ordinal)) + if (newType == null) { - var isAcceptable = newType != null && IsAcceptableTypeChange(type, newType); - if (!isAcceptable) + breakingChanges.Add(new BreakingChange(type.Id, memberId: null, kind: ChangeKind.Removal)); + } + else + { + newTypes.Remove(newType); + + if (!string.Equals(type.Id, newType.Id, StringComparison.Ordinal) + && !IsAcceptableTypeChange(type, newType)) { - var typeChange = FilterExclusions(type, member: null, exclusions: exclusions); - if (typeChange != null) - { - breakingChanges.Add(typeChange); - } + breakingChanges.Add(new BreakingChange(type.Id, memberId: null, kind: ChangeKind.Removal)); + continue; } - } - - if (newType != null) - { - CompareMembers(type, newType, exclusions, breakingChanges); + CompareMembers(type, newType, breakingChanges); } } - return new ApiComparisonResult(breakingChanges, exclusions); + return breakingChanges; } - private void CompareMembers(TypeDescriptor type, TypeDescriptor newType, List exclusions, List breakingChanges) + private void CompareMembers(TypeDescriptor type, TypeDescriptor newType, List breakingChanges) { - var removedOrChanged = 0; + var newMembers = newType.Members.ToList(); + foreach (var member in type.Members) { - var newMember = newType.FindMember(member.Id); - var isAcceptable = IsAcceptableMemberChange(_newApiListing, newType, member); - if (isAcceptable) + if (IsAcceptableMemberChange(newType, member, out var newMember)) { - if (newMember == null) - { - removedOrChanged++; - } - - continue; + newMembers.Remove(newMember); } - - if (newMember == null) + else { - removedOrChanged++; - var memberChange = FilterExclusions(type, member, exclusions); - if (memberChange != null) - { - breakingChanges.Add(memberChange); - } + breakingChanges.Add(new BreakingChange(type.Id, member.Id, ChangeKind.Removal)); } } - if (type.Kind == TypeKind.Interface && type.Members.Count - removedOrChanged < newType.Members.Count) + if (type.Kind == TypeKind.Interface && newMembers.Count > 0) { - var members = newType.Members.ToList(); - foreach (var member in newType.Members) - { - var change = FilterExclusions(type, null, exclusions); - if (change == null) - { - members.Remove(member); - } - } - - if (type.Members.Count - removedOrChanged < members.Count) - { - breakingChanges.Add(new BreakingChange(type, "New members were added to the following interface")); - } + breakingChanges.AddRange(newMembers.Select(member => new BreakingChange(newType.Id, member.Id, ChangeKind.Addition))); } } - private bool IsAcceptableMemberChange(ApiListing newApiListing, TypeDescriptor newType, MemberDescriptor member) + private bool IsAcceptableMemberChange(TypeDescriptor newType, MemberDescriptor member, out MemberDescriptor newMember) { var acceptable = false; + newMember = null; var candidate = newType; while (candidate != null && !acceptable) { - if (candidate.Members.Any(m => m.Id == member.Id)) + var matchingMembers = candidate.Members.Where(m => m.Id == member.Id).ToList(); + + if (matchingMembers.Count == 1) { + newMember = matchingMembers.Single(); acceptable = true; } else if (member.Kind == MemberKind.Method) { - var newMember = newType.Members.FirstOrDefault(m => SameSignature(member, m)); - if (newMember != null) + var matchingMember = newType.Members.FirstOrDefault(m => SameSignature(member, m)); + if (matchingMember != null) { - acceptable = (!member.Sealed ? !newMember.Sealed : true) && - (member.Virtual ? newMember.Virtual || newMember.Override : true) && - (member.Static == newMember.Static) && - (!member.Abstract ? !newMember.Abstract : true); + acceptable = (member.Sealed || !matchingMember.Sealed) + && (!member.Virtual || matchingMember.Virtual || matchingMember.Override) + && member.Static == matchingMember.Static + && (member.Abstract || !matchingMember.Abstract); + + if (acceptable) + { + newMember = matchingMember; + } } } - candidate = candidate.BaseType == null ? null : FindOrGenerateDescriptorForBaseType(newApiListing, candidate); + candidate = candidate.BaseType == null ? null : FindOrGenerateDescriptorForBaseType(candidate); } return acceptable; } - private static TypeDescriptor FindOrGenerateDescriptorForBaseType(ApiListing newApiListing, TypeDescriptor candidate) + private TypeDescriptor FindOrGenerateDescriptorForBaseType(TypeDescriptor candidate) { - return newApiListing.FindType(candidate.BaseType) ?? - ApiListingGenerator.GenerateTypeDescriptor(candidate.Source.BaseType.GetTypeInfo(), newApiListing.SourceFilters); + return _newApiListing.FindType(candidate.BaseType) ?? + ApiListingGenerator.GenerateTypeDescriptor(candidate.Source.BaseType.GetTypeInfo(), _newApiListing.SourceFilters); } private bool SameSignature(MemberDescriptor original, MemberDescriptor candidate) @@ -299,25 +279,5 @@ private bool HasCompatibleVisibility(TypeDescriptor oldType, TypeDescriptor newT throw new InvalidOperationException("Unrecognized visibility"); } } - - private BreakingChange FilterExclusions(TypeDescriptor type, MemberDescriptor member, List exclusions) - { - var exclusion = exclusions - .FirstOrDefault(e => e.IsExclusionFor(type.Id, member?.Id)); - - if (exclusion != null) - { - var element = _newApiListing.FindElement(exclusion.NewTypeId, exclusion.NewMemberId); - if (exclusion.Kind == ChangeKind.Removal && element == null || - exclusion.Kind == ChangeKind.Modification && element != null || - exclusion.Kind == ChangeKind.Addition && element != null) - { - exclusions.Remove(exclusion); - return null; - } - } - - return member == null ? new BreakingChange(type) : new BreakingChange(member, type.Id); - } } } diff --git a/src/Microsoft.AspNetCore.BuildTools.ApiCheck/BreakingChange.cs b/src/Microsoft.AspNetCore.BuildTools.ApiCheck/BreakingChange.cs index 1063bf2c1..dff0d9fbb 100644 --- a/src/Microsoft.AspNetCore.BuildTools.ApiCheck/BreakingChange.cs +++ b/src/Microsoft.AspNetCore.BuildTools.ApiCheck/BreakingChange.cs @@ -1,21 +1,51 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using ApiCheck.Description; +using Microsoft.DotNet.PlatformAbstractions; namespace ApiCheck { public class BreakingChange { - public BreakingChange(ApiElement oldItem, string context = null) + public BreakingChange(string typeId, string memberId, ChangeKind kind) { - Context = context; - Item = oldItem; + TypeId = typeId; + MemberId = memberId; + Kind = kind; } - public string Context { get; } - public ApiElement Item { get; } + public string TypeId { get; } + public string MemberId { get; } + public ChangeKind Kind { get; } - public override string ToString() => Context == null ? Item.Id : $"{Context} => {Item.Id}"; + public override bool Equals(object obj) + { + if (ReferenceEquals(null, obj)) + { + return false; + } + + if (ReferenceEquals(this, obj)) + { + return true; + } + + return obj.GetType() == GetType() && Equals((BreakingChange)obj); + } + + private bool Equals(BreakingChange other) + { + return string.Equals(TypeId, other.TypeId) && string.Equals(MemberId, other.MemberId) && Kind == other.Kind; + } + + public override int GetHashCode() + { + var hashCodeCombiner = HashCodeCombiner.Start(); + hashCodeCombiner.Add(TypeId); + hashCodeCombiner.Add(MemberId); + hashCodeCombiner.Add(Kind); + + return hashCodeCombiner.CombinedHash; + } } } diff --git a/src/Microsoft.AspNetCore.BuildTools.ApiCheck/ChangeKind.cs b/src/Microsoft.AspNetCore.BuildTools.ApiCheck/ChangeKind.cs index e844265b5..9423d0be0 100644 --- a/src/Microsoft.AspNetCore.BuildTools.ApiCheck/ChangeKind.cs +++ b/src/Microsoft.AspNetCore.BuildTools.ApiCheck/ChangeKind.cs @@ -10,7 +10,6 @@ namespace ApiCheck public enum ChangeKind { Removal, - Modification, Addition, } } diff --git a/src/Microsoft.AspNetCore.BuildTools.ApiCheck/Loader/AssemblyLoader.cs b/src/Microsoft.AspNetCore.BuildTools.ApiCheck/Loader/AssemblyLoader.cs index 288878a42..ae8bf53e6 100644 --- a/src/Microsoft.AspNetCore.BuildTools.ApiCheck/Loader/AssemblyLoader.cs +++ b/src/Microsoft.AspNetCore.BuildTools.ApiCheck/Loader/AssemblyLoader.cs @@ -1,7 +1,7 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -#if NET452 +#if NET46 using System.IO; #endif using System.Reflection; diff --git a/src/Microsoft.AspNetCore.BuildTools.ApiCheck/Microsoft.AspNetCore.BuildTools.ApiCheck.csproj b/src/Microsoft.AspNetCore.BuildTools.ApiCheck/Microsoft.AspNetCore.BuildTools.ApiCheck.csproj index 7ea29507c..9de609c9a 100644 --- a/src/Microsoft.AspNetCore.BuildTools.ApiCheck/Microsoft.AspNetCore.BuildTools.ApiCheck.csproj +++ b/src/Microsoft.AspNetCore.BuildTools.ApiCheck/Microsoft.AspNetCore.BuildTools.ApiCheck.csproj @@ -2,13 +2,13 @@ - 1.0.2 + 1.0.3 - netcoreapp1.1;net452;netstandard1.0 + netcoreapp1.1;net46;netstandard1.0 diff --git a/src/Microsoft.AspNetCore.BuildTools.ApiCheck/Microsoft.AspNetCore.BuildTools.ApiCheck.nuspec b/src/Microsoft.AspNetCore.BuildTools.ApiCheck/Microsoft.AspNetCore.BuildTools.ApiCheck.nuspec index 88716e64b..502ecb0c4 100644 --- a/src/Microsoft.AspNetCore.BuildTools.ApiCheck/Microsoft.AspNetCore.BuildTools.ApiCheck.nuspec +++ b/src/Microsoft.AspNetCore.BuildTools.ApiCheck/Microsoft.AspNetCore.BuildTools.ApiCheck.nuspec @@ -11,7 +11,7 @@ - + diff --git a/src/Microsoft.AspNetCore.BuildTools.ApiCheck/Program.cs b/src/Microsoft.AspNetCore.BuildTools.ApiCheck/Program.cs index 685ae99ab..549991d6c 100644 --- a/src/Microsoft.AspNetCore.BuildTools.ApiCheck/Program.cs +++ b/src/Microsoft.AspNetCore.BuildTools.ApiCheck/Program.cs @@ -25,30 +25,46 @@ public static int Main(string[] args) app.Command("generate", c => { - var assemblyPathOption = c.Option("-a|--assembly", "Path to the assembly to generate the ApiListing for", CommandOptionType.SingleValue); - var assetsJson = c.Option("-p|--project", "Path to the project.assets.json file", CommandOptionType.SingleValue); - var framework = c.Option("-f|--framework", "The moniker for the framework the assembly to analize was compiled against.", CommandOptionType.SingleValue); - var noPublicInternal = c.Option("-epi|--exclude-public-internal", "Exclude types on the .Internal namespace from the generated report", CommandOptionType.NoValue); - var outputPath = c.Option("-o|--out", "Output path for the generated ApiListing file", CommandOptionType.SingleValue); + var assemblyPathOption = c.Option("-a|--assembly", + "Path to the assembly to generate the ApiListing for", CommandOptionType.SingleValue); + var assetsJson = c.Option("-p|--project", "Path to the project.assets.json file", + CommandOptionType.SingleValue); + var framework = c.Option("-f|--framework", + "The moniker for the framework the assembly to analize was compiled against.", + CommandOptionType.SingleValue); + var noPublicInternal = c.Option("-epi|--exclude-public-internal", + "Exclude types on the .Internal namespace from the generated report", + CommandOptionType.NoValue); + var outputPath = c.Option("-o|--out", "Output path for the generated ApiListing file", + CommandOptionType.SingleValue); c.HelpOption("-h|--help"); - c.OnExecute(() => OnGenerate(c, assemblyPathOption, assetsJson, framework, noPublicInternal, outputPath)); + c.OnExecute(() => OnGenerate(c, assemblyPathOption, assetsJson, framework, noPublicInternal, + outputPath)); }); app.Command("compare", c => { - var apiListingPathOption = c.Option("-b|--api-listing", "Path to the API listing file to use as reference.", CommandOptionType.SingleValue); - var exclusionsPathOption = c.Option("-e|--exclusions", "Path to the exclusions file for the ApiListing", CommandOptionType.SingleValue); - var assemblyPathOption = c.Option("-a|--assembly", "Path to the assembly to generate the ApiListing for", CommandOptionType.SingleValue); - var assetsJson = c.Option("-p|--project", "Path to the project.assets.json file", CommandOptionType.SingleValue); - var framework = c.Option("-f|--framework", "The moniker for the framework the assembly to analize was compiled against.", CommandOptionType.SingleValue); - var noPublicInternal = c.Option("-epi|--exclude-public-internal", "Exclude types on the .Internal namespace from the generated report", CommandOptionType.NoValue); - var compactOutputOption = c.Option("--compact-output", "Display an error on a single line (primarily for use within MSBuild)", CommandOptionType.NoValue); + var apiListingPathOption = c.Option("-b|--api-listing", + "Path to the API listing file to use as reference.", CommandOptionType.SingleValue); + var exclusionsPathOption = c.Option("-e|--exclusions", + "Path to the exclusions file for the ApiListing", CommandOptionType.SingleValue); + var assemblyPathOption = c.Option("-a|--assembly", + "Path to the assembly to generate the ApiListing for", CommandOptionType.SingleValue); + var assetsJson = c.Option("-p|--project", "Path to the project.assets.json file", + CommandOptionType.SingleValue); + var framework = c.Option("-f|--framework", + "The moniker for the framework the assembly to analize was compiled against.", + CommandOptionType.SingleValue); + var noPublicInternal = c.Option("-epi|--exclude-public-internal", + "Exclude types on the .Internal namespace from the generated report", + CommandOptionType.NoValue); c.HelpOption("-h|--help"); - c.OnExecute(() => OnCompare(c, apiListingPathOption, exclusionsPathOption, assemblyPathOption, assetsJson, framework, noPublicInternal, compactOutputOption)); + c.OnExecute(() => OnCompare(c, apiListingPathOption, exclusionsPathOption, assemblyPathOption, + assetsJson, framework, noPublicInternal)); }); app.HelpOption("-h|--help"); @@ -156,12 +172,11 @@ private static int OnGenerate( private static int OnCompare( CommandLineApplication command, CommandOption apiListingPathOption, - CommandOption exclusionsPathOption, + CommandOption breakingChangesPathOption, CommandOption assemblyPath, CommandOption assetsJson, CommandOption framework, - CommandOption excludeInternalNamespace, - CommandOption compactOutputOption) + CommandOption excludeInternalNamespace) { if (!apiListingPathOption.HasValue() || !assemblyPath.HasValue() || @@ -186,66 +201,120 @@ private static int OnCompare( oldApiListingFilters.Add(ApiListingFilters.IsInInternalNamespace); } - var oldApiListing = ApiListingGenerator.LoadFrom(File.ReadAllText(apiListingPathOption.Value()), oldApiListingFilters); + var oldApiListing = ApiListingGenerator.LoadFrom(File.ReadAllText(apiListingPathOption.Value()), + oldApiListingFilters); var generator = new ApiListingGenerator(assembly, newApiListingFilters); var newApiListing = generator.GenerateApiListing(); - var exclusions = !exclusionsPathOption.HasValue() ? - Enumerable.Empty() : - JsonConvert.DeserializeObject>(File.ReadAllText(exclusionsPathOption.Value())); + var knownBreakingChanges = (breakingChangesPathOption.HasValue() + ? JsonConvert.DeserializeObject>( + File.ReadAllText(breakingChangesPathOption.Value())) + : null) ?? new List(); + + var comparer = new ApiListingComparer(oldApiListing, newApiListing); + + var breakingChanges = comparer.GetDifferences(); + var newBreakingChanges = breakingChanges.Except(knownBreakingChanges).ToList(); + var incorrectBreakingChanges = knownBreakingChanges.Except(breakingChanges).ToList(); - var comparer = new ApiListingComparer(oldApiListing, newApiListing, exclusions.ToList()); + const string indent = " "; - var result = comparer.GetDifferences(); - var differences = result.BreakingChanges; + var breakingChangesToPrint = new List(); - var compactOutput = compactOutputOption.HasValue(); - if (!compactOutput) + if (newBreakingChanges.Count > 0) { + Console.WriteLine( + $"ERROR: Verifying breaking changes for framework {framework.Value()} failed."); + } + + var removedTypes = newBreakingChanges.Where(b => b.MemberId == null).OrderBy(b => b.TypeId).ToList(); + if (removedTypes.Count > 0) + { + Console.WriteLine(); + Console.WriteLine("The following types have been removed."); + Console.WriteLine(); + Console.WriteLine(string.Join(Environment.NewLine, removedTypes.Select(b => indent + b.TypeId))); Console.WriteLine(); } - foreach (var difference in differences) + breakingChangesToPrint.AddRange(removedTypes); + + var removedMembers = newBreakingChanges + .Where(b => b.MemberId != null && b.Kind == ChangeKind.Removal) + .OrderBy(b => b.MemberId) + .GroupBy(b => b.TypeId) + .ToList(); + if (removedMembers.Count > 0) { - if (compactOutput) - { - Console.WriteLine($@"ERROR: Missing type or member '{difference}'"); - } - else + Console.WriteLine(); + Console.WriteLine(); + Console.WriteLine("The following types have one or more members removed from them."); + Console.WriteLine(); + + foreach (var memberGrouping in removedMembers) { - Console.WriteLine($@"ERROR: Missing type or member - {difference}"); + Console.WriteLine(indent + memberGrouping.Key); + Console.WriteLine(string.Join(Environment.NewLine, + memberGrouping.Select(b => indent + indent + b.MemberId).ToList())); Console.WriteLine(); } } - var remainingExclusions = result.RemainingExclusions; - foreach (var exclusion in remainingExclusions) + breakingChangesToPrint.AddRange(removedMembers.SelectMany(t => t.ToList())); + + var newMembersOnInterfaces = newBreakingChanges + .Where(b => b.MemberId != null && b.Kind == ChangeKind.Addition) + .OrderBy(b => b.MemberId) + .GroupBy(b => b.TypeId) + .ToList(); + if (newMembersOnInterfaces.Count > 0) { - if (compactOutput) - { - Console.WriteLine($"ERROR: Exclusion is no longer necessary: Old type '{exclusion.OldTypeId}' member " + - $"'{exclusion.OldMemberId}, New type '{exclusion.NewTypeId}' member '{exclusion.NewMemberId}'"); - } - else - { - Console.WriteLine($@"ERROR: The following exclusion is in the exclusion file, but is no longer necessary: - Old type: {exclusion.OldTypeId} - Old member: {exclusion.OldMemberId} - New type: {exclusion.NewTypeId} - New member: {exclusion.NewMemberId}"); + Console.WriteLine(); + Console.WriteLine(); + Console.WriteLine("The following interfaces have one or more members added to them."); + foreach (var memberGrouping in newMembersOnInterfaces) + { + Console.WriteLine(indent + memberGrouping.Key); + Console.WriteLine(string.Join(Environment.NewLine, + memberGrouping.Select(b => indent + indent + b.MemberId).ToList())); Console.WriteLine(); } } - if (differences.Count > 0 || remainingExclusions.Count > 0) + breakingChangesToPrint.AddRange(newMembersOnInterfaces.SelectMany(t => t.ToList())); + + foreach (var exclusion in incorrectBreakingChanges) { - if (!compactOutput && differences.Count > 0) - { - Console.WriteLine("The process for breaking changes is described in: https://github.com/aspnet/Home/wiki/Engineering-guidelines#breaking-changes"); - Console.WriteLine("For how to add an exclusion to Api-check go to: https://github.com/aspnet/BuildTools/wiki/Api-Check#apicheck-exceptions"); - } + Console.WriteLine( + "ERROR: The following exclusion is in the exclusion file, but is no longer necessary:"); + Console.WriteLine(JsonConvert.SerializeObject(exclusion, Formatting.Indented)); + Console.WriteLine(); + } + + if (breakingChangesToPrint.Any()) + { + Console.WriteLine(); + Console.WriteLine(); + Console.WriteLine("Following is the list of exclusions that either need to be added to the list of breaking changes, or the breaking changes themselves need to be reverted:"); + Console.WriteLine( + JsonConvert.SerializeObject( + breakingChangesToPrint, + Formatting.Indented, + new JsonSerializerSettings + { + NullValueHandling = NullValueHandling.Ignore + })); + Console.WriteLine(); + } + + if (newBreakingChanges.Count > 0 || incorrectBreakingChanges.Count > 0) + { + Console.WriteLine( + "The process for breaking changes is described in: https://github.com/aspnet/Home/wiki/Engineering-guidelines#breaking-changes"); + Console.WriteLine( + "The process to add an exclusion to this tool is described in: https://github.com/aspnet/BuildTools/wiki/Api-Check#apicheck-exceptions"); + Console.WriteLine(); return Error; } diff --git a/test/ApiCheck.Test/ApiCheck.Test.csproj b/test/ApiCheck.Test/ApiCheck.Test.csproj index dc3887452..db1701690 100644 --- a/test/ApiCheck.Test/ApiCheck.Test.csproj +++ b/test/ApiCheck.Test/ApiCheck.Test.csproj @@ -3,7 +3,7 @@ - netcoreapp1.1;net452 + netcoreapp1.1;net46 netcoreapp1.1 diff --git a/test/ApiCheck.Test/ApiListingComparerTests.cs b/test/ApiCheck.Test/ApiListingComparerTests.cs index f59f7554b..db501ab98 100644 --- a/test/ApiCheck.Test/ApiListingComparerTests.cs +++ b/test/ApiCheck.Test/ApiListingComparerTests.cs @@ -1,4 +1,7 @@ -using System; +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; using System.Collections.Generic; using System.Linq; using System.Reflection; @@ -14,14 +17,14 @@ public class ApiListingComparerTests public Assembly V1Assembly => typeof(ApiCheckApiListingV1).GetTypeInfo().Assembly; public Assembly V2Assembly => typeof(ApiCheckApiListingV2).GetTypeInfo().Assembly; - public IEnumerable> TestFilters => new Func[] - { - ti => ti is TypeInfo && !((TypeInfo)ti).Namespace.StartsWith("ComparisonScenarios") - }; - + public IEnumerable> TestFilters + => new Func [] + { + ti => (ti as TypeInfo)?.Namespace?.StartsWith("ComparisonScenarios") == false + }; [Fact] - public void Compare_Detects_ChangesInTypeVisibility() + public void Compare_Detects_ChangesInTypeVisibility_as_removal() { // Arrange var v1ApiListing = CreateApiListingDocument(V1Assembly); @@ -29,14 +32,18 @@ public void Compare_Detects_ChangesInTypeVisibility() var comparer = new ApiListingComparer(v1ApiListing, v2ApiListing); // Act - var changes = comparer.GetDifferences(); + var breakingChanges = comparer.GetDifferences(); // Assert - var change = Assert.Single(changes.BreakingChanges, bc => bc.Item.Id == "public class ComparisonScenarios.PublicToInternalClass"); + var expected = new BreakingChange( + "public class ComparisonScenarios.PublicToInternalClass", + memberId: null, + kind: ChangeKind.Removal); + Assert.Contains(expected, breakingChanges); } [Fact] - public void Compare_Detects_TypeRenames() + public void Compare_Detects_TypeRenames_as_removal() { // Arrange var v1ApiListing = CreateApiListingDocument(V1Assembly); @@ -44,14 +51,18 @@ public void Compare_Detects_TypeRenames() var comparer = new ApiListingComparer(v1ApiListing, v2ApiListing); // Act - var changes = comparer.GetDifferences(); + var breakingChanges = comparer.GetDifferences(); // Assert - var change = Assert.Single(changes.BreakingChanges, bc => bc.Item.Id == "public interface ComparisonScenarios.TypeToRename"); + var expected = new BreakingChange( + "public interface ComparisonScenarios.TypeToRename", + memberId: null, + kind: ChangeKind.Removal); + Assert.Contains(expected, breakingChanges); } [Fact] - public void Compare_Detects_TypeGenericArityChanges() + public void Compare_Detects_TypeGenericAritybreakingChanges_as_removal() { // Arrange var v1ApiListing = CreateApiListingDocument(V1Assembly); @@ -59,14 +70,18 @@ public void Compare_Detects_TypeGenericArityChanges() var comparer = new ApiListingComparer(v1ApiListing, v2ApiListing); // Act - var changes = comparer.GetDifferences(); + var breakingChanges = comparer.GetDifferences(); // Assert - var change = Assert.Single(changes.BreakingChanges, bc => bc.Item.Id == "public struct ComparisonScenarios.StructToMakeGeneric"); + var expected = new BreakingChange( + "public struct ComparisonScenarios.StructToMakeGeneric", + memberId: null, + kind: ChangeKind.Removal); + Assert.Contains(expected, breakingChanges); } [Fact] - public void Compare_Detects_NamespaceChanges() + public void Compare_Detects_NamespacebreakingChanges_as_removal() { // Arrange var v1ApiListing = CreateApiListingDocument(V1Assembly); @@ -74,14 +89,18 @@ public void Compare_Detects_NamespaceChanges() var comparer = new ApiListingComparer(v1ApiListing, v2ApiListing); // Act - var changes = comparer.GetDifferences(); + var breakingChanges = comparer.GetDifferences(); // Assert - var change = Assert.Single(changes.BreakingChanges, bc => bc.Item.Id == "public class ComparisonScenarios.ClassToChangeNamespaces"); + var expected = new BreakingChange( + "public class ComparisonScenarios.ClassToChangeNamespaces", + memberId: null, + kind: ChangeKind.Removal); + Assert.Contains(expected, breakingChanges); } [Fact] - public void Compare_Detects_ClassBeingNested() + public void Compare_Detects_ClassBeingNested_as_removal() { // Arrange var v1ApiListing = CreateApiListingDocument(V1Assembly); @@ -89,14 +108,18 @@ public void Compare_Detects_ClassBeingNested() var comparer = new ApiListingComparer(v1ApiListing, v2ApiListing); // Act - var changes = comparer.GetDifferences(); + var breakingChanges = comparer.GetDifferences(); // Assert - var change = Assert.Single(changes.BreakingChanges, bc => bc.Item.Id == "public class ComparisonScenarios.ClassToNest"); + var expected = new BreakingChange( + "public class ComparisonScenarios.ClassToNest", + memberId: null, + kind: ChangeKind.Removal); + Assert.Contains(expected, breakingChanges); } [Fact] - public void Compare_Detects_ClassBeingUnnested() + public void Compare_Detects_ClassBeingUnnested_as_removal() { // Arrange var v1ApiListing = CreateApiListingDocument(V1Assembly); @@ -104,14 +127,18 @@ public void Compare_Detects_ClassBeingUnnested() var comparer = new ApiListingComparer(v1ApiListing, v2ApiListing); // Act - var changes = comparer.GetDifferences(); + var breakingChanges = comparer.GetDifferences(); // Assert - var change = Assert.Single(changes.BreakingChanges, bc => bc.Item.Id == "public class ComparisonScenarios.ClassToUnnestContainer+ClassToUnnest"); + var expected = new BreakingChange( + "public class ComparisonScenarios.ClassToUnnestContainer+ClassToUnnest", + memberId: null, + kind: ChangeKind.Removal); + Assert.Contains(expected, breakingChanges); } [Fact] - public void Compare_Detects_GenericTypeConstraintsBeingAdded() + public void Compare_Detects_GenericTypeConstraintsBeingAdded_as_removal() { // Arrange var v1ApiListing = CreateApiListingDocument(V1Assembly); @@ -119,14 +146,18 @@ public void Compare_Detects_GenericTypeConstraintsBeingAdded() var comparer = new ApiListingComparer(v1ApiListing, v2ApiListing); // Act - var changes = comparer.GetDifferences(); + var breakingChanges = comparer.GetDifferences(); // Assert - var change = Assert.Single(changes.BreakingChanges, bc => bc.Item.Id == "public class ComparisonScenarios.GenericTypeWithConstraintsToBeAdded"); + var expected = new BreakingChange( + "public class ComparisonScenarios.GenericTypeWithConstraintsToBeAdded", + memberId: null, + kind: ChangeKind.Removal); + Assert.Contains(expected, breakingChanges); } [Fact] - public void Compare_Detects_MethodParametersBeingAdded() + public void Compare_Detects_MethodParametersBeingAdded_as_removal() { // Arrange var v1ApiListing = CreateApiListingDocument(V1Assembly); @@ -134,10 +165,14 @@ public void Compare_Detects_MethodParametersBeingAdded() var comparer = new ApiListingComparer(v1ApiListing, v2ApiListing); // Act - var changes = comparer.GetDifferences(); + var breakingChanges = comparer.GetDifferences(); // Assert - var change = Assert.Single(changes.BreakingChanges, bc => bc.Item.Id == "public System.Void MethodToAddParameters()"); + var expected = new BreakingChange( + "public class ComparisonScenarios.ClassWithMethods", + "public System.Void MethodToAddParameters()", + kind: ChangeKind.Removal); + Assert.Contains(expected, breakingChanges); } [Fact] @@ -149,14 +184,15 @@ public void Compare_DoesNotFailForTypeAddingAnInterface() var comparer = new ApiListingComparer(v1ApiListing, v2ApiListing); // Act - var changes = comparer.GetDifferences(); + var breakingChanges = comparer.GetDifferences(); // Assert - Assert.Null(changes.BreakingChanges.FirstOrDefault(bc => bc.Item.Id == "public ComparisonScenarios.TypeWithExtraInterface")); + Assert.Null(breakingChanges.FirstOrDefault( + bc => bc.TypeId == "public ComparisonScenarios.TypeWithExtraInterface")); } [Fact] - public void Compare_DetectsNewMembersBeingAddedToAnInterface() + public void Compare_DetectsNewMembersBeingAddedToAnInterface_as_addition() { // Arrange var v1ApiListing = CreateApiListingDocument(V1Assembly); @@ -164,10 +200,17 @@ public void Compare_DetectsNewMembersBeingAddedToAnInterface() var comparer = new ApiListingComparer(v1ApiListing, v2ApiListing); // Act - var changes = comparer.GetDifferences(); + var breakingChanges = comparer.GetDifferences(); // Assert - Assert.Single(changes.BreakingChanges, bc => bc.Item.Id == "public interface ComparisonScenarios.IInterfaceToAddMembersTo"); + var interfaceBreakingChanges = breakingChanges.Where( + b => b.TypeId == + "public interface ComparisonScenarios.IInterfaceToAddMembersTo") + .ToList(); + Assert.Single(interfaceBreakingChanges, + b => b.MemberId == "System.Int32 get_NewMember()" && b.Kind == ChangeKind.Addition); + Assert.Single(interfaceBreakingChanges, + b => b.MemberId == "System.Void set_NewMember(System.Int32 value)" && b.Kind == ChangeKind.Addition); } [Fact] @@ -176,30 +219,26 @@ public void Compare_AllowsExclusionsOnNewInterfaceMembers() // Arrange var v1ApiListing = CreateApiListingDocument(V1Assembly); var v2ApiListing = CreateApiListingDocument(V2Assembly); - var comparer = new ApiListingComparer(v1ApiListing, v2ApiListing, new[] { - new ApiChangeExclusion - { - OldTypeId = "public interface ComparisonScenarios.IInterfaceToAddMembersTo", - OldMemberId = null, - NewTypeId = "public interface ComparisonScenarios.IInterfaceToAddMembersTo", - NewMemberId = "System.Int32 get_NewMember()", - Kind = ChangeKind.Addition - }, - new ApiChangeExclusion - { - OldTypeId = "public interface ComparisonScenarios.IInterfaceToAddMembersTo", - OldMemberId = null, - NewTypeId = "public interface ComparisonScenarios.IInterfaceToAddMembersTo", - NewMemberId = "System.Void set_NewMember(System.Int32 value)", - Kind = ChangeKind.Addition - } - }); + var comparer = new ApiListingComparer(v1ApiListing, v2ApiListing); + + var knownBreakingChanges = new List + { + new BreakingChange( + "public interface ComparisonScenarios.IInterfaceToAddMembersTo", + "System.Int32 get_NewMember()", + ChangeKind.Addition), + new BreakingChange( + "public interface ComparisonScenarios.IInterfaceToAddMembersTo", + "System.Void set_NewMember(System.Int32 value)", + ChangeKind.Addition) + }; // Act - var changes = comparer.GetDifferences(); + var breakingChanges = comparer.GetDifferences().Except(knownBreakingChanges); // Assert - Assert.Null(changes.BreakingChanges.FirstOrDefault(bc => bc.Item.Id == "public interface ComparisonScenarios.IInterfaceToAddMembersTo")); + Assert.Null(breakingChanges.FirstOrDefault( + bc => bc.TypeId == "public interface ComparisonScenarios.IInterfaceToAddMembersTo")); } [Fact] @@ -211,12 +250,21 @@ public void Compare_DetectsNewMembersInThePresenceOfRenamedAndRemovedMembers() var comparer = new ApiListingComparer(v1ApiListing, v2ApiListing); // Act - var changes = comparer.GetDifferences(); + var breakingChanges = comparer.GetDifferences(); // Assert - Assert.Single(changes.BreakingChanges, bc => bc.Item.Id == "System.Void MemberToBeRenamed()"); - Assert.Single(changes.BreakingChanges, bc => bc.Item.Id == "System.Void MemberToBeRemoved()"); - Assert.Single(changes.BreakingChanges, bc => bc.Item.Id == "public interface ComparisonScenarios.IInterfaceToAddMembersTo"); + var interfaceBreakingChanges = breakingChanges.Where( + b => b.TypeId == + "public interface ComparisonScenarios.IInterfaceWithMembersThatWillGetRenamedRemovedAndAdded") + .ToList(); + Assert.Single(interfaceBreakingChanges, + b => b.MemberId == "System.Void MemberToBeRenamed()" && b.Kind == ChangeKind.Removal); + Assert.Single(interfaceBreakingChanges, + b => b.MemberId == "System.Void MemberToBeRemoved()" && b.Kind == ChangeKind.Removal); + Assert.Single(interfaceBreakingChanges, + b => b.MemberId == "System.Void RenamedMember()" && b.Kind == ChangeKind.Addition); + Assert.Single(interfaceBreakingChanges, + b => b.MemberId == "System.Void AddedMember()" && b.Kind == ChangeKind.Addition); } [Fact] @@ -228,16 +276,29 @@ public void Compare_DetectsNewMembersInThePresenceOfTheSameNumberOfRemovedAndAdd var comparer = new ApiListingComparer(v1ApiListing, v2ApiListing); // Act - var changes = comparer.GetDifferences(); + var breakingChanges = comparer.GetDifferences(); // Assert - Assert.Single(changes.BreakingChanges, bc => bc.Item.Id == "System.Void FirstMemberToRemove()"); - Assert.Single(changes.BreakingChanges, bc => bc.Item.Id == "System.Void SecondMemberToRemove()"); - Assert.Single(changes.BreakingChanges, bc => bc.Item.Id == "System.Void ThirdMemberToRemove()"); - Assert.Single(changes.BreakingChanges, bc => bc.Item.Id == "public interface ComparisonScenarios.IInterfaceWithSameNumberOfRemovedAndAddedMembers"); + var interfaceBreakingChanges = breakingChanges.Where( + b => b.TypeId == + "public interface ComparisonScenarios.IInterfaceWithSameNumberOfRemovedAndAddedMembers") + .ToList(); + Assert.Single(interfaceBreakingChanges, + b => b.MemberId == "System.Void FirstMemberToRemove()" && b.Kind == ChangeKind.Removal); + Assert.Single(interfaceBreakingChanges, + b => b.MemberId == "System.Void SecondMemberToRemove()" && b.Kind == ChangeKind.Removal); + Assert.Single(interfaceBreakingChanges, + b => b.MemberId == "System.Void ThirdMemberToRemove()" && b.Kind == ChangeKind.Removal); + Assert.Single(interfaceBreakingChanges, + b => b.MemberId == "System.Void FirstAddedMember()" && b.Kind == ChangeKind.Addition); + Assert.Single(interfaceBreakingChanges, + b => b.MemberId == "System.Void SecondAddedMember()" && b.Kind == ChangeKind.Addition); + Assert.Single(interfaceBreakingChanges, + b => b.MemberId == "System.Void ThirdAddedMember()" && b.Kind == ChangeKind.Addition); } - private ApiListing CreateApiListingDocument(Assembly assembly, IEnumerable> additionalFilters = null) + private ApiListing CreateApiListingDocument(Assembly assembly, + IEnumerable> additionalFilters = null) { additionalFilters = additionalFilters ?? Enumerable.Empty>(); var generator = new ApiListingGenerator(assembly, TestFilters.Concat(additionalFilters));