Align WPF and WinForms code style guidelines

[JeremyKuhne]

WPF and WinForms aren't far off. We both point to the runtime repo and Framework Design Guidelines, but WinForms has elaborated a bit more:

https://github.com/dotnet/winforms/blob/main/docs/coding-style.md

If we share the same guidelines, it will facilitate moving between the two repos for both internal and external contributors.

@Kuldeep-MS, @pchaurasia14

[miloush]

I think I personally don't see any issues with those guidelines, assuming they are not enforced retrospectively in a breaking manner (e.g. turning public fields into properties).

One point of discussion we had recently was the naming convention for pointer variables, I don't see those covered.

Btw. why does it use backslashes for comments rather than forward slashes?

[h3xds1nz]

Besides what miloush has pointed out, imho using collection expressions in cases like:
return [];
over e.g.
return Array.Empty<T>();

or assigning e.g. private fields like:

_hooks = [];
over
_hooks = new List<T>();

should be discouraged.

I'm also not a fan of primary constructors but I guess it's whatever.

Otherwise I feel the guidelines are pretty well defined.

[JeremyKuhne]

I think I personally don't see any issues with those guidelines, assuming they are not enforced retrospectively in a breaking manner (e.g. turning public fields into properties).

@miloush No, we'll never make binary breaking changes. I'll call that out.

One point of discussion we had recently was the naming convention for pointer variables, I don't see those covered.

I'm going to call that out explicitly- I was just thinking about that again last night. I've tried a variety of options at scale, and I currently believe pointers should be named like any other variable, except in the case of pinning an existing variable. As pinning is usually constrained to a small block, I now use initials where the existing name is the best name:

fixed (char* n = name)
{
}

// Not
fixed (char* namePtr = name)

// Avoid (can lead to much longer P/Invoke statements)
fixed (char* namePointer = name)

Btw. why does it use backslashes for comments rather than forward slashes?

Whoops :) I'll fix that

using collection expressions

@h3xds1nz while personally I don't see [] for empty as a big win, analyzers are not so nuanced to have conditions here. Using collection expressions as a default can lead to performance improvements, notably with params ReadOnlySpan<> at the moment. C# prefers overloads that are params ReadOnlySpan<> and can stack allocate arrays for them, even with reference types. I introduced a number of these for System.Drawing this time and I suspect that there are plenty of opportunities throughout WPF to add these APIs.

I'm also not a fan of primary constructors but I guess it's whatever.

I'm going to update this one to "consider for internal types only". API review will currently not approve any primary constructor APIs (as C# is free to add additional API and behavior in new versions).

[miloush]

Looking through the WPF codebase for fixed blocks, I see 3 patterns:

1. Possibly the largest number is just prefix with p:

wpf/src/Microsoft.DotNet.Wpf/src/UIAutomation/UIAutomationClientSideProviders/MS/Internal/UnsupportedAutomationProxies/WindowsCalendar.cs

Line 210 in 5c87aff

fixed (NativeMethods.MCHITTESTINFO_V6* pHitTestInfo = &hitTestInfo)

wpf/src/Microsoft.DotNet.Wpf/src/Common/Graphics/exports.cs

Line 2000 in 5c87aff

fixed (float* pData = floatArray)

2. Then there are just normal variables:

wpf/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/Imaging/BitmapSource.cs

Line 684 in 5c87aff

fixed (void* pixelArray = &((byte[])pixels)[offset])

wpf/src/Microsoft.DotNet.Wpf/src/PresentationBuildTasks/MS/Internal/MarkupCompiler/PathInternal.cs

Lines 282 to 283 in 5c87aff

	fixed (char* f = first)
	fixed (char* s = second)

3. And then there is a few p prefixes with type indication:

wpf/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/StreamGeometry.cs

Line 91 in 5c87aff

fixed (byte *pbPathData = _data)

wpf/src/Microsoft.DotNet.Wpf/src/UIAutomation/UIAutomationClientSideProviders/MS/Internal/AutomationProxies/CommonXSendMessage.cs

Line 121 in 5c87aff

fixed (char* pchRes = achRes)

---

I was gonna say I have never seen "Pointer" being added but then

wpf/src/Microsoft.DotNet.Wpf/src/PresentationFramework/MS/Internal/IO/Packaging/UnsafeIndexingFilterStream.cs

Line 105 in 5c87aff

fixed (byte *bufferPointer = &buffer[offset])

Also this is a good one:

wpf/src/Microsoft.DotNet.Wpf/src/PresentationCore/MS/internal/TextFormatting/FormattedTextSymbols.cs

Lines 173 to 174 in 5c87aff

	fixed (char* fixedCharArray = &charArray[0])
	fixed (int* fixedNominalAdvances = &nominalAdvances[0])

