Remove always true "if" and unreachable code in System.Xml.Linq.XObject.SkipNotify method.

[andi9310]

This: if (o.Annotations() != null)
is always true because the above while can finish only in two conditions: when o != null or when o.annotations == null. The first condition will be catched by " if (o == null)" and if the second one is true, "o.Annotations() != null" will also always be true i think.

[davkean]

As mentioned on this pull request dotnet/corefx#55, we'd like to see some test coverage proving the bug, before we fix the bug.

[carlos-sarmiento]

I want to fix this issue. Regarding @davkean comment in dotnet/corefx#55 I found out the following:

If we check the C# 5.0 Language Specification, section 10.14.4 states (in part):
"When a function member returning an enumerator interface type is implemented using an iterator block, invoking the function member does not immediately execute the code in the iterator block. Instead, an enumerator object is created and returned. "

Then, by design, a call to a function defining an iterator block (such as yield) returns an enumerator object whose MoveNext method implements the function that uses the yield statement. As such, we have a guarantee that it is impossible for o.Annotations() to be null, since in ALL cases, we will receive an object.

So it is impossible to create a test that will use that part of the code, since it is impossible for the method to return null. At the same time, we know for a fact that the method will always return an object and the check is redundant.

[IDisposable]

Carlos's analysis is correct, you cannot write a test to "expose a bug" that can never occur. Rather, this is simple code-path analysis that calls out dead code.

[davkean]

Sorry, I should clarify - I agree with the assessment of that the code is dead. However, I do not agree to the removal of said dead code, I would rather see the code changed to follow the spirit of which it was written (which appears to be to raise some Changing/Changed events).

ie My statement is, write some tests that show that these Changing/Changed events do not fire when they are supposed to, and then change the code to make them fire.

[sharwell]

While pull request dotnet/corefx#55 is a correct interpretation of the original code, I believe the original code did not behave as intended. I believe the original code was meant to read as follows:

if (o.Annotations<XObjectChangeAnnotation>().Any()) return false;

If the condition was written like this, the meaning of the method would be:

Skip notify unless the current object or any of its parents includes the XObjectChangeAnnotation annotation.

As it was actually written, and as dotnet/corefx#55 preserved, the meaning of the method is:

Skip notify unless the current object or any of its parents includes any annotation.