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

Champion "Allow no-arg constructor and field initializers in struct declarations" #99

Open
4 of 5 tasks
Tracked by #829
gafter opened this issue Feb 14, 2017 · 360 comments
Open
4 of 5 tasks
Tracked by #829
Assignees
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion
Milestone

Comments

@gafter
Copy link
Member

gafter commented Feb 14, 2017

See also dotnet/roslyn#13921 dotnet/roslyn#1029

LDM history:

@iam3yal
Copy link
Contributor

iam3yal commented Feb 15, 2017

@jnm2 I don't think that it's about the benefit but more of a consistency so yes symmetry but dunno what it means for the run-time, I'm not sure what would happen when you define a parameterless constructor and set the fields, I voted because I hope that we could have default values but dunno.

@gafter gafter changed the title Allow no-arg constructor and field initializers in struct declarations Champion Allow no-arg constructor and field initializers in struct declarations Feb 15, 2017
@gafter gafter changed the title Champion Allow no-arg constructor and field initializers in struct declarations Champion "Allow no-arg constructor and field initializers in struct declarations" Feb 21, 2017
@gafter gafter added this to the 8.0 candidate milestone Feb 22, 2017
@DavidArno
Copy link

Downvoting this as per my comment for Roslyn #13921. For a struct, S, if default(S) and new S() leads to different values, then this feature has no real benefit and will cause serious bugs. This should be implemented properly in the CLR before it's considered for C#.

@orthoxerox
Copy link

I agree. If we're going to get non-nullable reference types and structs with parameterless constructors, then we should be able to override default for them,

@iam3yal
Copy link
Contributor

iam3yal commented Feb 22, 2017

I really hope that they will design and implement this properly.

@HaloFour
Copy link
Contributor

To play Devil's advocate, why is it expected that new struct() and default(struct) produce the same value? That's never been the case with reference types.

Does C# support consuming structs with default constructors today? You could always define one using IL or some other language. How about the IL that C# produces for a generic method with a new constraint? Would that work today with such a type?

The concept of a "default" for struct types is interesting but even in the case of a blittable default you're talking about adding some serious overhead.

@iam3yal
Copy link
Contributor

iam3yal commented Feb 22, 2017

@HaloFour Well, it might be possible to mitigate the overhead by an attribute? like opt-in/out?

@DavidArno
Copy link

@HaloFour,

Your devil's advocate is a good question. To my mind, the answer is "and that is exactly what's wrong with reference types". It is the driver behind non-nullable reference types. SO just at the time when we are looking to address the "null problem", we'd be introducing the same problem in to structs.

@HaloFour
Copy link
Contributor

@eyalsk @DavidArno

I think we're wading into the territory of a different discussion here.

I just tested and answered my questions. C# does currently support consuming structs that define default constructors. It just doesn't support defining them. There is one notable exception and that's in a generic method where the type has a constructor constraint. I believe that this was already considered a bug in the JIT.

@gafter
Copy link
Member Author

gafter commented Feb 22, 2017

We would need the core clr bug https://github.com/dotnet/coreclr/issues/6843 fixed before we could do this.

@jnm2
Copy link
Contributor

jnm2 commented Feb 22, 2017

What down-to-earth benefits would we gain from this proposal? I'm inclined to say this raises the complexity of the mental model with no benefits in sight, since no one in Codeplex, /roslyn or this thread has mentioned a single pragmatic benefit despite people repeatedly asking.

@iam3yal
Copy link
Contributor

iam3yal commented Feb 22, 2017

@jnm2 What do you mean? are you saying that it has zero benefit?

Just as a real world example, I have a text parser where the scanner returns a struct that looks like this:

public TextChar(char value, int index = 0, int lineNumber = 1, int columnNumber = 1)
{
	// ...
}

Now, when you create a new instance of the struct or use default lineNumber and columnNumber should default to 1 when there are no characters at all but atm it default to 0.

Update: Rephrased.

@DavidArno
Copy link

@jnm2,

Let's say I have a type that holds information on a car gearbox, including the number of gear stick positions and the gearing ratios and there will always be at least two positions (forward and reverse).

