Skip to content

Disallow new abstract type members #259

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

Closed
wants to merge 1 commit into from
Closed

Disallow new abstract type members #259

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
17 changes: 14 additions & 3 deletions src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiListingComparer.cs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Eilon these "oops" comments are about corner cases. Worth filing bugs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I don't understand the comment in the code. What is "oops" about it? And what is the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

API check doesn't complain when an internal abstract member is added though that is a significant breaking change -- prevents creation of a subclass outside the defining assembly. As I said, it's a corner case but "oops", something is slipping though…

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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And the oops here is that API Check is disallowing a non-breaking change to a field. This one is also a corner case and it's also easy to work around (with entries in the breaking change files).

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
Open in desktop
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