Skip to content

Commit

Permalink
Disallow new abstract type members
Browse files Browse the repository at this point in the history 
- #146
- also add tests of field removals

nits:
- whitespace cleanup in `ApiListingGenerator` and `ApiListingComparerTests`
- remove unused `additionalFilters` parameter from `ApiListingComparerTests.CreateApiListingDocument()`
  • Loading branch information
@dougbu
dougbu committed May 19, 2017
1 parent da73e10 commit 055bbc9
Show file tree
Hide file tree
Showing 5 changed files with 286 additions and 43 deletions.
17 changes: 14 additions & 3 deletions src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiListingComparer.cs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// 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;
Expand Down Expand Up @@ -68,9 +68,20 @@ private void CompareMembers(TypeDescriptor type, TypeDescriptor newType, List<Br
}
}

if (type.Kind == TypeKind.Interface && newMembers.Count > 0)
if (newMembers.Count > 0)
{
breakingChanges.AddRange(newMembers.Select(member => new BreakingChange(newType.Id, member.Id, ChangeKind.Addition)));
if (type.Kind == TypeKind.Interface)
{
breakingChanges.AddRange(newMembers.Select(member => new BreakingChange(newType.Id, member.Id, ChangeKind.Addition)));
}
else
{
var disallowedNewMembers = newMembers.Where(member => member.Abstract).ToArray();
if (disallowedNewMembers.Length > 0)
{
breakingChanges.AddRange(disallowedNewMembers.Select(member => new BreakingChange(newType.Id, member.Id, ChangeKind.Addition)));
}
}
}
}

Expand Down
12 changes: 8 additions & 4 deletions src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiListingGenerator.cs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// 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;
Expand Down Expand Up @@ -220,6 +220,7 @@ public static MemberDescriptor GenerateMemberApiListing(TypeInfo type, MemberInf
}

return constructorDescriptor;

case MemberTypes.Method:
var method = (MethodInfo)member;
if (!method.IsPublic && !method.IsFamily && !method.IsFamilyOrAssembly)
Expand Down Expand Up @@ -274,6 +275,7 @@ public static MemberDescriptor GenerateMemberApiListing(TypeInfo type, MemberInf
methodDescriptor.ReturnType = TypeDescriptor.GetTypeNameFor(method.ReturnType.GetTypeInfo());

return methodDescriptor;

case MemberTypes.Field:
var field = (FieldInfo)member;
if (!field.IsPublic && !field.IsFamily && !field.IsFamilyOrAssembly)
Expand Down Expand Up @@ -318,12 +320,14 @@ public static MemberDescriptor GenerateMemberApiListing(TypeInfo type, MemberInf
// All these cases are covered by the methods they implicitly define on the class
// (Properties and Events) and when we enumerate all the types in an assembly (Nested types).
return null;

case MemberTypes.TypeInfo:
// There should not be any member passsed into this method that is not a top level type.
// There should not be any member passed into this method that is not a top level type.
case MemberTypes.Custom:
// We don't know about custom member types, so better throw if we find something we don't understand.
// We don't know about custom member types, so better throw if we find something we don't understand.
case MemberTypes.All:
throw new InvalidOperationException($"'{type.MemberType}' [{member}] is not supported.");

default:
return null;
}
Expand Down Expand Up @@ -457,4 +461,4 @@ rawDefaultValue is float ||
throw new InvalidOperationException("Unsupported default value type");
}
}
}
}
169 changes: 138 additions & 31 deletions test/ApiCheck.Test/ApiListingComparerTests.cs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// 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;
Expand All @@ -17,11 +17,10 @@ public class ApiListingComparerTests
public Assembly V1Assembly => typeof(ApiCheckApiListingV1).GetTypeInfo().Assembly;
public Assembly V2Assembly => typeof(ApiCheckApiListingV2).GetTypeInfo().Assembly;

public IEnumerable<Func<MemberInfo, bool>> TestFilters
=> new Func<MemberInfo, bool> []
{
ti => (ti as TypeInfo)?.Namespace?.StartsWith("ComparisonScenarios") == false
};
public IEnumerable<Func<MemberInfo, bool>> TestFilters => new Func<MemberInfo, bool> []
{
ti => (ti as TypeInfo)?.Namespace?.StartsWith("ComparisonScenarios") == false
};

[Fact]
public void Compare_Detects_ChangesInTypeVisibility_as_removal()
Expand Down Expand Up @@ -80,6 +79,68 @@ public void Compare_Detects_TypeGenericAritybreakingChanges_as_removal()
Assert.Contains(expected, breakingChanges);
}

