dotnet / csharplang

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

'var' instead of a method return type #1484

Closed antrv closed 6 years ago

antrv commented 6 years ago

Allow this construction

public var GetDictionary()
{
      return new Dictionary<string, (int, int)>();
}

instead of this

public Dictionary<string, (int, int)> GetDictionary()
{
      return new Dictionary<string, (int, int)>();
}

The rules of type inference with the case of multiple return statements can be the same as for creating array for example.

svick commented 6 years ago

Related: var return for local functions https://github.com/dotnet/csharplang/issues/943.

jnm2 commented 6 years ago

This would make it harder to notice when you introduce breaking changes to a public API since the public API would be determined by an implementation detail of the method (returning a more or less specific or implicitly convertible type). We need the practice of locking in the API spec in the signature.

If this was ever added, I hope it would ship with warnings or errors out of the box when used in public signatures.

Instead of Dictionary<string, (int, int)>

For a public method, this looks like a good reason to use IReadOnlyDictionary<string, NewDomainType>.

sharwell commented 6 years ago

Currently it's possible to bind everything in a compilation unit using only the information in that compilation unit plus the signatures of everything in other compilation units. Exposing an inferred type across compilation unit boundaries has the potential to tremendously degrade overall performance.

I lean against requests like this at least until we reach a point of high assurance that the developer experience will not be negatively impacted by attempting to provide the feature.

This would make it harder to notice when you introduce breaking changes to a public API since the public API would be determined by an implementation detail of the method (returning a more or less specific or implicitly convertible type). We need the practice of locking in the API spec in the signature.

This is only a concern for a subset of development situations, and possibly even a minority. Providing a stable public API incurs a non-trivial cost to the developer, and this cost should be pay-to-play (i.e. developers not attempting to provide a stable API should not be forced to deal with that overhead).

Also note that tools like Public API Analyzer can be used to reveal the true signatures in situations like this without forcing the developer to write it. If the signature is expected in code, an analyzer targeting that style could be easily applied.

HaloFour commented 6 years ago

I'm with @jnm2, the signature is the most important part of a method, so much more so than the actual implementation. Depending on the logic the compiler would have to use in order to derive that return type you could accidentally shift the signature of an API thus breaking all consumers.

I think that target-typed new expressions would largely alleviate the concerns around verbosity as you don't have to repeat yourself in the signature and in the creation of the return value:

public Dictionary<string, (int, int)> GetDictionary()
{
      return new();
}
jnm2 commented 6 years ago

@sharwell

This is only a concern for a subset of development situations, and possibly even a minority. Providing a stable public API incurs a non-trivial cost to the developer, and this cost should be pay-to-play (i.e. developers not attempting to provide a stable API should not be forced to deal with that overhead).

This is a really good point. But even within codebases which don't need to maintain a stable API: to the extent which it feels painful to put Dictionary<string, (int, int)> in a signature, it's a good thing that it feels painful. The pain pushes you toward a pit of success.

DavidArno commented 6 years ago

@jnm2,

This would make it harder to notice when you introduce breaking changes to a public API...

Good point. In that case my vote remains for #943, but I'm retracting my upvote here.

theunrepentantgeek commented 6 years ago

This is only a concern for a subset of development situations, and possibly even a minority.

I disagree. Every non-trivial application I've worked on has been segmented into multiple layers where each layer provides a public API consumed by the layer above. (I'm thinking of systems as small as 2 kloc and as large as 1000 kloc and everywhere in between.)

Even when the API is never consumed outside of one specific application, stability is important - having part of a persistence layer API spontaneously change from List<TimeSeries> to IList<TimeSeries> would be a serious breaking change.

CyrusNajmabadi commented 6 years ago

Note: i worked on such a system with TypeScript, and it's worth pointing out that we tried very hard to make this fast and efficient. However, it turned out a very large perf issue which did impact IDE scalability. Indeed, we ended up having to add a "do not try to analyze all the source code" option in order to deal with this because it can just be so expensive.

Consider this code today:

foo.Bar()<dot>

Today, i just need to find the right 'Bar' on 'Foo' and ask it what it returns. Once i find Bar that answer is trivial. In a system where you have inferred reteturn types, this answer is unbounded. the answer is: "you need to analyze Bar's body'. And that means analyzing everything that 'Bar' calls that also has inferred return types, and so and and so forth. When people use this feature extensively (and many people do), you can literally have to analyze every method in order to answer these questions.

Also, a single edit tears all this analysis down.

Today, in completion, we can say "oh, you just edited a method body. that's ok, we can reuse the entire rest of the symbolic world and only renalyze this method". This is, obviously, a massive speedup for the most common types of edits that are happening, and it's a critical part of providing fast completion. That's not true with inferred return types. Once you've edited your method that means all methods that called you now must be reanalyzed (and all methods that called those methods, and so on and so forth). Now, completion may take hugely longer, which has all sorts of consequences for things like muscle memory.

--

So, from a user-happiness perspective, i love this feature. It's great. For an engineering standpoint it cannot be underestimated how costly this is and how this single feature can dramatically decimate all the perf/engineering decisions you've made so far.

CyrusNajmabadi commented 6 years ago

Also, while i can't speak to it fully myself, i think inferred return types has a deep implication for the 'passes' and 'ordering' the compiler depends on when analyzing code in an incremental fashion today. i.e. the compiler needs to be albe to determine things like where a method sits in a method hierarchy. However, in order to do that it needs the signature of hte method. But in order to get the signature it needs the body. But in order to bind the body it needs the inheritance hierarchy.

Now you have a circularity that is deeply problematic. I think @gafter can speak to this issue with greater depth.

bondsbw commented 6 years ago

Agreed with @jnm2 about the API risk and with @svick / @DavidArno that local functions make more sense.

I might also entertain private var and internal var methods. (Performance issues aside.)

gulshan commented 6 years ago

I think my proposal #265 was the narrowest in the scope in this regard of type inference.

gafter commented 6 years ago

For the reasons discussed here, and also due to possible difficulties inferring the type when there are mutually recursive methods, we are never going to do this.

ymassad commented 6 years ago

@gafter , is this feature never going to be implemented even for private methods? Or is this a different proposal?

If return type inference is to be implemented for local functions, why not do it also for private methods? Is there any difference?

gafter commented 6 years ago

We have no plans to implement return type inference for local functions or private methods.