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

Porting two recent tuple-related fixes to VB #14518

Merged
merged 5 commits into from
Oct 16, 2016
Merged

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Oct 13, 2016

Fixes: #14260

== VB counterpart for:
Fixes: #10891 - Forbid constructing tuples via "new"
Fixes: #11689 - Enforce that ValueTuple types are structs

Note that #11689 requires validation that ValueTuple is a value type only in common cases, That is - to convey that cases where ValueTuple happens to be a Class, Enum, Delegate, etc are clearly not supported.

From the language prospective, tuples are value types composed of their elements. That is visible through the semantics of definite assignment, conversions, nullable lifting, async capturing and so on. This change intentionally introduces emit failures when underlying ValueTuple types are not structs.

@VSadov
Copy link
Member Author

VSadov commented Oct 13, 2016

@dotnet/roslyn-compiler - please review

@VSadov
Copy link
Member Author

VSadov commented Oct 13, 2016

@MattGertz - for RC approval.

This is mostly just a parity with C#.

  • We do not have a well defined behavior for cases when ValueTuple types are classes/interfaces/enums etc, so we produce an error to intentionally not support such scenarios.
  • We do not want tuple types be used in New <Type> expressions because a simpler, more concise syntax exists for tuple creation, while New (a, b) may also be infringing on the syntax for another proposed feature.

@MattGertz
Copy link
Contributor

Approved pending tests and signoffs.

@@ -315,6 +315,8 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
resultKind = LookupResultKind.NotCreatable
errorReported = True



Case TypeKind.Array
Copy link
Member

Choose a reason for hiding this comment

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

Nit: spacing

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks

@VSadov
Copy link
Member Author

VSadov commented Oct 14, 2016

@dotnet-bot test this please

@AlekseyTs
Copy link
Contributor

LGTM

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.

6 participants