order of explicit xml docs with `inheritdoc`

ZacharyPatten commented 3 years ago

Currently, if you try to override XML docs from an inheritdoc they must appear above the inheritdoc.

Examples (that are currently failing)

/// <summary>hello world</summary>
/// <typeparam name="T">hello world</typeparam>
public class A<T> { }

/// <inheritdoc/>
/// <typeparam name="T">goodbye world</typeparam>
public class B<T> : A<T> { }

/// <inheritdoc cref="A{T}"/>
/// <typeparam name="T">goodbye world</typeparam>
public class C<T> { }

public class D
{
    /// <summary>hello world</summary>
    /// <typeparam name="T">hello world</typeparam>
    void Method1<T>() { }

    /// <inheritdoc cref="Method1{T}"/>
    /// <typeparam name="T">goodbye world</typeparam>
    void Method2<T>() { }
}

B<T>
- B<T> inheritdoc's from A<T> and attempts to override the XML docs for T but due to how the ordering of the XML docs are currently processed, it is not overriding. The XML docs for T on B<T> say hello world even though it has goodbye world directly on it.
C<T>
- Same as the B<T> example but a cref is used rather than class inheritance. The XML docs for T on C<T> say hello world even though it has goodbye world directly on it
D.Method2<T>
- Same as the other examples, but using methods rather than classes. The XML docs for T on D.Method2<T> say hello world even though it has goodbye world directly on it

This issue exists for all member types, not just methods and classes as shown in these examples.

Bug? Or just add a warning?

Personally, I am totally fine with how roslyn works, but I just want a warning to be in place if I accidentally add XML that is being hidden by an above inheritdoc.

However, @sharwell said this is a bug (explicit docs should always override inheritdoc regardless of order). https://gist.github.com/ZacharyPatten/cb37ae76a01b68d34efdfe7de7c041c4

Why am I okay with how roslyn currently works? Consider the scenario where you have multiple inheritdocs:

/// <summary>hello world</summary>
/// <typeparam name="T1">hello world</typeparam>
public class E<T1> { }

/// <summary>goodbye world</summary>
/// <typeparam name="T2">goodbye world</typeparam>
public class F<T2> { }

/// <inheritdoc cref="E{T1}"/>
/// <inheritdoc cref="F{T2}"/>
public class G<T1, T2> { }

The summary on G<T1, T2> is hello world because the E<T1> inheritdoc comes before the F<T2> inheritdoc.

There is no getting around the concept that order matters when using multiple inheritdocs. Yes there can be an exclusion for explicit XML docs on members, but I kinda like the idea of enforcing the order of the docs.

Conclusion

If this is considered a bug, then a fix would be nice.

If this is not considered a bug, then a compiler warning would be nice.

ZacharyPatten commented 3 years ago

@Youssef1313

ZacharyPatten commented 3 years ago

I had a brief discussion with @CyrusNajmabadi and we came to the conclusion it might be best not to treat this as a bug. Regardless, there are lots of questions I have about this topic and how inheritdoc should/is-supposed-to work in general. I included several examples on the PR that are either additional bugs or very unintuitive scenarios.

I converted the PR to a draft, because I think there really needs to be more discussion around how inheritdoc should work.

I would be happy to un-draft that PR if it was determined that the PR handles this issue in the appropriate manor, but without future discussion I plan on keeping it as a draft for reference.

andre-ss6 commented 2 years ago

I had a brief discussion with @CyrusNajmabadi and we came to the conclusion it might be best not to treat this as a bug.

Do you remember the reasoning behind this?

ZacharyPatten commented 2 years ago

I had a brief discussion with @CyrusNajmabadi and we came to the conclusion it might be best not to treat this as a bug.

Do you remember the reasoning behind this?

Order matters when it comes to XML docs. The quick info tool tip only shows the first node it finds. For example...

/// <summary>a</summary>
/// <summary>b</summary>
class A { }

... displays "a". The "b" node is ignored. In my opinion, inheritdoc should essentially work like a copy-paste as if you copied the xml content from the source directly to the target rather than using an inheritdoc, which means order would matter there as well. The only thing we really need is a warning to tell us if an explicit XML node is being hidden by an inheritdoc and that would fix pretty much everything.

However, if you want to treat this as a bug and have explicit XML on a member to override any inheritdoc things can get complicated. inheritdoc was intended to work while being nested inside another XML nodes, and allow you to specify an X-Path. Example:

/// <summary>
/// aaa
/// <para>bbb</para>
/// <para>ccc</para>
/// </summary>
class A { }

/// <summary>
/// ddd
/// <para><inheritdoc /></para>
/// </summary>
class B : A { }

Because inheritdoc is nested inside a para according to the current logic it is supposed to do something like copy the X-Path of the para node from the parent. But ugh this gets confusing and rather dumb in my opinion. Also there are numerous bugs right now with this feature, such as the X-Path not including the index of the node, so all nodes at the depth are merged into the resulting docs.

But back to the point... if explicit XML overrides inheritdoc then what do you do? Do you just wipe out the docs that are inherited? If you wipe them out it means they are gone in the inheritance from then on and you cannot X-Path it. Do you just reorganize the docs to make sure the explicit docs are always on top? That is essentially what I did in this PR https://github.com/dotnet/roslyn/pull/54618 but I ran into a lot of issues when doing that. Some issues were based on existing bugs but others were not.

andre-ss6 commented 2 years ago

I see.

I just now discovered while researching about this issue that you can specify the path parameter in order to filter tags to inherit; And wow, I also didn't know you could put <inheritdoc /> inside another tag 😮. Both of these are enough for my use case.

dotnet / roslyn