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

Infer types from constraints #7850

Closed
wants to merge 4 commits into
base: future
from

Conversation

Projects
None yet
8 participants
@HellBrick

HellBrick commented Jan 8, 2016

This is a prototype of #5023 rebased on top of the future branch. See #5023 (comment) for the description of the implementation mechanics and #6644 for the original PR against master.

/cc @gafter

HellBrick added some commits Nov 8, 2015

@davkean

This comment has been minimized.

Member

davkean commented Jan 8, 2016

Closing and reopening, so that merge branch gets updated.

@davkean davkean closed this Jan 8, 2016

@davkean davkean reopened this Jan 8, 2016

@davkean

This comment has been minimized.

Member

davkean commented Jan 8, 2016

@dotnet-bot retest this please
@dotnet-bot test eta please

@davkean

This comment has been minimized.

Member

davkean commented Jan 8, 2016

What's the plans with this PR? You mention this is a prototype, are you looking for feedback? Or is this just to get some coverage for tests?

@amcasey

This comment has been minimized.

Member

amcasey commented Jan 8, 2016

The test failures appear to due to some issues that have already been resolved in master. They should clear up as soon as we've merged master into future.

@davkean

This comment has been minimized.

Member

davkean commented Jan 8, 2016

Ignore these failures for the moment, looks like future is broken.

@HellBrick

This comment has been minimized.

HellBrick commented Jan 8, 2016

To be honest, I have no clue about what comes next for this PR. This is my shot at an up-for-grabs issue that I care about: I just came up with an idea on how to do it and implemented it. The tests have been taken care of in the previous incarnation of this PR (against the master branch), the feature seems to be working fine and it's designed in the most non-invasive way I could think of to minimize the chance of screwing other things up.

However, there wasn't a proper discussion of the implementation neither before nor after the PR, so I have no idea if it's good or bad or anything in between. There was this comment, but I'm not sure what LDM is, whether it covered the details of this PR and what to do next. So yes, some clarity and/or feedback would be nice =)

@gafter

This comment has been minimized.

Member

gafter commented Jan 8, 2016

"LDM" is the (members of the) language design meeting.

We just looked at the proposal. It appears to be a breaking language change (i.e. will change the behavior of existing code), and there doesn't appear to be any nice way to fix that.

Here is an example of how the break arises. Given the two methods

void M(object) {}
void M<T, U>(T t) where T : IEnumerable<U> {}

and existing invocation

M("foo");

the previous version of the language (without this change) would call the former method. With the new rules in place, it would call the latter method.

Our customers tar and feather us when we take breaking changes, so this, unfortunately, might kill the idea of this feature.

Having said that, don't lose all hope. We (the LDM) are sad that we cannot take any breaking changes, even one such as this that seems like an improvement. We're wondering if it will be possible for us to make breaking changes if we also ship a tool that updates your code so that it has the same meaning as before. Having a mechanism for such a tool to exist would have made it easier for us to do many things in the past, and would have enabled us to avoid language-design contortions, for example due to new keywords (such as var, nameof and async) being identifiers in the previous version.

/cc @MadsTorgersen

@amcasey

This comment has been minimized.

Member

amcasey commented Jan 8, 2016

The pull request that should fix Future is #7853.

@HellBrick

This comment has been minimized.

HellBrick commented Jan 8, 2016

Ah, now I see it's way trickier than I expected =) Thanks for the explanation.

I perfectly understand how the backwards compatibility can sometimes be a burden, so I do hope that some story for mitigating breaking changes will be worked out eventually. Is there a public issue that tracks this idea? It would be a very interesting thing to follow.

@gafter gafter added this to the 2.0 milestone Jan 9, 2016

@gafter gafter added the Blocked label Jan 9, 2016

@BreyerW

This comment has been minimized.

BreyerW commented Jan 9, 2016

As for breaking change - i wonder couldn't it be reduced by adding small constraint like that you have to say that you want to infer generic version. I think better to show example:

Instead of calling M("foo"); you have to write smth like M<,>("foo");. Just that you have to add <,> to tell compiler that you want to infer version with two generics. Or if we want to be more generic then <> or <var> would be enough to tell that we want best matched generic version with any number of generics

In this case calling M("foo"); still mean calling void M(object) {}

The problem is - will this constraint kill/greatly reduce usefulness of feature or not?

EDIT: Deleted line about standard resolution overload because i reminded that in c# situation with overloaded generic methods with same number of generics is unlikely (due to fact that generic constraint arent part of method signature)

@SergeyZhurikhin

This comment has been minimized.

SergeyZhurikhin commented Jan 10, 2016

@gafter I accept, the call M("foo") as M<string, char>("foo").

@davkean

This comment has been minimized.

Member

davkean commented Jan 13, 2016

(Closing/opening to sync update the merge branch to pick up the build break we just fixed)

@davkean davkean closed this Jan 13, 2016

@davkean davkean reopened this Jan 13, 2016

@davkean

This comment has been minimized.

Member

davkean commented Jan 13, 2016

@dotnet-bot retest this please

@khellang khellang referenced this pull request Jan 18, 2016

Merged

Added caching #58

@gafter gafter modified the milestones: 2.0 (RC), 2.0 (Preview) Mar 7, 2016

@gafter

This comment has been minimized.

Member

gafter commented Apr 28, 2016

There is no way for us to accept this change until we have a way of handling breaking changes, which will not be soon. So I'm going to close this PR. However, we are leaving it around for reference in case we can take it in the future.

@HellBrick Thanks for your contribution!

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