Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SA1129 : C# 10 Re-evaluation #3430

Closed
Foxtrek64 opened this issue Jan 9, 2022 · 4 comments · Fixed by #3432
Closed

SA1129 : C# 10 Re-evaluation #3430

Foxtrek64 opened this issue Jan 9, 2022 · 4 comments · Fixed by #3432

Comments

@Foxtrek64
Copy link

Hello,

SA1129 as many of you know is a rule that has the developer prefer default(T) over new T() for value types. Historically, these were functionally equivalent and the former was preferred to avoid confusion and to make code consistent.

With C# 10, developers are allowed to create parameterless constructors for their structs, which means that these two forms are no longer (necessarily) equivalent.

As such, it is likely time to re-evaluate this rule and discuss whether to rescope or to retire this rule entirely.

I personally see three possibilities:

  1. Rescope entirely to builtin value types (prefer default(int) over new int()).
  2. Rescope entirely to builtin value types and invert preference (new int() or new() over default(int)).
  3. Retire entirely (possibly implementing 1 or 2 separately as a new rule).
@sharwell
Copy link
Member

I believe the current rule is still correct for almost all cases. The cases to consider, if any, would be:

  1. The behavior for new T() where T is a generic type parameter constrained to struct
  2. The behavior for new T() where T is a value type known to have a user-defined parameterless constructor in metadata

I believe the risk associated with unintentional creation of an invalid instance is sufficient that case (1) should retain the current behavior. Case (2) is a straightforward false positive that can be addressed at any time.

@sharwell
Copy link
Member

I fixed this with the conservative approach.

@alexrp
Copy link
Contributor

alexrp commented Jul 28, 2022

@sharwell What would be the recommendation for types like SpinWait? It doesn't have an explicit parameterless constructor and is designed to work default-initialized. I suspect most people would find var wait = new SpinWait(); ...; wait.SpinOnce(); more clear than var wait = default(SpinWait); ...; wait.SpinOnce();. But SA1129 wants the latter.

@sharwell
Copy link
Member

sharwell commented Sep 7, 2022

I believe the simplest recommendation is to just stick to the main rule. This is what the runtime repository itself does with SpinWait:
https://github.com/dotnet/runtime/blob/e48558390c988a830040e355c55f729615c91246/src/libraries/System.Private.CoreLib/src/System/Threading/SpinWait.cs#L319

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants