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

Invalid metadata emitted for a System.ValueTuple type #43595

Closed
AlekseyTs opened this issue Apr 23, 2020 · 6 comments · Fixed by #44231
Closed

Invalid metadata emitted for a System.ValueTuple type #43595

AlekseyTs opened this issue Apr 23, 2020 · 6 comments · Fixed by #44231

Comments

@AlekseyTs
Copy link
Contributor

namespace System
{
    public struct ValueTuple<T1, T2>
    {
    }
}

Observed:

.class public sequential ansi sealed beforefieldinit System.ValueTuple`2<T1,T2>
       extends [mscorlib]System.ValueType
{
  .field public !T1 Item1
  .custom instance void [mscorlib]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = ( 01 00 00 00 ) 
  .field public !T2 Item2
  .custom instance void [mscorlib]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = ( 01 00 00 00 ) 
} // end of class System.ValueTuple`2

Note, declaration of two public fields that appeared from nowhere.

@AlekseyTs
Copy link
Contributor Author

cc @jcouv

@jcouv
Copy link
Member

jcouv commented Apr 23, 2020

That one is expected and covered by existing tests. The type is recognized as a tuple and therefore has tuple elements.

@jcouv jcouv added the Resolution-By Design The behavior reported in the issue matches the current design label Apr 23, 2020
@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Apr 24, 2020

That one is expected

I don't think so. The fields are not declared and shouldn't appear out of thin air.

and covered by existing tests.

Doesn't make the behavior correct.

@AlekseyTs
Copy link
Contributor Author

That certainly wasn't a behavior prior to #39370 and the Symbol Model change was not supposed to change behavior. Also, the fact that it not By Design can be observed by compiling the following code:

namespace System
{
    public struct ValueTuple<T1, T2>
    {
	
    }
}

class Program
{
    static void Main()
    {
        _ = (1, 2).Item1;
        _ = (1, 2).Item2;
    }
}

Observed:

(13,20): error CS8128: Member 'Item1' was not found on type '(T1, T2)' from assembly '?, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.
(14,20): error CS8128: Member 'Item2' was not found on type '(T1, T2)' from assembly '?, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.

As we can see, the fields do not exist, even compiler thinks that. And that is expected and always worked this way.

@AlekseyTs AlekseyTs added Regression and removed Resolution-By Design The behavior reported in the issue matches the current design labels Apr 24, 2020
@jcouv
Copy link
Member

jcouv commented Apr 24, 2020

I understand it was not the prior behavior, because then ValueTuple was the underlying type of tuples, but not a tuple itself. That has intentionally changed.

@AlekseyTs
Copy link
Contributor Author

I understand it was not the prior behavior, because then ValueTuple was the underlying type of tuples, but not a tuple itself. That has intentionally changed.

We are talking about undeclared fields not about our view on types themselves. One has nothing to do with another and fields just aren't becoming declared because of that. I agree the behavior is a fall out of the implementation work with intent to change how we represent tuple types, but we didn't want that kind of change in behavior, it is not unavoidable (even with the new view on the types) and this is just an oversight.

I think this is a bad change, obviously inconsistent with overall compiler's behavior around missing declarations in tuples. We are emitting error fields. The symbols are TupleErrorFieldSymbols. They have a use-site error associated with them. They are created for the purpose of reporting the errors. That is the intent behind them and I see no indication of intent to emit them. The current behavior simply doesn't connect with other pieces of the "puzzle". I also do not remember any discussions around that change.
In my opinion, this is an oversight and, if indeed there is an existing test where we can observe the current behavior, this is a double oversight. We should fix this ASAP.

@jcouv jcouv self-assigned this Apr 24, 2020
@jcouv jcouv added this to the 16.7 milestone Apr 24, 2020
@jcouv jcouv modified the milestones: 16.7, 16.8 Jul 1, 2020
@jcouv jcouv added the 4 - In Review A fix for the issue is submitted for review. label Jul 7, 2020
@jcouv jcouv modified the milestones: 16.8, 16.9 Oct 6, 2020
@jcouv jcouv modified the milestones: 16.9, 16.10 Jan 27, 2021
@jcouv jcouv removed the 4 - In Review A fix for the issue is submitted for review. label Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants