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 "Nullable-enhanced common type" #33

Open
3 of 5 tasks
MadsTorgersen opened this issue Feb 9, 2017 · 43 comments
Open
3 of 5 tasks

Champion "Nullable-enhanced common type" #33

MadsTorgersen opened this issue Feb 9, 2017 · 43 comments

Comments

@MadsTorgersen
Copy link
Contributor

MadsTorgersen commented Feb 9, 2017

Summary

There is a situation in which the current common-type algorithm results are counter-intuitive, and results in the programmer adding what feels like a redundant cast to the code. With this change, an expression such as condition ? 1 : null would result in a value of type int?.

This and #881 should be taken into the language at the same time.

@scottdorman
Copy link
Contributor

To clarify, the example in the Summary condition ? 1 : null would previously need to be written as condition ? 1 : (int?)null.

In the other example, condition ? new Dog() : new Cat(), you state that it "would result in a value of
their common base type Animal." Is there a way to control this?

For example, what if there were an IAnimal interface they both implemented in addition to the base class, and I wanted the result to be of that interface type rather than the abstract Animal type? Could this be influenced by writing this as IAnimal animal = condition ? new Dog() : new Cat()?

@DavidArno
Copy link

What would happen in the following case? Presumably a compiler error as there's ambiguity over which base type to use?

interface IPet {}
interface IMammal {}

class Cat : IPet, IMammal {}
class Dog : IPet, IMammal {}
...
var animal = condition ? new Dog() : new Cat();

Though this could be then solved by doing as @scottdorman suggests:

IPet animal = condition ? new Dog() : new Cat();

@scottdorman
Copy link
Contributor

@DavidArno In your example, there is no common base class so I think a compiler error is reasonable. In that case, you wouldn't be able to use var and would have to explicitly type it to the interface you want.

You could still use var and just cast each piece to the same interface, like

var animal = condition ? (IPet)new Dog() : (IPet)new Cat();

but that defeats the purpose of the proposal.

@alrz
Copy link
Member

alrz commented Feb 10, 2017

In your example, there is no common base class

object?

@gafter
Copy link
Member

gafter commented Feb 10, 2017

@scottdorman

In the other example, condition ? new Dog() : new Cat(), you state that it "would result in a value of
their common base type Animal." Is there a way to control this?

Yes. You can cast either operand to the common type that you want to be the result.

@scottdorman
Copy link
Contributor

@gafter

Yes. You can cast either operand to the common type that you want to be the result.

So, I'd be able to write this as

var animal = condition ? (IAnimal)new Dog() : new Cat()

but not

IAnimal animal = condition ? new Dog() : new Cat()

That still seems like an unnecessary cast. The fact that I'm strongly typing the variable should be enough for the compiler to know what type it should try to cast Dog or Cat to.

@gafter
Copy link
Member

gafter commented Feb 11, 2017

@scottdorman The type of a ?: expression does not depend on the context in which it appears.

@scottdorman
Copy link
Contributor

@gafter Ok. That makes sense. So in my example, the only viable option is to cast one of the operands to the desired type. In that case, it doesn't feel like I've gained any benefit here.

Now, for the first example (condition ? 1 : null), this would let me skip the explicit cast so less typing/clutter in the code, which is good. :)

@alrz
Copy link
Member

alrz commented Feb 12, 2017

This is expected to affect the following aspects of the language:

Probably ?? would be also affected.

var animal = obj as Dog ?? obj as Cat; 

@DavidArno
Copy link

Looking through the spec of the proposal, it will be a real shame here if the common type were just limited to class types. I would have thought that it would also commonly be the case that the two types share a common interface, but the inheritance common type would be object. In such cases, it would be good to have that interface treated as the common type in such expressions.

@scottdorman
Copy link
Contributor

it will be a real shame here if the common type were just limited to class types.

I tend to agree with that statement. It seems to me, as long as there is an unambiguous common type which can be inferred, that type should be used. If that unambiguous common type happens to be an interface, then that's what gets used. If it's ambiguous for any reason (multiple interfaces, etc.) such that the compiler can't clearly decide then it's a compiler error.

@gafter
Copy link
Member

gafter commented Feb 13, 2017

...such that the compiler can't clearly decide...

Whether or not the compiler "can decide" depends on the rules the compiler is supposed to use to decide.

@scottdorman
Copy link
Contributor

Whether or not the compiler "can decide" depends on the rules the compiler is supposed to use to decide.

True, but in the example case where there are two interfaces being implemented:

interface IPet {}
interface IMammal {}

class Cat : IPet, IMammal {}
class Dog : IPet, IMammal {}
...
var animal = condition ? new Dog() : new Cat();

How would the compiler know which interface to use as the common base type? I think in this example, the compiler can't know which one to use and so should raise a compiler error.

However, if we wrote it as

