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

INamedTypeSymbol.Arity incorrectly reports zero for ValueTuple<int, int> in VB #20648

Open
gafter opened this issue Jul 5, 2017 · 6 comments · Fixed by #39370
Open

INamedTypeSymbol.Arity incorrectly reports zero for ValueTuple<int, int> in VB #20648

gafter opened this issue Jul 5, 2017 · 6 comments · Fixed by #39370
Labels
Area-Compilers Bug Concept-API This issue involves adding, removing, clarification, or modification of an API. Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality Language-VB New Language Feature - Tuples Tuples
Milestone

Comments

@gafter
Copy link
Member

gafter commented Jul 5, 2017

The compiler APIs incorrectly report that the type ValueTuple<int, int> is nongeneric (has no type arguments or type parameters).

@gafter gafter added Area-Compilers Bug Concept-API This issue involves adding, removing, clarification, or modification of an API. labels Jul 5, 2017
@gafter gafter added this to the 15.5 milestone Jul 5, 2017
@gafter gafter self-assigned this Jul 5, 2017
gafter added a commit to gafter/roslyn that referenced this issue Jul 6, 2017
@gafter
Copy link
Member Author

gafter commented Jul 13, 2017

At one time the implementation of tuples recognized that tuples and their underlying types are distinct types. Later, the LDM decided that these should not be distinct types. We (mostly) modified the compiler’s language behavior to respect the LDM decision, but we have not modified the APIs accordingly.

Our compiler guidelines include this design point:

As a rule, the public APIs exposed by Roslyn should reflect the language view of the program, not the lowered or implementation view. For example, a Symbol's Name property should reflect the name from the language point of view if there is one; we have a separate MetadataName (internal) property to represent the implementation view.

In order to align our APIs with the language view for tuples, I propose to make the following changes:

  • The NamedTypeSymbol APIs are changed as follows:
    • IsTupleType: Any type that is “tuple-compatible” shall be considered a tuple type (including original types such as ValueTuple<T1, T2>), in which case this API returns true.
    • Construct: A constructed type that is a tuple type (e.g., the construction of ValueTuple<T1,T2> with the types {Int32, Int32}) shall be considered a tuple type. This currently works in the language, but not in the APIs. The APIs will be fixed so that NamedTypeSymbol.Construct returns tuple types when appropriate.
    • TupleUnderlyingType: for a tuple type, it shall return a corresponding tuple type with no user-added names (at the top level). For example, given the type (int a, int b), its underlying type is (int, int), also known as System.ValueTuple<int, int>. For non-tuple types, it behaves as before.
    • Arity: Tuple types with fewer than 8 elements have the same arity as the number of elements. For example, (int, int) as arity 2. Tuple types with 8 or more elements have arity 8. This is a straightforward consequence of a tuple type without named elements having itself as its underlying type.
    • TypeArguments: This is now the same as the type arguments of the tuple’s underlying type (i.e., its own type arguments for unnamed tuple types).
    • ConstructedFrom: taken from the underlying type
    • OriginalDefinition: taken from the underlying type
    • Locations: taken from the underlying type.
    • TypeSubstitution and other internal APIs: taken from the underlying type
  • Instances of ValueTuple<T> (length one tuples) are considered a tuple type (even though there is no surface syntax for them in the language)
  • FieldSymbol APIs are changed as follows:
    • OriginalDefinition: For fields originating in the underlying type, that underlying field. For compiler-synthesized fields of long tuples, itself. For user-named elements, the same as today.
    • Locations: For fields originating in the underlying type, that underlying field’s location. For compiler-synthesized fields of long tuples, empty. For user-named elements, the location where a name was provided.
    • IsTupleField: true if the field is an instance field of a type (ContainingType) for which IsTupleType is true.
    • IsVirtualTupleField, IsDefaultTupleElement, TupleUnderlyingField, CorrespondingTupleField, TupleElementIndex: as today.

Some interesting effects are on long tuples.

  • ValueTuple<T1, T2, T3, T4, T5, T6, T7, TRest> is not a tuple type. Its field Item1 and Rest have property IsTupleField false. This is existing behavior.
  • When it is instantiated or substituted with a tuple type as TRest, the result is a tuple type that has additional synthetic fields for Item8 and beyond.
  • The TRest field has the property IsTupleField that returns true when accessed from a long tuple type. This is existing behavior.

In C# 6 and prior, a constructed type always had the same number of members as the type it was constructed from. This is no longer the case in C# 7, as long tuple types have additional members that are not members of the type from which they are constructed (e.g. Item8). This change is reflected in the compiler APIs, which will no longer support the obsolete invariant. I haven’t yet found any code that depends on this invariant.

@gafter gafter added the Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality label Jul 13, 2017
@gafter
Copy link
Member Author

gafter commented Jul 18, 2017

@dotnet/roslyn-compiler This is a description of the set of changes we discussed today.

@gafter
Copy link
Member Author

gafter commented Nov 5, 2018

Per LDM decisions on 2018-11-05, the following additional changes should be made:

  • Instances of System.ValueTuple<T> are tuple types. (i.e., reconfirming the existing decision)
  • The type System.ValueTuple (an empty non-generic struct type) is a tuple type.
  • Instances of System.ValueTuple<T1, T2, T3, T4, T5, T6, T7, TRest> is a tuple type if TRest is a tuple type with one or more elements. The 0-element tuple type does not satisfy the requirement for TRest to permit the instantiation to be considered a tuple type.

@jcouv
Copy link
Member

jcouv commented Dec 17, 2019

Re-opened to track remaining work for VB (yet unscheduled, per discussion with Jared)

@jcouv jcouv removed the 4 - In Review A fix for the issue is submitted for review. label Dec 17, 2019
@jcouv jcouv changed the title INamedTypeSymbol.Arity incorrectly reports zero for ValueTuple<int, int> INamedTypeSymbol.Arity incorrectly reports zero for ValueTuple<int, int> in VB Dec 17, 2019
@jcouv jcouv modified the milestones: 16.5, Backlog Dec 17, 2019
gafter pushed a commit to gafter/roslyn that referenced this issue Apr 20, 2020
gafter pushed a commit that referenced this issue Apr 21, 2020
* Add a test to verify that a bug is fixed.
Fixes #27322 (C# only)
VB fix depends on #20648
@gafter gafter removed their assignment Jul 2, 2020
@Youssef1313
Copy link
Member

Youssef1313 commented Dec 3, 2022

@jcouv This issue is supposed to be done for C# and still open only for VB right?

It's currently referenced in C# compiler source:

// Work around https://github.com/dotnet/roslyn/issues/20648: The compiler's internal APIs such as `declType.IsTupleType`

// Work around https://github.com/dotnet/roslyn/issues/20648: The compiler's internal APIs such as `declType.IsTupleType`

// Work around https://github.com/dotnet/roslyn/issues/20648: The compiler's internal APIs such as `declType.IsTupleType`

// Work around https://github.com/dotnet/roslyn/issues/20648: The compiler's internal APIs such as `declType.IsTupleType`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Bug Concept-API This issue involves adding, removing, clarification, or modification of an API. Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality Language-VB New Language Feature - Tuples Tuples
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants