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

Infer types from generic constraints #6644

Conversation

HellBrick
Copy link

This is a prototype of #5023. See #5023 (comment) for the description of the implementation mechanics.

@dnfclas
Copy link

dnfclas commented Nov 8, 2015

Hi @HellBrick, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

@dnfclas
Copy link

dnfclas commented Nov 8, 2015

@HellBrick, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

@bbarry
Copy link

bbarry commented Nov 8, 2015

It looks like these tests are failing exactly for the reason the code was changed. The expected refactorings no longer need the type parameters on these method calls with the change here:

in EditorFeatures/CSharpTest/ExtractMethod/ExtractMethodTests.cs

Edit: also the comments in the MethodTypeInference.cs file should be updated to match spec changes.

@HellBrick
Copy link
Author

Thanks, I've fixed the tests.

Some of them were failing due to the fact that explicit generic parameters are no longer needed in these cases.
@HellBrick HellBrick force-pushed the infer-types-from-generic-constraints/#5023 branch from 9323e31 to b5c4c9d Compare November 8, 2015 15:03
@HellBrick
Copy link
Author

Edit: also the comments in the MethodTypeInference.cs file should be updated to match spec changes.

That's a bit tricky, since there is no updated spec at this point =)

@SolalPirelli
Copy link

I see that your tests use methods with one argument; would this also work with parameterless methods, e.g. M<T, X>() where T : IEnumerable<X> called as M<List<int>>()? (sorry, my computer is in a bit of a messy state right now, can't check for myself)

@bbarry
Copy link

bbarry commented Nov 9, 2015

No. This change is about getting rid of some (all?) cases where the generic parameters are already fixed (you might be able to tighten them further) but the compiler fails to tell so the developer must supply them.

@khellang
Copy link
Member

khellang commented Nov 9, 2015

Will this solve this problem (inferred return type) as well?

EDIT: Seems like it does. Pulled down the code and took it for a test drive. Nice work.

@davkean
Copy link
Member

davkean commented Nov 10, 2015

tag @dotnet/roslyn-compiler

@HellBrick
Copy link
Author

@SolalPirelli It doesn't work like that. This PR improves the type inferrer, but in your case it's never actually invoked. Waaaaaay earlier down the stack the compiler detects that method is called with less type arguments than required and produces an error. If this behavior is altered to try inferring the missing types before failing, then the mechanism from the current PR would kick in and save the day. But I believe it's a separate issue (#6207 looks related).

@gafter
Copy link
Member

gafter commented Nov 16, 2015

@HellBrick We'll take a look at this, thanks!

@davkean davkean added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Nov 18, 2015
@davkean
Copy link
Member

davkean commented Nov 20, 2015

test eta please

tag @dotnet/roslyn-compiler

@gafter
Copy link
Member

gafter commented Nov 20, 2015

@HellBrick Nice work! We're still discussing it (but mostly we have vacations that prevent us from meeting much until January).

@gafter gafter self-assigned this Nov 20, 2015
@gafter gafter modified the milestones: 1.3, 1.2 Nov 20, 2015
@gafter gafter added the 4 - In Review A fix for the issue is submitted for review. label Nov 20, 2015
@davkean
Copy link
Member

davkean commented Dec 9, 2015

@gafter @dotnet/roslyn-compiler It's been nearly ~3 weeks, can we take a look?

@HellBrick
Copy link
Author

mostly we have vacations that prevent us from meeting much until January

It's January now =) Any plans for further action here?

@gafter
Copy link
Member

gafter commented Jan 1, 2016

Our plans are to meet around the third week of January and start prioritizing our language design work.

@gafter
Copy link
Member

gafter commented Jan 7, 2016

@HellBrick OK, the LDM looked at this very briefly, and we decided that we don't want to make language changes to overload resolution in 1.2 (except to fix compat bugs). We can consider it for a later language version (i.e. version 2.0, tentatively called C# 7). And I am still interested in pushing it through the process.

Can you please retarget this to the future branch (i.e. close this PR and open a new PR against the future branch]?

Tag me on the new PR and I'll continue working it with the language team.

/cc @MadsTorgersen

@HellBrick
Copy link
Author

Sure, I'll open a new PR tomorrow. It's a pity though that such a small nice improvement has to wait until the next major language version. If you don't mind me asking, what's the reason behind postponing it? I'm just trying to get a better understanding of your process and to see if there's anything else that can be done here to simplify acceptance of this feature.

P.S. Out of curiosity: what are the minor milestones (1.2, 1.3) mapped to, timeline- or release-wise? Do they correspond to the upcoming VS 2015 updates?

@HellBrick HellBrick closed this Jan 7, 2016
@HellBrick HellBrick deleted the infer-types-from-generic-constraints/#5023 branch January 8, 2016 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers Area-Language Design Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants