ISite.Container nullability annotation does not match the documentation

[jnm2]

ISite.Container is marked as non-nullable:

runtime/src/libraries/System.ComponentModel.Primitives/src/System/ComponentModel/ISite.cs

Line 29 in 627f133

IContainer Container { get; }

But the docs explicitly mention that the property can be null (https://docs.microsoft.com/en-us/dotnet/api/system.componentmodel.isite.container?view=netcore-3.1#remarks):

null for the Container property indicates that the IComponent instance does not have an ISite.

At least two .NET Framework implementations of ISite.Container return null, so this has been established for a while.

[ghost]

Tagging subscribers to this area: @safern
See info in area-owners.md if you want to be subscribed.

[ericstj]

cc @buyaa-n was this discussed? Couldn't find it here: dotnet/corefx#41185

[buyaa-n]

null for the Container property indicates that the IComponent instance does not have an ISite.

At least two .NET Framework implementations of ISite.Container return null, so this has been established for a while.

That Remark is a bit confusing, but yeah seems it should be nullable

Now wondering if the ISite.Component property also needs to be nullable, don't see null returning overload but its Remark:

A valid value for this property (that is, the value is not null) indicates that the component has been added to a container.

Also confusing ...

cc @buyaa-n was this discussed? Couldn't find it here: dotnet/corefx#41185

Doesn't seem we discussed it cc @stephentoub

[jeffhandley]

According to apisof.net, this API is used in only 6.2% of apps scanned; is the API commonly used in Windows desktop applications though? The bar we're using for 5.0 follows:

• Must not introduce new warnings. A few examples (but this is not exhaustive):
  • Changing the return type of a non-virtual member from nullable to non-nullable is OK; the other direction is NOT OK
  • Changing the return type of a virtual member is NOT OK
  • Changing a method parameter from non-nullable to nullable is OK; the other direction is NOT OK
• Must be related to annotations made during 5.0 (not in 3.0)
• Must be based on customer feedback
• Must affect a commonly-used API

If this is a commonly-used API for Windows desktop applications, then it would meet the bar.

[stephentoub]

We should make it nullable. But technically it doesn't meet the cited bar for release/5.0, as it could introduce new warnings. Whether it would actually cause problems in the .NET 5 build, not sure... I expect we'll have a couple of places to fix up in dotnet/runtime, but that the bulk of the impact would be on winforms, which I don't believe is annotated yet.

[jeffhandley]

it could introduce new warnings

Ah, right; of course. Thanks, @stephentoub. We'll keep this in 6.0 then.