Confusing documentation for `AggregateException.GetBaseException()`

[kmcclellan]

The comment for AggregateException.GetBaseException() says it returns an AggregateException, which is not always (or even usually) the case.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The comment for AggregateException.GetBaseException() says it returns an AggregateException, which is not always (or even usually) the case.

Author:	kmcclellan
Assignees:	-
Labels:	area-System.Runtime, untriaged
Milestone:	-

[skyoxZ]

I think these behaviors would be more intuitive:

Exception # GetBaseException()

=> InnerException is null ? this : InnerException.GetBaseException();

AggregateException # GetBaseException()

=> InnerExceptions.Count == 1 ? InnerException.GetBaseException() : this;

but current behaviors are not:

runtime/src/libraries/System.Private.CoreLib/src/System/Exception.cs

Line 65 in f2ac74b

public virtual Exception GetBaseException()

runtime/src/libraries/System.Private.CoreLib/src/System/AggregateException.cs

Line 217 in f2ac74b

public override Exception GetBaseException()

[danmoseley]

@skyoxZ your implementations above are functionally equivalent or am I missing something? they just use the stack.

@kmcclellan I agree the docs and comment doesn't match the implementation. It seems to me the implementation is right. ("Find the deepest exception, but stop in the case where there's more than one to choose from")

Do you want to offer a PR to fix the comment and add a test -- it does not seem to test the case that doesn't return an AggregateException..?

runtime/src/libraries/System.Threading.Tasks/tests/AggregateExceptionTests.cs

Lines 39 to 55 in f70d8a9

	public static void BaseExceptions()
	{
	AggregateException ex = new AggregateException();
	Assert.Equal(ex.GetBaseException(), ex);
	Exception[] innerExceptions = new Exception[0];
	ex = new AggregateException(innerExceptions);
	Assert.Equal(ex.GetBaseException(), ex);
	innerExceptions = new Exception[1] { new AggregateException() };
	ex = new AggregateException(innerExceptions);
	Assert.Equal(ex.GetBaseException(), innerExceptions[0]);
	innerExceptions = new Exception[2] { new AggregateException(), new AggregateException() };
	ex = new AggregateException(innerExceptions);
	Assert.Equal(ex.GetBaseException(), ex);
	}

The docs are in another repo, there's an edit button on the doc page

[skyoxZ]

Find the deepest exception, but stop in the case where there's more than one to choose from

@danmoseley Current behavior is not:

1. Exception.GetBaseException() always digs to InnerException even if reaching an AggregateException with more than 1 innerExceptions.

2. AggregateException.GetBaseException() will not dig more when its only inner exception is not an AggregateException even if there's deeper exception.

[danmoseley]

Ah, yes. I suspect this would be unacceptably breaking. @stephentoub ?

[stephentoub]

Ah, yes. I suspect this would be unacceptably breaking. @stephentoub ?

Yeah, I don't think there's a compelling enough ROI for such a change.

[danmoseley]

@skyoxZ do their ToString()'s at least include all relevant info? Those are far more heavily used.

On a somewhat related note, we are arguably missing a way to aggregate just Messages. Eg., this PR just now does it itself:
https://github.com/dotnet/msbuild/pull/8557/files#diff-88d685bfe8c280d1311439222f7714a8cecd2d2a69028a7dc7dd781cd91ff71bR129-R135

[skyoxZ]

yes, their ToString()'s include the whole inner-exception tree.

[tannergooding]

Given @stephentoub's comment, it sounds like this needs to be a simple docs change to call out the actual behavior.