interface IPet {}
interface IMammal {}

class Cat : IPet, IMammal {}
class Dog : IPet, IMammal {}
...
IPet pet = condition ? new Dog() : new Cat();
var animal = condition ? (IPet)new Dog() : new Cat();
IMammal mammal = condition ? new Dog() : new Cat();

Then in the first two instances, the compiler has enough information to know that the common base type is IPet and in the last instance it knows the common base type is IMammal.

@HaloFour
Copy link
Contributor

I honestly think that attempting to determine a common type in those cases should be deferred until after "intersection" types are at least considered. It would be much nicer if the compiler could consider the expression condition ? new Dog() : new Cat() to be of type (IPet & IMammal) than of either specific interface.

@scottdorman
Copy link
Contributor

It would be much nicer if the compiler could consider the expression condition ? new Dog() : new Cat() to be of type (IPet & IMammal) than of either specific interface.

That's an interesting approach, but I think a change like that is a more fundamental change to the type system itself (we can't have a type treated as the intersection of two interfaces right now unless it's done so in the context of an actual base class which implements both of those interfaces).

It also seems like it could introduce some inconsistency as to when the type is derived. If expressions like this cause type resolution to be deferred until after but other expressions don't have that deferred behavior then things can get sticky for the compiler and for us as well as we won't be able to reliably know the "rules" being used.

@HaloFour
Copy link
Contributor

HaloFour commented Feb 13, 2017

@scottdorman

That's an interesting approach, but I think a change like that is a more fundamental change to the type system itself

I don't disagree, but I think it's worth it. The biggest problem with "most common denominator" between types is exactly the problem of being unable to determine which is the most correct type where interfaces are involved. Otherwise disparate types just boil down to object which isn't particularly useful.

we can't have a type treated as the intersection of two interfaces right now unless it's done so in the context of an actual base class which implements both of those interfaces

Or a generic type parameter with two interface constraints.

@alrz
Copy link
Member

alrz commented Feb 16, 2017

compiler could consider the expression condition ? new Dog() : new Cat() to be of type (IPet & IMammal)

In case you want to hold this feature forever, yes, the compiler should totally do that. I don't see that would happen anytime soon, perhaps we should see if it would break when it's done in a future release after this has been implemented.

On the surface it doesn't look like breaking, because it's a "widening" change, an intersection type would always cover the "most specific common type", yet I can't say for sure as it depends on the details for both proposals.

@Thaina
Copy link

Thaina commented Feb 17, 2017

If we approach with most common denominator but in the future we have union type and intersect type would this feature will be changed?

I mean (Cat | Dog) type

Or we would constraint intersection to only interface?

@DavidArno
Copy link

@alrz,

Would having a (IPet & IAnimal) type come out of a ternary expression really take forever to implement? For the expression:

IPet animal = condition ? new Dog() : new Cat();

The compiler can collapse that multiple common type expression down to IPet within the context of the statement. In the case of:

var animal = condition ? new Dog() : new Cat();

Then it would be a compilation error as there is no one type the compiler can choose for animal. The same compiler error could be applied to other cases, such as method group resolution etc.

@gafter gafter added this to the 7.1 candidate milestone Feb 21, 2017
@gafter
Copy link
Member

gafter commented Feb 26, 2017

I am narrowing the spec for this feature to include the handling of c ? 1 : null but it will no longer cover the "common base type" scenario. This is being separated because the former is considered a candidate for 7.1, but the latter is not.

@gulshan
Copy link

gulshan commented Mar 21, 2017

Shouldn't the title be changed to reflect the current narrower scope? It can be something like- "Infer nullability from expression". Or the main "improved common type" discussion can carry on here while a new issue can track the upcoming nullability inference from expression feature.

@alrz
Copy link
Member

alrz commented Apr 10, 2017

Does this affect type inference? for example:

public static T[] ToArray<T>(this (T, T) tuple)
  => new T[] { tuple.Item1, tuple.Item2 };


var array = default((Task<int>,Task<double>)).ToArray(); // should infer T=Task

@gafter
Copy link
Member

gafter commented Apr 10, 2017

@alrz This championed proposal only affects the computation of the common type of a value type and a null literal. Please read the proposal itself.

@gafter gafter changed the title Champion "Improved common type" Champion "Nullable-enhanced common type" Apr 20, 2017
@gafter gafter assigned MadsTorgersen and unassigned gafter Nov 11, 2019
@jcouv jcouv changed the title Champion "Nullable-enhanced common type" Champion "Nullable-enhanced common type" (VS 16.8, .NET 5) Sep 1, 2020
@MadsTorgersen MadsTorgersen modified the milestones: 9.0 candidate, Likely Never Sep 9, 2020
@333fred 333fred changed the title Champion "Nullable-enhanced common type" (VS 16.8, .NET 5) Champion "Nullable-enhanced common type" Sep 9, 2020
@Corey-M
Copy link

