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

Surprising System.ValueTuple requirement for deconstruction #18629

Closed
jskeet opened this issue Apr 12, 2017 · 15 comments
Closed

Surprising System.ValueTuple requirement for deconstruction #18629

jskeet opened this issue Apr 12, 2017 · 15 comments
Assignees
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers Feature Request New Language Feature - Tuples Tuples Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Milestone

Comments

@jskeet
Copy link

jskeet commented Apr 12, 2017

Version Used: 2.1.0.61520

Steps to Reproduce:

Try to compile

using System;

static class Test
{
    static void Deconstruct(this DateTime date,
                            out int year, out int month, out int day)
    {
        year = date.Year;
        month = date.Month;
        day = date.Day;
    }
    
    static void Main()
    {
        (int year, int month, int day) = DateTime.UtcNow;
        Console.WriteLine(year);
        Console.WriteLine(month);
        Console.WriteLine(day);
    }
}

without a reference to System.ValueTuple.dll

Expected Behavior:

Compilation with no errors. No tuple literals or tuple types are involved.

Actual Behavior:

Error:

Test.cs(16,9): error CS8179: Predefined type 'System.ValueTuple`3' is not defined or imported

When compiling with a reference to System.ValueTuple.dll, the resulting IL has no reference to System.ValueTuple.dll, so why is it required at compile-time? This is very surprising.

There mention of this in #11299:

I assume deconstruction-assignment should work even if System.ValueTuple is not present. (answer: that is no longer the case, ValueTuple is required)

I haven't found any mention of that in the LDM meetings.

  • Is this likely to be permanent?
  • Any chance of an explanation of a seemingly-arbitrary requirement?
@jskeet jskeet changed the title Surprising ValueTuple requirement Surprising System.ValueTuple requirement for deconstruction Apr 12, 2017
@jaredpar
Copy link
Member

CC @jcouv

@jskeet
Copy link
Author

jskeet commented Apr 13, 2017

(Do let me know if filing this sort of strange picky issue isn't useful, btw. When writing, I tend to explore all kinds of corner cases. This one probably isn't as corner-case-like as most, admittedly - but some really will be. If it's useful to see what surprises more investigative users, that's great - but I certainly don't want to cause churn to no-one's benefit.)

@jcouv
Copy link
Member

jcouv commented Apr 13, 2017

@jskeet No problem with filing issues that may be questions. Please tag me on tuples and deconstruction questions for a faster triage.

The compiler doesn't emit any ValueTuple unless the return value of the deconstruction is used (for example, ((x, y) = (1, 2)).Item1).
Still, you can "see" tuples in the deconstruction. Technically, the left-hand-side in your example, (int year, int month, int day), is a tuple of three declaration expressions, used in an assignment. This changed just before RTM (it used to be a deconstruction-declaration statement syntax, but we felt that declaration expressions would be more general for the future).
So we always require ValueTuple, because we don't know at the time of checking if it will be used. It's probably possible to relax this, but I don't plan to.

The requirement of ValueTuple even when the returned tuple is not used/emitted was discussed with LDM. I'm not sure if it was explicitly recorded. I did record it in the feature notes (which may serve you for inspiration/quirks): #11299

I assume deconstruction-assignment should work even if System.ValueTuple is not present. (answer: that is no longer the case, ValueTuple is required)

@jcouv jcouv self-assigned this Apr 13, 2017
@gafter gafter added Question Resolution-Answered The question has been answered labels Apr 13, 2017
@gafter gafter closed this as completed Apr 13, 2017
@jskeet
Copy link
Author

jskeet commented Apr 14, 2017