Currently, my options are to use a class (so I can control its construction) and suffer null ref problems, or have a struct. The latter then either suffers the equivalent of the null ref problem if created with default(Gearbox) or new Gearbox(), or I have to leave all the fields as mutable and, on all methods and properties of the struct, I have to put guard code that checks for a misconstruction and sets up a proper default set of values.

If this feature were correctly implemented, eg via an overridable default method in the struct, then I could set those default values (or even throw some "not properly constructed" exception at the point of creation) when default(Gearbox) or new Gearbox() is called.

As the proposal currently stands though, default(Gearbox) wouldn't call the constructor, so from my point of view it doesn't achieve anything useful: I'd still need that guard code or would still need to use a class.

@iam3yal
Copy link
Contributor

iam3yal commented Feb 23, 2017

BTW, some people workaround this by doing something like the following:

struct S
{
	private int _lineNumber;
	
	public S(int lineNumber = 1)
	{
		_lineNumber = lineNumber;
	}

	public int LineNumber => _lineNumber == default(int) ? 1 : _lineNumber;
}

Now, this can work but doing this for every field is just boilerplate.

@jnm2
Copy link
Contributor

jnm2 commented Feb 23, 2017

@DavidArno default is not a call. Default means zero-fill. Default happens when you create an array. Default happens to all class fields before your initialization code runs. Default happens due to a number of things, and it's for safety so that you don't get junk. Treating it like a call makes no sense because it isn't a call. We need it to be what it is: zero-fill.

@paulomorgado
Copy link

I would expect that new S() could be different from default(S).

That means that developers should be aware that their struct can be created without running any constructor.

@jnm2
Copy link
Contributor

jnm2 commented Feb 23, 2017

@eyalsk same thing about default. It means "I want this memory to be zero" and changing that semantic would be next to impossible, let alone desirable.

I'm not sure I follow you with the rest of what you showed. Those parameter defaults should only apply if you use that constructor. new TextChar() will not use the constructor because you did not specify the non-optional value, so you would not expect the other parameter defaults to apply since those parameters do not come into play since you chose not to use that constructor.

@HaloFour
Copy link
Contributor

HaloFour commented Feb 23, 2017

I'd like to reiterate that this proposal has nothing to do with changing how the C# compiler interprets new T() where T is a struct. That behavior is already means something different from default(T) and, as far as I'm aware, that has been the case since C# 1.0. To the CLR, and even to C#, new T() and default(T) have never meant the same thing.

Of course since neither C# nor VB.NET permitted defining a struct with a parameterless constructor the odds of running into one would be exceptionally low. So low that a bug made it into the BCL assuming that such a beast wouldn't exist. But it's quite possible for some other language to define such a struct, and C# already respects such structs by invoking their constructors.

So, in my opinion we need to move on from the conversation about whether structs should have parameterless constructors or whether C# should consider new T() and default(T) to be two different things. That ship has apparently sailed a long time ago. At this point the question should be only whether C# should permit defining new structs with parameterless constructors. Now, if you don't like the idea of parameterless constructors in structs and don't want Microsoft to encourage writing them I definitely think that's a fair argument.

@iam3yal
Copy link
Contributor

iam3yal commented Feb 23, 2017

@jnm2

I'm not sure I follow you with the rest of what you showed. Those parameter defaults should only apply if you use that constructor. new TextChar() will not use the constructor because you did not specify the non-optional value, so you would not expect the other parameter defaults to apply since those parameters do not come into play since you chose not to use that constructor.

Let's disregard my previous post because it wasn't clear so let's focus on the following examples instead:

public struct Line
{
	public Line(int lineNumber = 1)
	{
		LineNumber = lineNumber;
	}

	public int LineNumber { get; }
}

Possible today but when you do new TextChar() then LineNumber results 0 instead of 1.

public struct Line
{
	private readonly int _lineNumber = 1;

	public Line(int lineNumber)
	{
		_lineNumber = lineNumber;
	}

	public int LineNumber => _lineNumber;
}

Not possible today but I'd like to have it so in the case of the following code:

public struct TextChar
{
	private readonly int _lineNumber = 1;
	
	private readonly int _columnNumber = 1;
	
