fix(VersionCompat): Fix public override method removal being treated as a breaking changed

[efevans]

b/520002639

do not merge until squashing

[gemini-code-assist]

Code Review

This pull request introduces compatibility checks to handle cases where a method or property is removed from a class but an implementation still exists in an ancestor class. The review feedback identifies two important issues in the new logic: an infinite loop in GetPropertyImplementationFromAncestor caused by incorrectly reassigning baseType using descendentType, and a potential NullReferenceException if both the getter and setter of a property are null. Both comments are valid and should be addressed.

[efevans]

From what I can tell, VS is stripping the BOM. I'll revert this if requested, however from what I read online BOM marker is not-preferred by unix systems. Left the change in until it's requested removed.

[robertvoinescu-work]

Fine by me, seems to be what the /.editorconfig is setup to do so we should update there if we want to keep it as is.

[efevans]

BOM removal

[efevans]

BOM removal

[efevans]

/gemini review

[gemini-code-assist]

Code Review

This pull request updates the compatibility checker to avoid flagging removed methods or properties as breaking changes if a matching implementation is inherited from an ancestor class. It adds corresponding unit tests to verify these scenarios. The review feedback highlights critical operator precedence bugs in the AreLikeMethods helper functions that cause incorrect evaluations for non-virtual members, as well as a case of reversed arguments when calling this helper during property checks.

[robertvoinescu-work]

I think it looks good, but am suggesting a slightly different implementation. Let me know what you think.

[robertvoinescu-work]

Fine by me, seems to be what the /.editorconfig is setup to do so we should update there if we want to keep it as is.

[robertvoinescu-work]

I think it'd be a little more cohesive to me if we do this a little earlier and have all inherited methods be included ininO (and also inN). If we do it that way we can also avoid searching up the parent tree multiple times. Same comment for the property one. I think what this means is just translate your code that searches through parent classes to return all methods and use that in the passed methodSelector.

[efevans]

Moved the method discovery down below on line 266 so that when MethodsCtors() is called, the Func that will return all the methods will include those of the ancestors. This will then get used to defend against the false negatives.

Note that there is a change within MethodCtors() on line line 244 and 250 where we only include a message about a new or missing exported method if that method was defined at this level in the tree. This way we avoid duplicates messages about a removed exported method in an ancestor, otherwise it would show up once on the diff between the old and new ancestor class, as well as the diff between the old and new descendent class.

Properties has a similar change, except it just calls the ExportedMethod/ExportedProperties method without passing in a Func object (the method case needed this as it shared a definition with Ctors.

[efevans]

Ended up needing more changes that I expected, there's relatively similarly changes to both the methods and properties sides. Properties is a bit more awkward because they are comprised of accessors so in the ExportedProperties method, as we collect exported properties, we keep track of the one with the most accessors. I explain a bit more of the why in the XML doc for that method, but basically property overrides can't implement an accessor that wasn't in the base definition so as long as we always keep the implementation with more accessors, we should end up with PropertyDefinition that isn't missing any accessors.

[efevans]

Moved the method discovery down below on line 266 so that when MethodsCtors() is called, the Func that will return all the methods will include those of the ancestors. This will then get used to defend against the false negatives.

Note that there is a change within MethodCtors() on line line 244 and 250 where we only include a message about a new or missing exported method if that method was defined at this level in the tree. This way we avoid duplicates messages about a removed exported method in an ancestor, otherwise it would show up once on the diff between the old and new ancestor class, as well as the diff between the old and new descendent class.

Properties has a similar change, except it just calls the ExportedMethod/ExportedProperties method without passing in a Func object (the method case needed this as it shared a definition with Ctors.

[efevans]

Another note here similar to the changes removing the BOM in some files; .editorconfig contains a rule about this too csharp_space_after_cast = true so while I didn't intend for this change, it was picked up by VS. My personal preference would be to start allowing these small lint-related changes in PRs, maybe this would be best talked about when Amanda is back.

[efevans]

Cache for getting the parent type of a class to avoid the number of calls to Resolve() which can quickly get expensive and slow down the run time.

[robertvoinescu-work]

How big will this cache grow to? We are running the breaking change checker for multiple dlls and the garbage collector will be prevented from cleaning them up.

Instead of a cache and running the logic for each method what if we just push down the logic little lower (closer to the level you had before) and gate the evaluation behind a conditional so we grab inheritedO and inheritedN if we detect a breaking change - which is a rare case and so can probably take the occasional hit. For example something like this before any of the code in `CallStructInterface:

if (inO && !inN)
{
  // It might have been moved up to a parent class which is not a breaking change.
  // This is slow, but breaking changes are infrequent.
  var inheritedN = _n.GetInheritedMethod(o); // Maybe a if(_n.TryGetInheritedMethod(o, out var inheritedN))?
  if (inheritedN != null) 
  { 
    inN = true; 
    n = inheritedN; 
  }
}
else if (!inO && inN)
{
  // Similar here just swap o and n
  var inheritedO = _o.GetInheritedMethod(n);
  ...
}

Open to other suggestions too.

[efevans]

I think you're suggestion is a good middle ground, let's go with that. Does this look about where you're imagining these changes to go?

var oProps = _o.ExportedProperties().ToImmutableHashSet(SamePropertyComparer.Instance);
var nProps = _n.ExportedProperties().ToImmutableHashSet(SamePropertyComparer.Instance);
foreach (var prop in oProps.Union(nProps).OrderBy(x => x.FullName))
{
    var inO = oProps.TryGetValue(prop, out var o);
    var inN = nProps.TryGetValue(prop, out var n);

    // NEW CODE
    if (inO && !inN)
    {
        // get definition for n from _n's ancestor if it exists
    }
    else if (!inO && inN)
    {
        // get definition for o from _o's ancestor if it exists
    }
    // END NEW CODE
    
    // rest of implementation
    ...

[robertvoinescu-work]

Yeah i believe so. Thinking about it more we might just need the inO && !inN check. Correct me if I'm wrong but for !inO && inN we don't need to look up the family tree because we will eventually run the code for that parent and detect the change.

[efevans]

I may not be understanding your point, but you can still have a change that won't be detectable in the parent level and we'll need to look up the tree. Consider a hierarchy like this

class A { public virtual int Do() => 1; }
class B : A { public override int Do() => 2; }

if you removed the overriding Do from B in a change

class A { public virtual int Do() => 1; }
class B : A {  }

then there's no change at the parent level. The public surface of A also hasn't changed, but the current breaking changes checker will think there is a major breaking change due to Do not longer being defined at this level of the hierarchy.

You do bring up a good point that we should avoid duplicating diff changes, I'll need to ponder the implementation a bit more.

[robertvoinescu-work]

The current logic returns all methods including private and internal.

[robertvoinescu-work]

How big will this cache grow to? We are running the breaking change checker for multiple dlls and the garbage collector will be prevented from cleaning them up.

Instead of a cache and running the logic for each method what if we just push down the logic little lower (closer to the level you had before) and gate the evaluation behind a conditional so we grab inheritedO and inheritedN if we detect a breaking change - which is a rare case and so can probably take the occasional hit. For example something like this before any of the code in `CallStructInterface:

if (inO && !inN)
{
  // It might have been moved up to a parent class which is not a breaking change.
  // This is slow, but breaking changes are infrequent.
  var inheritedN = _n.GetInheritedMethod(o); // Maybe a if(_n.TryGetInheritedMethod(o, out var inheritedN))?
  if (inheritedN != null) 
  { 
    inN = true; 
    n = inheritedN; 
  }
}
else if (!inO && inN)
{
  // Similar here just swap o and n
  var inheritedO = _o.GetInheritedMethod(n);
  ...
}

Open to other suggestions too.

[efevans]

@robertvoinescu-work Some pretty big changes to the implementation. The methods and properties from ancestor classes is retrieved near the beginning of the MethodsCtors and Properties, but now we only go through the methods and properties for a class that could possibly cause a breaking change. Take the following class example:

old version:

public class MyClass
{
    public virtual int CountMoney() => 5;
}

public class ChildClass : MyClass
{
    public override int CountMoney() => 6;
}

new version:

public class MyClass
{
    public virtual int CountMoney() => 5;
}

public class ChildClass : MyClass {}

ChildClass overrides CountMoney(), but a new version with that override removed isn't included in the list (the ImmutableHashSet in both methods) that gets iterated over because neither side, old or new, has a definition that could be a major or minor change (I'm calling these "breakable" changes, even though they may end up just be minor or no changes at all depending on the other side).

There's some extension methods that abstract away the idea of what it means to be breakable, but the gist is a method or property is breakable if one of the following is true:

• It's the a non-virtual definition
• It's the original virtual definition of a virtual method/property
• it's a sealed virtual definition

These are the only cases (at least I could think up) that could result in a change that needs to be determined at this level of the family tree, whereas other changes like visibility, return type, etc. would all be found higher up in the tree where the method or property is originally defined.

Let me know what you think, I think this is a good trade off that solves the initial issue of the bug (as well as on the property side) without causing adverse side effects like extra reporting or excessively slow run times.