Corey-M commented Sep 30, 2020

According to the last few status updates this feature has been rejected. Is that correct? Is there another coming feature that will allow me to compile var x = true ? 1 : null; or the many much more useful variants thereof?

From the outside this appears to be a trivial type resolution issue for a fairly common situation. I don't have any clue what could have caused it to be rejected more than 3 years after it was apparently "approved to proceed with implementation" according to the issue edits. Could we maybe get some insight into why this was rejected?

@333fred
Copy link
Member

333fred commented Oct 1, 2020

Is there another coming feature that will allow me to compile var x = true ? 1 : null;

No, we don't think this particular use case is representative or generally useful.

or the many much more useful variants thereof?

Yes: #2460. Most real examples of this code have a target-type, and in those cases this will work.

Could we maybe get some insight into why this was rejected?

It was rejected because we believe that target-typing various constructs like ternary is the better option that is more generally useful than changing the best common type algorithm. For example, target-typing would work for MethodThatTakesBase(true ? derived1 : derived2), as well as MethodThatTakesNullableInt(true ? 1 : null).

@333fred
Copy link
Member

333fred commented Oct 1, 2020

@Thaina
Copy link

Thaina commented Oct 1, 2020

This is sad. This would be really useful in generic lambda such as Linq Select

@Corey-M
Copy link

Corey-M commented Oct 1, 2020

@333fred

No, we don't think this particular use case is representative or generally useful.

And yet it is a simple exemplar that adequately demonstrates the problem being worked around, and similar to the one that Mads used in the proposal summary.

Yes: #2460. Most real examples of this code have a target-type, and in those cases this will work.

This is vastly disappointing Fred. I know the language design team are doing a lot of really interesting things and target typing is both a useful and complex change that I wouldn't want to interfere with in any way, even if it is of very little utility to me personally.

The disappointing bit is that a feature that was green-lit 3 years ago and could - with narrow enough focus - have been a trivial and beneficial change that slotted comfortably into the existing type resolution logic with essentially zero negative side effects... this has been discarded in favor of a large and complex addition to the language that doesn't solve the common issue that this would have resolved.

The var keyword and the attendant type determination done by the compiler are features that have been tremendously useful to us, especially since they are absolutely required to support such things as anonymous types. And yet by discarding this proposal in favor of target typing the design team are essentially telling us that var is not important to them.

As nice as it is to have an illusion of being able to contribute to the language I use and champion to others, it's jarring to find out that it really is just an illusion. The design team clearly have no interest in my opinion on what might possibly improve my experience with the language. Considering how many staggeringly stupid suggestions they get on a daily basis I guess I can't blame them for not listening. I'll just take my burst bubble and leave you all to it.

@CyrusNajmabadi
Copy link
Member

And yet by discarding this proposal in favor of target typing the design team are essentially telling us that var is not important to them.

There are limited resources (including time). We have prioritized the set of work per release that we feel was appropriate. That inherently means some amount of work will not be included, which inherently means that some set of people will not see the features done they want or see their own priorities align with those of the team. This is inherently true for this problem space, and you'll very likely find yourself on that side of the equation a lot of the time.

If it helps, I'm on the LDM and i don't get to see the things I necessarily want. :)

The design team clearly have no interest in my opinion on what might possibly improve my experience with the language.

Your opinion is valued. But that doesn't mean it is agreed with or that your priorities align with those of the team. As i've stated i am on hte LDM, and we take directions that go against my opinion routinely.

Don't mistake "you are listened to and your opinion is valued" with "your opinion will be followed". :)

@Corey-M
Copy link

Corey-M commented Oct 2, 2020

@CyrusNajmabadi I appreciate what you're saying Cyrus, I know that the LD team is busy as hell and I can only guess at the number of directions you guys are pulled in internally, let alone externally. But let me quote the LDM notes from 19-Apr-2017:

This is a small, isolated change that leads to a nice improvement. Let's do it!

The team agreed to make a "small, isolated change" and acknowledged that there would be "a nice improvement" and then... what? 3+ years later it gets cancelled in favor of a major change that doesn't actually provide that nice improvement. This one is such a small thing that wouldn't even interfere with target typing, and yet 3 years after it was approved that "small, isolated change" went away without any apparent reason other than the LD team had other things on their minds.

I like pattern matching, ranges, string interpolation, etc. I honestly am enjoying the way that most of the big changes are improving the language, and we have the LD team to thank for that. It would be nice if they'd take a little time out occasionally to think about how smashing the little annoyances could be... well, not as important, but certainly a very welcome thing.

And perhaps consider what it looks like to the consumers when you can't get an approved "small, isolated change" done in 3 years, or that an approved change on any scale can be yanked years after approval.