If this ever does come up for discussion again, I'd certainly like to urge the team to consider it as a useful change. It probably doesn't matter too much for application authors, but I spend all my time writing libraries - I try to keep a wide range of targets (e.g. down to netstandard 1.3) and I avoid adding dependencies as far as possible. At the moment, I can add Deconstruct methods for clients to use if they want, but the language feature will be unavailable to me :(

@jcouv jcouv reopened this Apr 14, 2017
@jcouv jcouv added Feature Request and removed Resolution-Answered The question has been answered Question labels Apr 14, 2017
@jcouv jcouv added this to the Unknown milestone Apr 14, 2017
@jcouv jcouv removed their assignment Apr 14, 2017
@jcouv
Copy link
Member

jcouv commented Apr 14, 2017

Re-opening the issue to track the request.
I think this is not a trivial change, as the binding for deconstruction prepares as tuple symbol (which checks the existence of ValueTuple types to use as underlying types).

@AArnott
Copy link
Contributor

AArnott commented Jun 7, 2017

I hit this too. I was using deconstruction in my Roslyn analyzer library (Microsoft/vs-threading) which still supports VS2015. I was disappointed to see that I have to use System.ValueTuple, which isn't supported on VS2015's version of Roslyn. So I changed all my ValueTuple use to Tuple and wrote my own Deconstruct extension methods, and the compilation fails because of the missing reference. Yet when I give it the reference, the resulting DLL doesn't retain that reference. So it's obviously not really needed, yet the compiler requiring it as input means that my project is super-fragile in that if anyone accidentally takes a real dependency on ValueTuple, the compiler will be happy, but VS2015 scenarios will break.

AArnott added a commit to microsoft/vs-threading that referenced this issue Jun 7, 2017
Due to dotnet/roslyn#18629, we have to reference it to compile, but it compiles away provided we never actually use it.
@jcouv
Copy link
Member

jcouv commented Jun 8, 2017

System.ValueTuple, which isn't supported on VS2015's version of Roslyn

@AArnott I didn't understand.
Roslyn 1.x (corresponding to Visual Studio 2015 and its updates) supports neither tuple, nor deconstruction.
System.ValueTuple is just a type and it can be used in C# 6 and older targets.

@AArnott
Copy link
Contributor

AArnott commented Jun 8, 2017

@jcouv: Roslyn 1.x does not guarantee that the System.ValueTuple assembly is around for an analyzer to load. Therefore, one cannot write an analyzer that both works with Roslyn 1.x and uses ValueTuple as an implementation detail of the analyzer. The unfortunate thing is that ValueTuple has nothing to do with deconstruction itself, and I should be able to write my own deconstructible types or deconstructing extension methods without a dependency on the System.ValueTuple assembly. But the compiler seems to have an extraneous dependency during build on it even if I'm not using it.

I build my analyzer with Roslyn 2.x, so I'm allowed to use deconstruction within it. And in fact my current workaround is to go ahead and reference System.ValueTuple during compilation of my analyzer, and then I have a unit test to ensure that the assembly reference is not retained in the compiled assembly so that it still works in Roslyn 1.x environments.

@jcouv
Copy link
Member

jcouv commented Jun 8, 2017

@AArnott Thanks for the clarification. Makes sense.

@jcouv jcouv modified the milestones: 15.6, Unknown Jun 8, 2017
@jcouv jcouv self-assigned this Jun 8, 2017
@jcouv
Copy link
Member

jcouv commented Jun 13, 2017

📝 From discussion with @VSadov, I'm exploring how to bind the deconstruction to have a void type (instead of a tuple type) when the return value of the deconstruction expression isn't used. The binding for conditional access does something of that sort (inspecting parent syntax to decide if the result is used).

@gafter
Copy link
Member

gafter commented Jun 14, 2017

@jcouv would the semantic model still follow the language spec and give a tuple type?

@jcouv jcouv added the 4 - In Review A fix for the issue is submitted for review. label Jun 17, 2017
@jcouv
Copy link
Member

jcouv commented Jun 17, 2017

Fix is in-progress (#20295). With that change, the returned tuple value is recognized as unused in an expression-statement and some positions of a for-statement.

@gafter Yes, the semantic model will still return the proper type in all cases. The only caveat is that the tuple type for the expression may have an error type as its underlying type instead of a proper ValueTuple (when it was missing).

@jcouv jcouv added the Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented label Jun 29, 2017
@jcouv
Copy link
Member

jcouv commented Jun 29, 2017

Fixed by #20295 (in C# 7.2 and Visual Studio 2017 version 15.5).

@jskeet
Copy link
Author

jskeet commented Jun 29, 2017

Now I get to moan and say this has given me extra work to write about it in C# in Depth ;)

Seriously though - many thanks!

@VSadov
Copy link
Member

VSadov commented Jun 29, 2017

Nice!!

Now we should be able to deconstruct into spans.
Need to add a test for that when the change makes into the feature branch.

Specifically, spans are stack-only types and as such cannot be used to construct arrays/generics/tuples, etc...

The following should work:

Span<int> s;
(s, s) = SomethingDeconstructableIntoTwoSpans();

The following should not work though:

Span<int> s;
var wouldHaveForbiddenType = (s, s) = SomethingDeconstructableIntoTwoSpans();

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 Feature Request New Language Feature - Tuples Tuples Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Projects
No open projects
Development

No branches or pull requests

7 participants