Validate null before mutating CodeNamespaceImportCollection

[nileshpatil6]

Fixes #131870.

Add(CodeNamespaceImport) reads value.Namespace before it touches _data, so a null argument throws NullReferenceException and the collection is left untouched. That is the behaviour Add_Null_ThrowsNullReferenceException pins.

Three other paths store first and validate afterwards, if at all:

• IList.Add is _data.Add((CodeNamespaceImport)value). The cast succeeds for null, ArrayList.Add accepts it, and the method never calls SyncKeys(), so it does not throw at all. The null stays in _data, and the next operation that calls SyncKeys() throws while enumerating it.
• IList.Insert inserts and then calls SyncKeys(), which throws on c.Namespace after the element is already in the list.
• The indexer setter does the same via _data[index] = value.

So the collection ends up in a state where every later mutation throws, which is what the issue reports.

This routes the three paths through a small helper that reads Namespace before the store, so they throw the same NullReferenceException at the same point as Add(CodeNamespaceImport) and leave the collection unchanged.

I checked the behaviour against the shipped System.CodeDom 8.0.0 first, then against the patched class compiled standalone:

                                   before        after
IList.Add(null) throws             no            yes
  collection usable afterwards     no            yes
IList.Insert(0, null) throws       yes           yes
  collection usable afterwards     no            yes
collection[0] = null throws        yes           yes
  collection usable afterwards     no            yes

Added three tests next to Add_Null_ThrowsNullReferenceException, each asserting both halves: that the null throws, and that a normal insert still works afterwards. The second half is the part that fails on current main.

I kept NullReferenceException rather than switching to ArgumentNullException, since the existing tests assert it for Add and AddRange and changing it would be a separate, observable break. Happy to switch if you would rather these paths threw ArgumentNullException.

One thing I noticed but did not change: IList.Add also never updates _keys, so a non-null value added through it is invisible to the duplicate check in Add(CodeNamespaceImport) and can be added twice. Routing IList.Add through the public Add would fix that too, but it changes the returned index and applies the dedup, so it seemed better left out of a null-handling fix. Let me know if you want it here.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[nileshpatil6]

@dotnet-policy-service agree

[Copilot]

Pull request overview

This PR fixes CodeNamespaceImportCollection becoming unusable after null is added via non-generic IList APIs or via the typed indexer setter. It does so by ensuring Namespace is read (and thus the same NullReferenceException is thrown) before mutating the backing _data collection, matching the existing behavior of Add(CodeNamespaceImport) and preventing the collection from entering an invalid state.

Changes:

• Add a small helper (Validated) that forces value.Namespace to be evaluated before storing into _data.
• Route IList.Add, IList.Insert, and CodeNamespaceImportCollection.this[int].set through that helper so null throws before mutation.
• Add regression tests covering the IList.Add, IList.Insert, and indexer-setter null scenarios, including verifying the collection remains usable afterwards.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/libraries/System.CodeDom/src/System/CodeDom/CodeNamespaceImportCollection.cs	Validates Namespace prior to mutation for IList mutation paths and the typed indexer setter to prevent invalid internal state.
src/libraries/System.CodeDom/tests/System/CodeDom/CodeNamespaceImportCollectionTests.cs	Adds regression tests ensuring null mutation paths throw and do not corrupt the collection for subsequent operations.

[dotnet-policy-service]

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

[huoyaoyuan]

Thanks you for your interest of contribution. System.CodeDom isn't being actively developed. Before continuing, we should let the area owner to decide this is an issue we actually care, and whether to preserve NullReferenceException or use ArgumentNullException instead.

[huoyaoyuan]

This is not the correct way to throw NullReferenceException. The compilers are allowed to treat the _ access as unused, despite they aren't currently doing so.

Also, it's not the pattern we use to validate a parameter then return it in a helper method.