Skip to content

ApiCheck: Print out breaking changes to be added as exceptions #238

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 25, 2017
Merged

ApiCheck: Print out breaking changes to be added as exceptions #238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Internal.AspNetCore.Sdk/build/ApiCheck.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
<Project>
<Target Name="ApiCheck" Condition=" '$(EnableApiCheck)' == 'true' ">
<PropertyGroup>
<_ApiListingFileSuffix Condition=" '$(TargetFrameworkIdentifier)' == '.NETFramework' ">net45.json</_ApiListingFileSuffix>
<_ApiListingFileSuffix Condition=" '$(TargetFrameworkIdentifier)' == '.NETFramework' ">netframework.json</_ApiListingFileSuffix>
Copy link
Contributor

@Eilon Eilon Apr 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 😄

<_ApiListingFileSuffix Condition=" '$(_ApiListingFileSuffix)' == '' ">netcore.json</_ApiListingFileSuffix>
<_ApiListingFilePath>$(MSBuildProjectDirectory)\baseline.$(_ApiListingFileSuffix)</_ApiListingFilePath>
<_ApiExclusionsFilePath>$(MSBuildProjectDirectory)\exceptions.$(_ApiListingFileSuffix)</_ApiExclusionsFilePath>
<_ApiExclusionsFilePath>$(MSBuildProjectDirectory)\breakingchanges.$(_ApiListingFileSuffix)</_ApiExclusionsFilePath>
<_ApiListingFile Condition=" Exists('$(_ApiListingFilePath)') ">$(_ApiListingFilePath)</_ApiListingFile>
<_ApiExclusionsFile Condition=" Exists('$(_ApiExclusionsFilePath)') ">$(_ApiExclusionsFilePath)</_ApiExclusionsFile>
</PropertyGroup>
Expand Down
13 changes: 7 additions & 6 deletions src/Microsoft.AspNetCore.BuildTools.ApiCheck.Task/ApiCheckTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -141,7 +141,7 @@ protected override string GenerateCommandLineCommands()
arguments = $@"""{Path.GetFullPath(toolPath)}"" ";
}

arguments += "compare --compact-output";
arguments += "compare";
if (ExcludePublicInternalTypes)
{
arguments += " --exclude-public-internal";
Expand All @@ -167,14 +167,15 @@ protected override string GetWorkingDirectory()
/// <inheritdoc />
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<object>());
}
else
{
Log.LogError(singleLine, Array.Empty<object>());
base.LogEventsFromTextOutput(singleLine, messageImportance);
}
}
}
Expand Down
27 changes: 0 additions & 27 deletions src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiChangeExclusion.cs
View file
Open in desktop

This file was deleted.

19 changes: 0 additions & 19 deletions src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiComparisonResult.cs
View file
Open in desktop

This file was deleted.

134 changes: 47 additions & 87 deletions src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiListingComparer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,128 +13,108 @@ public class ApiListingComparer
{
private readonly ApiListing _newApiListing;
private readonly ApiListing _oldApiListing;
private readonly IList<ApiChangeExclusion> _exclusions;

public ApiListingComparer(
ApiListing oldApiListing,
ApiListing newApiListing,
IList<ApiChangeExclusion> exclusions = null)
ApiListing newApiListing)
{
_oldApiListing = oldApiListing;
_newApiListing = newApiListing;
_exclusions = exclusions ?? new List<ApiChangeExclusion>();
}

public ApiComparisonResult GetDifferences()
public IList<BreakingChange> GetDifferences()
Copy link
Contributor Author

@smitpatel smitpatel Apr 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sole task or ApiListingComparer is to find differences between 2 ApiListing, removing known breaking changes should not be part of its tasks.

Copy link
Contributor

@dougbu dougbu Apr 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Eilon I asked @javiercn about the errors output for extraneous exceptions. He said you had wanted that feature and considered the extra exceptions errors. IIRC the purpose was to make sure we'd reported all real breaking changes at the end of a milestone. Has something changed?

Copy link
Contributor

@dougbu dougbu Apr 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, I now see the feature remains. Only change is that this class isn't involved.

{
var exclusions = _exclusions.ToList();

var breakingChanges = new List<BreakingChange>();
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<ApiChangeExclusion> exclusions, List<BreakingChange> breakingChanges)
private void CompareMembers(TypeDescriptor type, TypeDescriptor newType, List<BreakingChange> 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)));
Copy link
Contributor Author

@smitpatel smitpatel Apr 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part finds actually newly added members on interface (or renamed ones because they are remove/add)

}
}

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)
Expand Down Expand Up @@ -299,25 +279,5 @@ private bool HasCompatibleVisibility(TypeDescriptor oldType, TypeDescriptor newT
throw new InvalidOperationException("Unrecognized visibility");
}
}

private BreakingChange FilterExclusions(TypeDescriptor type, MemberDescriptor member, List<ApiChangeExclusion> 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);
}
}
}
44 changes: 37 additions & 7 deletions src/Microsoft.AspNetCore.BuildTools.ApiCheck/BreakingChange.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
}
}
}
1 change: 0 additions & 1 deletion src/Microsoft.AspNetCore.BuildTools.ApiCheck/ChangeKind.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ namespace ApiCheck
public enum ChangeKind
{
Removal,
Modification,
Copy link
Contributor

@Eilon Eilon Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+💯

Addition,
}
}
2 changes: 1 addition & 1 deletion src/Microsoft.AspNetCore.BuildTools.ApiCheck/Loader/AssemblyLoader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down
4 changes: 2 additions & 2 deletions src/Microsoft.AspNetCore.BuildTools.ApiCheck/Microsoft.AspNetCore.BuildTools.ApiCheck.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@
<Import Project="..\..\build\common.props" />

<PropertyGroup>
<VersionPrefix>1.0.2</VersionPrefix>
<VersionPrefix>1.0.3</VersionPrefix>
Copy link
Contributor Author

@smitpatel smitpatel Apr 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pranavkm - This version to bump?


<!--
The netstandard1.0 TFM doesn't actually compile. It's just there so Internal.AspNetCore.Sdk can take a dependency
on this project.
-->
<TargetFrameworks>netcoreapp1.1;net452;netstandard1.0</TargetFrameworks>
<TargetFrameworks>netcoreapp1.1;net46;netstandard1.0</TargetFrameworks>
</PropertyGroup>

<PropertyGroup Condition=" '$(TargetFramework)' == 'netstandard1.0' ">
Expand Down
Loading