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

Tuple and underlying type unification. Part 3. #11136

Merged
merged 2 commits into from
May 6, 2016

Conversation

AlekseyTs
Copy link
Contributor

  • Added support for properties and events in tuple type symbol.
  • Made additional adjustments to implementation of tuple symbol display around presence of element names.
  • Renamed Wrapper...Symbol classes to Wrapped...Symbol.

@VSadov, @jcouv, @dotnet/roslyn-compiler Please review.

- Added support for properties and events in tuple type symbol.
- Made additional adjustments to implementation of tuple symbol display around presence of element names.
@AlekseyTs
Copy link
Contributor Author

test prtest/lin/dbg/unit32 please

http://dotnet-ci.cloudapp.net/job/roslyn_prtest_lin_dbg_unit32/6872/

@@ -349,7 +349,7 @@ private BoundExpression CreateTupleConversion(CSharpSyntaxNode syntax, BoundTupl
var destTupleType = (TupleTypeSymbol)targetType;
// do not lose the original element names in the literal if different from names in the target

// PROTOTYPE(tuples): Come back to this, what about locations?
// Come back to this, what about locations? (https://github.com/dotnet/roslyn/issues/11013)
Copy link
Member

@VSadov VSadov May 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should keep the locations of the original tuple type

@VSadov
Copy link
Member

VSadov commented May 5, 2016

LGTM

if (CanUseTupleTypeName(symbol))
// If top level tuple uses non-default names, there is no way to preserve them
// unless we use tuple syntax for the type. So, we give them priority.
if (HasNonDefaultTupleElementNames(symbol) || CanUseTupleTypeName(symbol))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider a textual representation of the underlying type with names too? Maybe System.ValueTuple<int a, int b>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would violate the language grammar as it is defined today. Besides, the case when both the top level and the nested levels have friendly names cannot originate from source. So, I think it is fine to drop some information in this case.


In reply to: 62353540 [](ancestors = 62353540)

@jcouv
Copy link
Member

jcouv commented May 6, 2016

Looks good to me, but I don't really understand why we care to expose properties and events from the underlying type. It doesn't seem strictly required for the tuples feature.

LGTM

@AlekseyTs AlekseyTs merged commit b73307b into dotnet:features/tuples May 6, 2016
@gafter
Copy link
Member

gafter commented May 7, 2016

@AlekseyTs @VSadov @jcouv
I am a bit concerned about the volume of changes being done in support of possible tuple semantics without a specification of them sitting alongside the implementation. Has https://github.com/dotnet/roslyn/blob/future/docs/features/tuples.md been updated to track the changes as the changes are being made? We should not be changing the implementation unless the checked-in specification reflects the semantics that are being implemented.

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

Successfully merging this pull request may close these issues.

None yet

5 participants