Runtime: Proposed rule: warn when calling Guid parameterless ctor

[GrabYourPitchforks]

Based on a conversation at https://twitter.com/twwilliams/status/1190312876927401984?s=21.

It's easy for developers to inadvertently write new Guid() in their code when they likely meant to write Guid.NewGuid. Since Guid is a value type, the ctor zero-inits the returned instance.

This proposal is for an analyzer to detect calls to this ctor and to produce a warning: direct the author to call NewGuid() instead if they intended to generate a new Guid, or explicitly write default(Guid) or Guid.Empty if they intended to zero-init.

Edit: Modified based on a suggestion from https://twitter.com/Jjagg_dev/status/1190799663441555456.

[jmarolf]

tagging @mikadumont and @terrajobst

[stephentoub]

I understand the benefit, but at the same time this could be super noisy. new Struct() is frequently used to zero-init, including for Guids, e.g. I just searched several repos... corefx has 22 such occurrences with Guid, roslyn has 5, aspnetcore has 3, etc., and from quick glance they're all for zero-init rather than expecting the equivalent of NewGuid.

If we're interested in such a rule around preferring default(Guid), why wouldn't we just have one for all structs? Seems like it'd be better to have such a style rule (as part of .editorconfig), and Guid would naturally fall out of that.

[GrabYourPitchforks]

If we're interested in such a rule around preferring default(Guid), why wouldn't we just have one for all structs?

Guid is particularly notorious because new Guid() and NewGuid() look nearly identical but have very different behaviors. I'm not aware of any other struct types where this particular point of confusion is so apparent.

[stephentoub]

That may be, but why restrict the analysis? What's to be gained by only flagging that case? And how do you intend to avoid the false positives, differentiating the cases where developers are using the new Xx() syntax rather than default(Xx) syntax, with the former having been standard way for many years?

There are also plenty of types that use Foo.Create static factories, for example (not just in the core libraries), which isn't that far off, and there are plenty of cases where types expose difference constructors with varying arguments that have different behaviors; this doesn't seem too different. Plus, if the language ever succeeds in exposing a value type's ability to have a parameterless constructor, IMHO it'll be even more important for code readers to understand where there's a difference between default(Struct) and new Struct(), such that we'd want the latter to be used only when it was actually meant to invoke a ctor.

My $.02, if we were to do such a rule, it should be all encompassing, focusing on the style difference to drive consistency, and the benefits for Guid would just fall out of that. If such a rule wanted to special-case the message that was generated for Guid, it could.

[mavasani]

Suggestion:

1. Implement this rule as _disabled by default_.
2. When the rule is enabled by the user via a ruleset/editorconfig entry, the rule fires on all value types like @stephentoub suggested. We can special case the diagnostic message for Guid.
3. Make the rule configurable so users can restrict the type names on which it fires. For example, introduce a new .editorconfig option applicable_symbol_names similar to disallowed-symbol-names.

[StingyJack]

This should be a CA rule that is enabled by default because it would prevent a common cause of defects. Every place I have worked in the last 20 years has had this new Guid() problem crop up into bugs, and thats not counting the times where it happened to a developer and they caught it before it left their workstation (but not before it ate up some productivity).

The usage of it described as a zero-init is something that I havent heard of before or seen outside of a Microsoft repository, but then I've only come across maybe 2 usages of a struct.

I have seen plenty of guid values being checked against Guid.Empty because of new Guid() and deserialization problems.

[sharwell]

StyleCop Analyzers has rule SA1129: Do not use default value type constructor for this.

[jmarolf]

@stephentoub I just searched several repos... corefx has 22 such occurrences with Guid, roslyn has 5, aspnetcore has 3, etc., and from quick glance they're all for zero-init rather than expecting the equivalent of NewGuid.

I would argue that most devs in the dotnet org already know the rules around structs and _especially_ know the rules around Guid.

If we're interested in such a rule around preferring default(Guid), why wouldn't we just have one for all structs?

I think a better suggestion would be to force the user to either use Guild.Empty or Guild.NewGuid so it is clear what the intent is. A new C# developer (or even a seasoned one) is not going to always know implicitly how structs work in this case. This feels very similar to the analyzers around ImmutableArray<T> which suggest to not use new ImmutableArray<T>() for this same reason.

[terrajobst]

I think we should teach developers to write

C# MyValueType myValue = default;

To zero-init value types. It's explicit and makes it very clear no constructor runs. I think it's better than Guid.Empty because it works for any value types.

[carlossanlop]

Let's move it to the runtime repo and prepare it for API review.

[Dotnet-GitSync-Bot]

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.

[stephentoub]

Let's move it to the runtime repo and prepare it for API review.

FWIW, note that dotnet/runtime has been using https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1129.md for this purpose for a while now.