@h3xds1nz's preference seems to be ptr prefix, while myself and @ThomasGoulet73 are used to p prefix. The p prefix might stand for pinned rather than pointer... The CLR via C# book uses p prefix. C# specification looks like p prefix to me, though more in pointer sense. Documentation for fixed statement is just chaos.

Either way, there is not that many instances of pointers in the repo and if the rules say "like normal variables", most of them would pass I guess.

[h3xds1nz]

@JeremyKuhne I know analyzers cannot enforce NOT using them in the cases I've mentioned, but it's definitely more readable to use the type IMHO and deserves to be mentioned if there's an agreement (I've mentioned it since you got multiple examples e.g. with succinct new() etc. and this falls into similar batch)

I know that unless you don't wanna define InlineArray yourself, you're forced to use them to get Roslyn generate an inline/stack-allocated array. Then again, people are not so knowledgable about these facts and nuances (e.g. using collection-expression for a heap-array type won't pre-allocate the capacity in case of List<T> (at least it didn't still 3 months ago, didn't check since then but doubt something changed), trying to concat two stack-allocated spans will generate a massive BS currently if you use collection-expressions instead of doing it manually via CopyTo, etc.

#9481 #9468 #10008 There's plenty of places I've managed to use params ROS<T> already directly. But yeah, as for public APIs, there's tons of places WPF could benefit to use ctors with (params) ROS but also methods. But as I've mentioned the other days, so far WPF managed to get maybe 4 new APIs since 2018? 😀

As for the pointer case since @miloush brought it up; yeah I'm fan of ptr but p prefix is more standardized imho and fine by me (though it implicates pinned, which ain't correct in a lot of "fixed" cases (e.g. stackalloc memory ain't pinned, just won't move), just takes time adjusting. In any case, I'd be definitely going for prefixes, it's even standard from Hungarian notation to use p prefix, and any kind of abbreviations (or the initials) are not a good practice IMHO.

[JeremyKuhne]

using collection-expression for a heap-array type won't pre-allocate the capacity in case of

Allowing C# to manage it they can improve behavior over time, much like they have for interpolated strings. I'm not precluding the option of pre-allocating the size- it is still completely legit to do List<int> numbers = new(100).

Either way, there is not that many instances of pointers in the repo and if the rules say "like normal variables", most of them would pass I guess.

Moving to CsWin32 and ComWrappers in WPF will expose us to more pinning for sure. If you look at WinForms we have almost 500 fixed statements. The vast majority are using the pin within a few lines of where they are pinned and don't benefit from a more descriptive name. While we haven't normalized all of the usages, there are a lot of real-world examples there.

Ideally C# would allow us to pin without needing another name as they're immutable anyway. Anything related to unsafe code isn't the highest priority for them though. :)

// The dream :)
fixed (char* name);
PInvoke.SetName(&name);

The biggest thing I don't want to see is devolving into using Hungarian notation as a broader thing. Perhaps a middle ground here of two options:

fixed (Point* ls = lineStart)
{
}

// Or
fixed (Point* point = &points)
{
}

fixed (char* pName = name)
{
    // Some larger block of code
}

// Not
fixed (char* namePtr = name)
fixed (char* c = name)

[h3xds1nz]

Allowing C# to manage it they can improve behavior over time, much like they have for interpolated strings. I'm not precluding the option of pre-allocating the size- it is still completely legit to do List<int> numbers = new(100).

I'd expect nothing less than improve of behavior over time but I cannot write it with my mind straight when I know what it is doing, haha. It's like cases where you'd e.g. want to put a ternary condition inside interpolated string e.g. (bool ? string literal : double) and you'll get a boxing append in the default interpolated string handler instead of separate generic and literal based on the condition.

// The dream :)
fixed (char* name);
`PInvoke.SetName(&name);

Something like that would be the dream indeed.

The biggest thing I don't want to see is devolving into using Hungarian notation as a broader thing. Perhaps a middle ground here of two options:

Nah, I definitely didn't mean to lean into any other aspects of it, but I'll pick the prefix naming scheme for pointers happily.

fixed (char* pName = name)
{
// Some larger block of code
}

Whether that's p (which would be the winner here given other members) or ptr anyday, over anything else.
It is the most descriptive IMHO and provides the best clarity; goes well with the fact you shall not use abbreviations, etc.

[JeremyKuhne]

Created a PR that starts this process. #10080

#10011 is one of the changes that would enable turning on the rules that #10080 disables.

[JeremyKuhne]

#10061 is another PR that would apply towards this.

Note that while a lot of the overrides are a reasonably quick auto-fix, there will be plenty of opportunity for manual fixes and follow up for a number of the rules. The idea would be to create issues for the remaining overrides and let community dive into contributing towards removing them as well.