Code modernization in System.Linq

[roji]

Here are three relatively impactful code cleanups, done automatically and split into three separate commits for easier reviewing/undoing etc.

[xtqqczze]

This might be too big to review. How about merging the most impactful change first, i.e. IDE0161: Convert to file-scoped namespace.

[stephentoub]

Use file-scoped namespace declarations in System.Linq

The repo doesn't use file-scoped namespaces; I don't think we want to change the style per library.

[stephentoub]

In the common case where the consumer doesn't add more to the returned list, this is increasing the memory allocated.

[roji]

Good catch, will revert these.

[MihaZupan]

Initial capacity shouldn't be an issue anymore as Roslyn will emit something like this now

List<TSource> list = new List<TSource>(1);
CollectionsMarshal.SetCount(list, 1);
Span<TSource> span = CollectionsMarshal.AsSpan(list);
span[0] = _item;

should we continue avoiding collection expressions in such cases?

[eiriktsarpalis]

Use file-scoped namespace declarations in System.Linq

The repo doesn't use file-scoped namespaces; I don't think we want to change the style per library.

@stephentoub do we want to consider switching the repo to file-scoped namespaces in the future? Would be nice if we could save ourselves one tab of indentation.

[roji]

This might be too big to review.

@xtqqczze note that each change is a distinct commit, that should help.

[EgorBo]

Use file-scoped namespace declarations in System.Linq

The repo doesn't use file-scoped namespaces; I don't think we want to change the style per library.

@stephentoub do we want to consider switching the repo to file-scoped namespaces in the future? Would be nice if we could save ourselves one tab of indentation.

afair some *.cs files have multiple namespace scopes in the same file?
I guess such a massive change will impact git blame experience (and, well, put all active PRs into a "in-conflict" state 😆)

[stephentoub]

Use file-scoped namespace declarations in System.Linq

The repo doesn't use file-scoped namespaces; I don't think we want to change the style per library.

@stephentoub do we want to consider switching the repo to file-scoped namespaces in the future? Would be nice if we could save ourselves one tab of indentation.

afair some *.cs files have multiple namespace scopes in the same file? I guess such a massive change will impact git blame experience (and, well, put all active PRs into a "in-conflict" state 😆)

It also makes servicing much harder.

[roji]

Thanks for the reviewing. Have removed the file-scoped namespaces, and corrected the other stuff.

[roji]

It also makes servicing much harder.

Yeah, FWIW in EF and Npgsql we try to do this kind of thing (and general large code cleanups) before we lock down a major release, to make servicing easier. It's true file-scoped namespaces do make git blaming slightly harder - I personally do think it's worth prioritizing code niceness with them, and worth the one-time pain (but I get that it's non-trivial and removed that commit).

[stephentoub]

before we lock down a major release, to make servicing easier

Can you elaborate?

[roji]

Can you elaborate?

Just that before a release is locked down, we usually do an automated code cleanup, to align code to the repo settings in .editorconfig and similar; this fixes any violations that have accumulated over the year from contributions where some rule wasn't respected. The timing - just before locking down the major version - is specifically done to make servicing easier, compared to e.g. if we did the cleanup right after the cleanup, at which point any backport would risk a conflict because of styling etc.

[stephentoub]

The timing - just before locking down the major version - is specifically done to make servicing easier, compared to e.g. if we did the cleanup right after the cleanup, at which point any backport would risk a conflict because of styling etc.

Thanks. dotnet/runtime invariably has multiple release branches active at any one time. That complicates this, eg when locking down the .NET 10 release, we'll still have release/8.0 and release/9.0 active for another 6-12 months.

[roji]

@stephentoub EF has the same servicing policy, with the multiple release branches - and it's indeed not ideal... Though the number of patches that go into older releases is typically very small compared to the number that goes into the just-released version (for example, there's typically a larger number of backports needed just after a major release, but far fewer that needs to go back even further).

Not necessarily trying to argue for anything here - just sharing info.

[eiriktsarpalis]

Should we perhaps open an issue tracking the introduction of file-scoped namespaces? Might be easier to invite debate from the wider team.

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

Files not reviewed (15)

• src/libraries/System.Linq/src/System/Linq/Single.cs: Evaluated as low risk
• src/libraries/System.Linq/src/System/Linq/GroupJoin.cs: Evaluated as low risk
• src/libraries/System.Linq/src/System/Linq/Skip.SizeOpt.cs: Evaluated as low risk
• src/libraries/System.Linq/src/System/Linq/Last.cs: Evaluated as low risk
• src/libraries/System.Linq/src/System/Linq/Select.SpeedOpt.cs: Evaluated as low risk
• src/libraries/System.Linq/src/System/Linq/Average.cs: Evaluated as low risk
• src/libraries/System.Linq/src/System/Linq/SequenceEqual.cs: Evaluated as low risk
• src/libraries/System.Linq/src/System/Linq/MaxMin.cs: Evaluated as low risk
• src/libraries/System.Linq/src/System/Linq/AppendPrepend.SpeedOpt.cs: Evaluated as low risk
• src/libraries/System.Linq/src/System/Linq/SkipTake.SpeedOpt.cs: Evaluated as low risk
• src/libraries/System.Linq/src/System/Linq/First.cs: Evaluated as low risk
• src/libraries/System.Linq/src/System/Linq/Skip.cs: Evaluated as low risk
• src/libraries/System.Linq/src/System/Linq/Join.cs: Evaluated as low risk
• src/libraries/System.Linq/src/System/Linq/OrderedEnumerable.SpeedOpt.cs: Evaluated as low risk
• src/libraries/System.Linq/src/System/Linq/Count.cs: Evaluated as low risk

[roji]

@eiriktsarpalis it doesn't seem from the above that there's a lot of appetite for doing that, but I'm definitely OK with it if @stephentoub, @EgorBo think it's worth discussing...