[Theory]
[InlineData("public class ComparisonScenarios.ClassToRemoveFieldsFrom")]
[InlineData("public struct ComparisonScenarios.StructToRemoveFieldsFrom")]
public void Compare_DetectsAllFieldRemovals(string typeToCheck)
{
// Arrange
var v1ApiListing = CreateApiListingDocument(V1Assembly);
var v2ApiListing = CreateApiListingDocument(V2Assembly);
var comparer = new ApiListingComparer(v1ApiListing, v2ApiListing);

// Oops. The NewInternalProperty addition is a breaking change; makes it impossible to subclass type in
// another assembly.
var expected = new List<BreakingChange>
{
// Changing a const's value doesn't cause a binary incompatibility but often causes problems.
new BreakingChange(
typeToCheck,
"public const System.Int32 ConstToChangeValue = 1",
ChangeKind.Removal),
// Removing a const doesn't cause a binary incompatibilty but often causes problems.
new BreakingChange(
typeToCheck,
"public const System.Int32 ConstToMakeField = 2",
ChangeKind.Removal),
// Oops. Making a field writable is not technically a breaking change.
new BreakingChange(
typeToCheck,
"public readonly System.Int32 FieldToMakeWritable",
ChangeKind.Removal),
new BreakingChange(
typeToCheck,
"public static readonly System.Int32 StaticFieldToMakeConst",
ChangeKind.Removal),
// Oops. Making a field writable is not technically a breaking change.
new BreakingChange(
typeToCheck,
"public static readonly System.Int32 StaticFieldToMakeWritable",
ChangeKind.Removal),
new BreakingChange(
typeToCheck,
"public static System.Int32 StaticFieldToMakeReadonly",
ChangeKind.Removal),
new BreakingChange(
typeToCheck,
"public System.Int32 FieldToMakeReadonly",
ChangeKind.Removal),
new BreakingChange(
typeToCheck,
"public System.Int32 FieldToRemove",
ChangeKind.Removal),
};

// Act
var breakingChanges = comparer.GetDifferences();

// Assert
var breakingChanginesInType = breakingChanges
.Where(change => string.Equals(change.TypeId, typeToCheck, StringComparison.Ordinal))
.OrderBy(change => change.MemberId);
Assert.Equal(expected, breakingChanginesInType);
}

[Fact]
public void Compare_Detects_NamespacebreakingChanges_as_removal()
{
Expand Down Expand Up @@ -191,6 +252,56 @@ public void Compare_DoesNotFailForTypeAddingAnInterface()
bc => bc.TypeId == "public ComparisonScenarios.TypeWithExtraInterface"));
}

[Fact]
public void Compare_DetectsAbstractMethodAdditions()
{
// Arrange
var v1ApiListing = CreateApiListingDocument(V1Assembly);
var v2ApiListing = CreateApiListingDocument(V2Assembly);
var comparer = new ApiListingComparer(v1ApiListing, v2ApiListing);
var typeToCheck = "public abstract class ComparisonScenarios.AbstractClassToAddMethodsTo";

// Act
var breakingChanges = comparer.GetDifferences();

// Assert
var breakingChangesInType = breakingChanges
.Where(change => string.Equals(change.TypeId, typeToCheck, StringComparison.Ordinal));
var breakingChange = Assert.Single(breakingChangesInType);
Assert.Equal(ChangeKind.Addition, breakingChange.Kind);
Assert.Equal("public abstract System.Void NewAbstractMethod()", breakingChange.MemberId);
}

[Fact]
public void Compare_DetectsAbstractPropertyAdditions()
{
// Arrange
var v1ApiListing = CreateApiListingDocument(V1Assembly);
var v2ApiListing = CreateApiListingDocument(V2Assembly);
var comparer = new ApiListingComparer(v1ApiListing, v2ApiListing);
var typeToCheck = "public abstract class ComparisonScenarios.AbstractClassToAddPropertiesTo";
var expected = new List<BreakingChange>
{
new BreakingChange(
typeToCheck,
"public abstract System.Int32 get_NewAbstractProperty()",
ChangeKind.Addition),
new BreakingChange(
typeToCheck,
"public abstract System.Void set_PropertyToAddSetterTo(System.Int32 value)",
ChangeKind.Addition),
};

// Act
var breakingChanges = comparer.GetDifferences();

// Assert
var breakingChangesInType = breakingChanges
.Where(change => string.Equals(change.TypeId, typeToCheck, StringComparison.Ordinal))
.OrderBy(change => change.MemberId);
Assert.Equal(expected, breakingChangesInType);
}

