dotnet / csharplang

The official repo for the design of the C# programming language
11.53k stars 1.03k forks source link

Champion "Target-typed `new` expression" (VS 16.8, .NET 5) #100

Open gafter opened 7 years ago

gafter commented 7 years ago

Summary: Allow Point p = new (x, y);

Shipped in preview in 16.7p1.


See also https://github.com/dotnet/roslyn/issues/35

Test plan: https://github.com/dotnet/roslyn/issues/28489

gulshan commented 7 years ago

At least this can be done with object initializers without type-name, which currently generate anonymous types.

Thaina commented 7 years ago

I am more fond of var everywhere instead of empty new

jnm2 commented 7 years ago

@Thaina I absolutely agree for locals, but for fields and properties this would actually be preferable to me and a_m_a_z_i_n_g, not gonna lie.

private readonly Dictionary<(Foo foo, Bar bar), List<Baz>> bazByFooAndBar = new();

Love it! Plus also:

private readonly Dictionary<Foo, Bar> barByFoo;

public ctor(IDictionary<Foo, Bar> initialValues)
{
   barByFoo = new(initialValues);
}

I do a lot of field initialization requiring access to this, so I end up saying new and repeating long types quite frequently in constructor bodies.

Thaina commented 7 years ago

@jnm2 Well, readonly in constructor seem great

But still that the only place I think we should do like that, I don't like mutable reference so field should not new anywhere. I wish we could have var field instead

Richiban commented 7 years ago

@Thaina

I wish we could have var field instead

I believe this is a limitation of the fact that C# has a two-pass compiler. The entire signature of a class needs to be known before any of the initialization expressions can be executed, so if you use var for a field the compiler doesn't know the type on the right-hand side of the =.

alrz commented 7 years ago

I think private var fields (and let for readonly) are ok, as long as they are immediately initialized, just like what we have for local vars.

gafter commented 7 years ago

I think private var fields (and let for readonly) are ok, as long as they are immediately initialized, just like what we have for local vars.

@alrz A var-declared local's initialization can only rely on earlier-declared locals. That eliminates the possibility of cycles. Fields do not have any corresponding restrictions that would eliminate such cycles by defining an order in which the types are inferred.

In any case, that would be a completely separate language proposal.

iam3yal commented 7 years ago

Yes please, let's make things less verbose whenever possible and whenever it make sense. :)

gordanr commented 7 years ago

I'm not sure is it related with this topic.

public List<Error> ValidateInput() => new List<Error>();

This piece of code could be, with some generalization, written as

public List<Error> ValidateInput() => new();

I really don't know is it readable and right direction. Possibly not. Just to leave a little note.

lachbaer commented 7 years ago

public List<Error> ValidateInput() => new();

I'm a bit riven by this. On one hand I think that a method's body should be independent of its signature, meaning that the creation of an object for a return should be verbose. On the other hand for expressions, so expression method bodies as well, this is very neat. It's like default vs. default(T), but I think default is valid in block bodies as well?

masaeedu commented 7 years ago

Constructors are kind of useless at the moment, you can't do type inference with them, can't pass a bound constructor as a first class function, etc. etc. At the same time you're stuck using them if you want to use readonly or getter-only properties. The end result is that every class I write has a static "constructor" function that simply delegates to the regular constructor, but behaves like a normal function from the perspective of consuming code.

It would be great if there could be some focus on bringing constructor functions into parity with plain old static methods, instead of piecemeal workarounds like this.

Joe4evr commented 6 years ago

Reading the raw notes:

It may be that there's a subset of the feature that's simpler. We would need that subset to not preclude going further later.

Looking at the concern that an API adding a new overload could break consumers, the "simple" subset of places where target-typed new can be applied (that I'm pretty sure won't preclude further options) would be

The first two are the places where it seems like most people are really clamoring for it; the last one I just came up at the spur of the moment.

orthoxerox commented 6 years ago

@Joe4evr Why would you want target-typed new for locals? There's var already. I would rather use it in function arguments.

quinmars commented 6 years ago

@orthoxerox for example when the declaration and the construction are not in the same scope:

List<string> list;

try
{
    list = service.GetList();
}
catch (Exception)
{
    list = new (); // <---
}
Thaina commented 6 years ago

@quinmars I was trying to solve that backward by #249

Like this

var list = :{
    try
    {
         return service.GetList();
    }
    catch
    {
         return new List<string>();
    }
}

There is also #616

Like this

var list = try service.GetList() catch new List<string>();
jcouv commented 6 years ago

Notes to self to remember to test or address (I'll keep adding entries):

CyrusNajmabadi commented 6 years ago

how should NormalizeWhitespace behave? Is it new(1, 2) or new (1, 2)?

My 2c: it should be "new(1, 2)" :)

alrz commented 6 years ago

There is a prototype available here: https://github.com/alrz/roslyn/commit/63f1bb7ad72be94e5668ee192d0e48d2a7a0b2d4

re open questions, if we define the the conversion such that it only depends on the constructor overload resolution (i.e. a conversion exists iff there exists an applicable constructor wrt provided arguments), we can bind initializers after the conversion succeeded.