@CyrusNajmabadi
Copy link
Member

and then... what?

Other priorities came up :) For one thing, while the LDM can advise and push in directions, we literally have fixed manpower to implement anything. Roslyn as an engineering team is 100% at capacity (if not beyond that). So things often may not make the cut. It's sad but can def happen. if you think about the product as a living thing just think that these things still may come, just at a later point when both the ldm and engineering teams think it can fit better.

Also, to put it in context, the last 3 years were NRT (and now we have things like Source Generators). These are massively difficult areas that have blown up huge in costs**. We have the best intentions, but there are literal realities behind that fact that we're not superhuman :)

It would be nice if they'd take a little time out occasionally to think about how smashing the little annoyances could be... well, not as important, but certainly a very welcome thing.

We always consider that. Anyone on the LDM can tell you that i'm one of the loudest voices for the 'small but oh so nice' improvements :) You can see many of the features i'm championing to see that that's a prime agenda for me. However, i'm one voice of many, and there are so many concerns and areas we have to put our minds and efforts to.

Finally, not to dig the knife in further, this feature doesn't hit my personal bar for pushing into C# 10. While i've def run into it occasionally and have felt your same frustration and same desire to have it "just work", it's also one i feel is in the NBD category. So trust me when i say that i understand where you're coming from. But, at teh same time, when we ourselves give our own fair and honest opinions on these things, they may really not align with yours. That's just how it goes sometimes :-/

--

** I can't really even express how costly NRT ended up being. I don't think it would be an exaggeration to say that it may have been 10x over what we had initially thought and costed out. NRT still isn't done afaict either. There's both a lot of IDE work, compiler issues, and future language work in this space. But the value, IMO, it huge and this was worth it. But if you consider the engineering side of this, I'm guessing how you could see how such an impactful situation could easily derail lots of best intentions around many other orbiting features :)

Anyways, i hope this helps. :)

@Corey-M
Copy link

Corey-M commented Oct 2, 2020

Roslyn as an engineering team is 100% at capacity (if not beyond that).

If only there were a community of interested people who could help out with that workload somehow.

To quote from the LDM notes for 09-Sep-2020:

This is a good candidate for community contribution

And:

A community member is contributing this feature

So yeah, I know you're all busy. And NRT was always going to be huge. The only simple solution is to net support nulls in the first place, but that ship sailed 20 years ago. The blowout in the engineering budget does go a long way towards explaining why little things got swept aside though. Or why Top Level Programs made the grade when I can't see any substantive advantage in them, no matter how hard I screw up my face and try.

But back to something actually relevant to this thread...

Now that this proposal has been rejected is there any chance at all that the LD team would accept a new proposal for the simplest form? Or will it get rejected (as so many things seem to) with the standard "duplicate", "already rejected", etc. Would it help if I learned Roslyn and figured out how to implement the feature first? Or am I just flogging a deceased equine here?

@HaloFour
Copy link
Contributor

HaloFour commented Oct 5, 2020

I have to agree with the dissenters here. Requiring a target type all but eliminates the usefulness of this feature for me. Any team I've been on has had pretty strict guidelines about where not to use ternary ops due to how easily they they obfuscate the conditional code. Of the few cases I would consider a ternary op would be in a variable assignment or as the body of a lambda. In neither case would I expect to have to declare the target type. I have to question this claim that most "real examples" have a target type.

@CyrusNajmabadi
Copy link
Member

I'll bring to next triage to reassess if we would target just this specific scenario.

@alrz
Copy link
Member

alrz commented Oct 5, 2020

I think the rationale was that we never synthesize types during type inference, we only choose one that already exist (whether the target type or of either of branches). But this is actually a useful feature worth reconsidering since it'll be probably a breaking change to add after 9.0 due to target-typing inplace.

If this was added, would it affect switch expressions as well? var x = 5 switch { 5 => 1, _ => null }; // error (though that may have the same issue with target-typing, since switch is already doing that).

@333fred
Copy link
Member

333fred commented Oct 5, 2020

But this is actually a useful feature worth reconsidering since it'll be probably a breaking change to add after 9.0 due to target-typing inplace.

No matter what the outcome is, we cannot take it for C# 9, it's far too close to release. Now, when we were discussing this in LDM last we came up with a way that we could do this without a breaking change by adding a new "type" stage. We'd have

  1. Natural type
  2. Converted type
  3. "Best"? type (name unknown)

The complexity of this last stage is what caused to explicity reject, as opposed to retriaging for a future milestone. That being said, I'm sympathetic to the arguments brought up here, and since we have more triage set for the next couple of LDM meetings I agree with @CyrusNajmabadi that it's worth bringing up again.

@CyrusNajmabadi
Copy link
Member

Have moved back to 10.0 candidate so we can triage this again.

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

No branches or pull requests