[Fact]
public void Compare_DetectsNewMembersBeingAddedToAnInterface_as_addition()
{
Expand All @@ -203,9 +314,8 @@ public void Compare_DetectsNewMembersBeingAddedToAnInterface_as_addition()
var breakingChanges = comparer.GetDifferences();

// Assert
var interfaceBreakingChanges = breakingChanges.Where(
b => b.TypeId ==
"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);
Expand All @@ -222,23 +332,24 @@ public void Compare_AllowsExclusionsOnNewInterfaceMembers()
var comparer = new ApiListingComparer(v1ApiListing, v2ApiListing);

var knownBreakingChanges = new List<BreakingChange>
{
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)
};
{
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 breakingChanges = comparer.GetDifferences().Except(knownBreakingChanges);

// Assert
Assert.Null(breakingChanges.FirstOrDefault(
bc => bc.TypeId == "public interface ComparisonScenarios.IInterfaceToAddMembersTo"));
Assert.DoesNotContain(
breakingChanges,
bc => bc.TypeId == "public interface ComparisonScenarios.IInterfaceToAddMembersTo");
}

[Fact]
Expand All @@ -253,9 +364,8 @@ public void Compare_DetectsNewMembersInThePresenceOfRenamedAndRemovedMembers()
var breakingChanges = comparer.GetDifferences();

// Assert
var interfaceBreakingChanges = breakingChanges.Where(
b => b.TypeId ==
"public interface ComparisonScenarios.IInterfaceWithMembersThatWillGetRenamedRemovedAndAdded")
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);
Expand All @@ -279,9 +389,8 @@ public void Compare_DetectsNewMembersInThePresenceOfTheSameNumberOfRemovedAndAdd
var breakingChanges = comparer.GetDifferences();

// Assert
var interfaceBreakingChanges = breakingChanges.Where(
b => b.TypeId ==
"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);
Expand All @@ -297,11 +406,9 @@ public void Compare_DetectsNewMembersInThePresenceOfTheSameNumberOfRemovedAndAdd
b => b.MemberId == "System.Void ThirdAddedMember()" && b.Kind == ChangeKind.Addition);
}

private ApiListing CreateApiListingDocument(Assembly assembly,
IEnumerable<Func<MemberInfo, bool>> additionalFilters = null)
private ApiListing CreateApiListingDocument(Assembly assembly)
{
additionalFilters = additionalFilters ?? Enumerable.Empty<Func<MemberInfo, bool>>();
var generator = new ApiListingGenerator(assembly, TestFilters.Concat(additionalFilters));
var generator = new ApiListingGenerator(assembly, TestFilters);

return generator.GenerateApiListing();
}
Expand Down
63 changes: 60 additions & 3 deletions test/ApiCheckBaseline.V1/ComparisonScenarios.cs
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Copyright (c) .NET Foundation. All rights reserved.
// 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.



// V1
namespace ComparisonScenarios
{
Expand All @@ -18,6 +16,56 @@ public struct StructToMakeGeneric
{
}

public struct StructToRemoveFieldsFrom
{
public StructToRemoveFieldsFrom(int fieldToIgnore)
{
FieldToIgnore = 0;
FieldToMakeReadonly = 3;
FieldToRemove = 4;
FieldToMakeWritable = 5;
}

internal int FieldToIgnore;

public const int ConstToChangeValue = 1;

public const int ConstToMakeField = 2;

public int FieldToMakeReadonly;

public int FieldToRemove;

public readonly int FieldToMakeWritable;

public static int StaticFieldToMakeReadonly = 6;

public static readonly int StaticFieldToMakeConst = 7;

public static readonly int StaticFieldToMakeWritable = 8;
}

public class ClassToRemoveFieldsFrom
{
internal int FieldToIgnore = 0;

public const int ConstToChangeValue = 1;

public const int ConstToMakeField = 2;

public int FieldToMakeReadonly = 3;

public int FieldToRemove = 4;

public readonly int FieldToMakeWritable = 5;

public static int StaticFieldToMakeReadonly = 6;

public static readonly int StaticFieldToMakeConst = 7;

public static readonly int StaticFieldToMakeWritable = 8;
}

public class ClassToChangeNamespaces
{
}
Expand Down Expand Up @@ -50,6 +98,15 @@ public class TypeWithExtraInterface
{
}

public abstract class AbstractClassToAddMethodsTo
{
}

public abstract class AbstractClassToAddPropertiesTo
{
public abstract int PropertyToAddSetterTo { get; }
}

public interface IInterfaceToAddMembersTo
{
bool ExistingMember { get; set; }
Expand Down
Loading

0 comments on commit 055bbc9

Please sign in to comment.