/cc @jcouv

HakanL commented 6 years ago

@alrz Would this prototype also cover initialization of class member fields, I didn't see that in the unit tests?

alrz commented 6 years ago

@HakanL Yes, it's not well tested yet but you can check out this one. constructor args are undone though.

HakanL commented 6 years ago

Thanks for pointing that out. I guess I was looking for this type of pattern:

   public class X
   {
       MyClass abc = new("Hello");
   }

Where I wouldn't have to enter MyClass twice (like today), essentially what you use for var inside methods. Please correct me if I misunderstood the scope of this ticket.

Joe4evr commented 6 years ago

Well, that pattern is the primary motivator for this entire proposal to begin with, so I'd be shocked if that didn't work.

HakanL commented 6 years ago

I was hoping that was the case, but I didn't see that pattern in the unit tests so that's why I asked.

alrz commented 6 years ago

@HakanL Ah, I thought you mean object initializers (like new() { Property = e }). type-level initializers should definitely work (edit: just confirmed that it's passing. https://github.com/dotnet/roslyn/commit/127b6ed95a504ef36754bf1f6e7bd07b6a59b5c5)

jcouv commented 6 years ago

@alrz Fantastic! Would you write a small spec (on csharplang) for the feature, for review in LDM? (example) (I set myself a reminder to bring it up at next opportunity). We're going to find/assign someone from the team to be a secondary owner for the feature, who will help with review and open issues.

Discussed with @gafter to try and identify some interesting scenarios. We'd propose that initializers not be supported at first, because they cause a strange order of binding (bind the target-typed-new without initializer, then do overload resolution, then go back and bind the initializer).

Some more scenarios worth testing:

jnm2 commented 6 years ago

Why would you discourage new() for primitive structs if you don't already discourage new int()?

jcouv commented 6 years ago

@jnm2 I'm not convinced that it should be disallowed, but @gafter felt it adds no value. I'll let him elaborate if needed. You can write new Int32() but I've never seen it used. In the case of target-typed new, you could just write 0, which is simpler and clearer. On the other hand, it may not be worth worrying about. Blocking it could be irregular and more work.

alrz commented 6 years ago

this feels like the "default should not be permitted for reference types because you can use null anyways" argument. I'm with others that it wouldn't worth it to restrict new() for primitive structs.

alrz commented 6 years ago

On the other hand, it could cause overload resolution to fail on primitive types,

class C {}
void M(C c) {}
void M(int i) {}

M(new());

This won't compile if we don't restrict primitives, which may or may not be desirable - any restriction on permitted types would have the same effect on the overload resolution - raising the success rate. I'll mention this in the proposed spec for LDM review.

gafter commented 6 years ago

One scenario we should have a test for: this conversion should not be considered for input to a user-defined conversion operator:

class Dog
{
    public Dog() {}
}
class Animal
{
    private Animal() {}
    public static implicit operator Animal(Dog dog) ...
}

class Program
{
    public static void M(Animal a) ...
    public static void Main()
    {
        M(new()); // should fail rather than construct a Dog and convert to Animal
    }
}
HakanL commented 6 years ago

@alrz Thanks for the additional tests and comment, but this is with an empty constructor and an initialization for each field, would it make sense to have one test that is just straight up new(constructor-parameter)? Or would that be automatically covered? I'm not too familiar with how Roslyn works so take my comment with a grain of salt. I'm just eager to start using this new way in a very specific use case I have, so I wanted to make sure that the case with a constructor and parameters will be covered by this ticket, so I can track the progress.

yaakov-h commented 6 years ago

@alrz What happens if try and new() your way into an abstract class or an interface?

Joe4evr commented 6 years ago

@alrz That bit with overload resolution is also what the language team has been concerned with. And the same problem can still manifest with non-primitives regardless of having a restriction for primitive types.

I'm of the opinion that it should just not be allowed to target-type to method parameters; not in the initial version at least. Withholding that part for later consideration will be much better in the long run than shipping it and allowing unsuspecting changes to become source-breaking later.

What happens if try and new() your way into an abstract class or an interface?

Most likely CS0144: Cannot create an instance of the abstract class or interface '{typename}'

alrz commented 6 years ago

@yaakov-h that would be an error. basically we should get all the same errors we get from a regular new plus the ones that indicate the absence of a target-type, etc.

@Joe4evr method arguments and object initializers are actually the motivating use cases for the feature, for instance, initializing a dictionary/list field, or passing XYZOptions objects to methods (mentioned in the original proposal in roslyn repo).

Joe4evr commented 6 years ago

Initializing fields, yes, I agree, but after reading this:

Spooky action at a distance

M(Foo)
M(Goo)

M(new (1))

Goo has a constructor that takes an int. Adding such a constructor to Foo will break the code. Now, adding a constructor is equivalent to adding a conversion in terms of the breaks it can entail.

Being able to have your source broken when one of your dependencies updated and introduced a new overload (which could've been on any level) is falling into a pit of failure, and thus should be avoided outright. I'd say that similar to gafter's comment here: "If a syntax that can be broken at almost no effort is shorter than the syntax that will stay working in the face of the same change, then [the language feature has] failed."

alrz commented 6 years ago

I agree. I'm convinced that allowing this directly in method arguments is not a good idea. I'll wait for LDM to decide if this feature would make it even in a limited set of contexts (because that itself adds a lot of language baggage and inconsistencies, people would expect it to work anywhere). maybe we should invest in more controlled ways of type inference, like new Dictionary() { .. }, etc to be able to use it anywhere but also not breaking things.

gafter commented 6 years ago

@alrz I can tell you that the LDM is unlikely to want to restrict the contexts in which this construct could be used.

With regards to primitive types, my suggestion is that this construct should not work with the default constructor of a value type when the constructor does not appear in metadata. That will prevent new() matching the primitive types and also prevent it matching user-defined struct types. The reason for this restriction is to reduce the accidental ambiguities that arise (e.g. in M(new())) from the zero-initializing default constructor that you cannot get rid of in the API. The "workaround" if you really intended to have a default value of those types is to write default instead of new(). This would have to be LDM-approved.

alrz commented 6 years ago

@gafter My concern is that there would a lot of innocuous changes that could be actually breaking,

For instance,

class Foo { }
class Bar { }

void M(Foo foo) {}

M(new());

The invocation compiles until you add an overload like void M(Bar bar) {}.

yaakov-h commented 6 years ago

Isn’t that just the same problem that “out var” has? I don’t like it, but there is precedent.

jnm2 commented 6 years ago

@yaakov-h Yes.

@alrz Seems to me you're just pushing the problem down one level. This is equally breaking:

class Foo { }
class Bar { }

void M(List<Foo> foo) {}

M(new List());

// Compiles until you add:
void M(List<Bar> foo) {}

Or instead of List<> use MyType<>. Sure it's less often but it's still a thing we have to take into consideration- and a more complicated breaking change rule than with new().

alrz commented 6 years ago

@jnm2 I didn't intend it to be target-typed. It would work the same way implicit array creation works, e.g. the type would be inferred from the collection initializer (or arguments). Note that new() doesn't help here:

void M<T>(List<T> list) {}
M(new() { 1 } ); // T can't be inferred
M(new List() { 1 }) // OK

We could also consider target-type, still, it would be a lot less likely to break compilation.

jnm2 commented 6 years ago

Ah, not target-typed. Thanks!

ghost commented 6 years ago

This syntax confuses me. It is too similar to value tuple syntax.

IanKemp commented 6 years ago

@alrz Any movement on this one? Would love to see this make it into 8.0 or maybe even earlier!

alrz commented 6 years ago

@IanKemp As far as I know, the proposal is scheduled for a future LDM review. no specifics. also, the next release would be 8.0 so don't expect this any sooner :)

jnm2 commented 6 years ago

Would this cover target-typed new[]? For example:

void Foo(Action[] actions) { … }

Foo(new[] { });
Foo(new[]
{
    () => Bar(42),
    Baz
});

int Bar(int x) => x;
void Baz() { … }

(Before someone points out that params is a thing: this should work in more positions than the last argument of a method. Setting a field or out parameter, for example.)

alrz commented 6 years ago

@jnm2 We already have it for fields and locals,

class C {
  Action[] field1 = {};
  Action[] field2 = {() => Bar(42), Baz};   
  void Test() {
    Action[] local1 = {};
    Action[] local2 = {() => Bar(42), Baz};
  }

  static int Bar(int x) => x;
  static void Baz() {}
}

If anything, that could be considered for "list/dictionary literals" (see https://github.com/dotnet/csharplang/issues/414)

jnm2 commented 6 years ago

@jnm2 We already have it for fields and locals,

Only for declaration initializers though, not when e.g. initializing fields in the constructor.

If anything, that could be considered for "list/dictionary literals" (see #414)

I'm not sure the similarity is as strong with #414 as it is with this issue, but you may be right. I see #414 as much more of a departure from where we are today, whereas this proposal is about removing the type name from new expressions when the type can be inferred from the target.

HaloFour commented 6 years ago

@jnm2

I might be missing something but IIRC you can already omit the type name when initializing an array, even if it's a local/field/argument:

public class C {
    private readonly int[] numbers;

    public C() => numbers = new[] { 1, 2, 3 };
}

Is this not what you meant? Do you mean specifically skipping new[] and just using the initializer block?

alrz commented 6 years ago

@jnm2

I'm not sure the similarity is as strong with #414 as it is with this issue,

with target-typed new you could do this

void Method1(Dictionary<string, Action>) {}
void Method2(List<Action>) {}

Method1(new() { { "key", () => {} } })
Method2(new() { () => {} })

414 tries to simplify that further:

Method1(["key": () => {}])
Method2([() => {}])

Maybe we could have it for arrays too:

void M(Action[]) {}

M([() => {}]);

PS: in the first example, a dictionary/list literal may work in case of overloaded members, but with target-typed new, it won't because both List and Dictionary have a parameterless constructor and you'll get an ambiguous call error.