	public TextChar(char value, int index = 0, int lineNumber = 1, int columnNumber = 1)
	{
		Value = value;

		Index = index;

		_lineNumber = lineNumber;

		_columnNumber = columnNumber;
	}

	public int ColumnNumber => _columnNumber;

	public int Index { get; }

	public int LineNumber => _lineNumber;

	public char Value { get; }
}

When I'd do new TextChar() then LineNumber and ColumnNumber would result 1 as opposed to 0.

Now, it's a not a deal breaker but I think that having the ability to do it will fix the slight disparity with classes and people won't need to circumvent or fight the language and do something like this:

public int LineNumber => _lineNumber == default(int) ? 1 : _lineNumber;

@DavidArno
Copy link

DavidArno commented Feb 23, 2017

@HaloFour,

Currently, New S() and default(S) do the same thing in all .NET languages, due to the optimization introduced in Activator.CreateInstance. That comes about because the dominant .NET languages didn't allow custom parameterless constructors, so there was no point in calling it for structs. Because of that, lots of code will have been written that uses default(S) instead of new S(), because they currently do exactly the same thing. As far as I can see therefore, "fixing" that optimisation, and allowing the two struct creation options to behave differently could prove a huge breaking change for many folk.

@JMN2,
If non-nullable reference types are introduced, then for a non-nullable class, C, what would you expect the result of default(S) to be? "Zero-filled" null? That seems broken to me as suddenly my non-nullable class as null reference, which just broke my code. Likewise with structs that implement parameterless constructors. Those constructors are being implemented because a zero-filled default doesn't make sense for that type, therefore a zero-filled default(S) would be an invalid value for that struct.

@HaloFour
Copy link
Contributor

HaloFour commented Feb 23, 2017

@DavidArno

Currently, new S() and default(S) do the same thing in all .NET languages, due to the optimization introduced in Activator.CreateInstance.

That only affects specific uses with generics. When using a struct directly the C# compiler will always invoke the parameterless constructor if it exists. That is also being regarded as a bug in the CLR which at the time of implementation was itself a breaking change.

Even though I do largely use the generic type argument syntax, I am referring to all uses of structs in my argument. If you were handed an assembly the contained a struct called FooStruct and you used new FooStruct() that never implied the same thing as default(FooStruct).

void Test1() {
    var s = new FooStruct(); // invokes parameterless constructor
}

void Test2<T>() where T : struct {
    var s = new T(); // invokes parameterless constructor
}

void Test3<T>() where T : new() {
    var s = new T(); // does not invoke parameterless constructor
}

As mentioned in https://github.com/dotnet/coreclr/issues/6843, structs with parameterless constructors are a part of the CLI spec. As far as I can tell they also meet CLS requirements. C# is required to at least respect them. So is the CLR.

@jnm2
Copy link
Contributor

jnm2 commented Feb 23, 2017

@eyalsk So all you are really asking for is struct field initializers. (Field initializers would be possible if you call new Struct() but impossible if you use default(Struct) because, as previously mentioned, default(T) is defined as zeroing the containing memory space.)

I suppose it's a matter of perspective. Since default(Struct) is not going away, nor should it, you'll still have to deal with (new Struct[1])[0] zero-initializing all the fields and you'll still have to harden your struct against zero-inited fields. Since you'll still always have to handle zero-init fields, are field initializers that much of a win? Field initializers can only work if you call a constructor. Creating an array does not call constructors for each element.

@iam3yal
Copy link
Contributor

iam3yal commented Feb 24, 2017

@jnm2 I can relate to what @DavidArno says but at the same time I wouldn't mind that new S() and default(S) would result a different value.

I wasn't speaking about default and as @HaloFour said we shouldn't derail the discussion as it should be discussed in a new issue so I didn't mention it.

are field initializers that much of a win? Field initializers can only work if you call a constructor. Creating an array does not call constructors for each element.

You're right but at the same time it depends: in my case I'm not returning an array but an Enumerable<TextChar> and all the instance of TextChar are being properly initialized and it's only happens in one place this is the reason I wrote that for me, it isn't a deal breaker, it's just nice to have.

Beyond that I think that it might be nice to fix the slight disparity between classes and structs that might be a surprise to new comers.

@jnm2
Copy link
Contributor

jnm2 commented Feb 24, 2017

@eyalsk default is a necessary part of this conversation. I asked what the benefit of this proposal is; you answered that the benefit is not having to "fight the language" by worrying about zero-inited fields.

But because default will happen to every struct, therefore you must harden your field logic against zero-init. Because you must harden your field logic against zero-init, this proposal does not alleviate any worry about zero-inited fields. Therefore, that's not a benefit. Unless some other benefit is proposed, this proposal is at -100. :')

@jcouv
Copy link
Member

jcouv commented Feb 12, 2021

Moved this to the working set following recent LDM on record structs (https://github.com/dotnet/csharplang/blob/master/meetings/2021/LDM-2021-01-27.md#field-initializers).

@koszeggy
Copy link

Breaking: My unit tests revealed that not only Activator.CreateInstance has problems with struct default constructors (which is known and affects .NET Framework only), but Expression.New(Type) also has issues.

In fact, Expression.New() fails to execute the default constructor in all pre-.NET 6 versions. From the mentioned design meeting:

As .NET Framework is not a supported target platform for C# 10, however, we don't think this is a showstopper like it would have been back in C# 6.

They meant it for the Activator.CreateInstance bug. But what about Expression.New(Type), which fails in all versions, except .NET 6? To support all platforms now I use a workaround like this for non-.NET 6 targets.

@HaloFour
Copy link
Contributor

@koszeggy

But what about Expression.New(Type), which fails in all versions, except .NET 6?

Presumably this is less of a concern now that C# and .NET are shipped together in lockstep and C# 10 would not be supported on .NET versions lower than 6.

@koszeggy
Copy link

@HaloFour:

C# 10 would not be supported on .NET versions lower than 6.

As of today I can compile such a struct even for .NET Framework 3.5 without any warning. But I placed a comment also in the analyzers repo.

@CyrusNajmabadi
Copy link
Member

As of today I can compile such a struct even for .NET Framework 3.5 without any warning

I don't believe this is covered under our support matrix. Effectively, all bets are off with this. If you use language features not explicitly marked as supported for that version, then effectively you get 'undefined behavior'.

@Phyyl
Copy link

Phyyl commented Oct 25, 2021

I also think that default(T) and new T() should always remain the same. If I initialize an array of struct values, I expect the default constructor to run, no matter if I defined it or not. I don't have a solution for the performance issues this could create. One thing is for sure, in a closed project, if I define a default constructor and do complex things in it, I'd accept that any array initialization would be slower. Allowing users to define a custom default value might be a better option for performance.

@Joe4evr
Copy link
Contributor

Joe4evr commented Oct 26, 2021

I also think that default(T) and new T() should always remain the same.

This is already not the case if you define a parameterless constructor in IL, and has been the case since C# 1.0.

See earlier in this thread: #99 (comment)

Your assumptions aren't correct. Structs with parameterless constructors have always been supported by the CLR. C# has also always supported consuming structs that define parameterless constructors in that if the compiler sees that such a constructor exists it will already invoke it if you use new T(). The difference between new T() and default(T) has been a part of the CLR and of C# since 1.0.

@333fred 333fred added the Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification label Jan 11, 2022
@charlesroddie
Copy link

Is there a way to get a warning on use of new T() for structs without parameterless constructors? new ImmutableArray<int>() and new List() look alike but the first will give NREs since it only appears to be a constructor. This is very dangerous.

@CyrusNajmabadi
Copy link
Member

Is there a way to get a warning on use of new T() for structs without parameterless constructors?

Yes. An analyzer would be able to look for such a thing and report such a problem for you in your code. You could also publish such a thing if you thought it would be valuable for others :)

@TahirAhmadov
Copy link

I think such an analyzer was (going to be?) added to one of the built-in analyzer libraries, but I'm not sure what the status of it is.

I do think we should push the ecosystem away from new S() when it actually means default(S) and we should do it increasingly aggressively, up to and including making it an error in a few years.

@jcouv jcouv moved this to Language/design in Compiler: Julien's umbrellas Jun 3, 2024
@dotnet dotnet locked and limited conversation to collaborators Nov 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion
Projects
Status: Language/design
Development

No branches or pull requests