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

Implement IOperation support for tuple literals #10856

Closed
VSadov opened this issue Apr 25, 2016 · 4 comments
Closed

Implement IOperation support for tuple literals #10856

VSadov opened this issue Apr 25, 2016 · 4 comments
Assignees
Milestone

Comments

@VSadov
Copy link
Member

VSadov commented Apr 25, 2016

Design and implement IOperation support for tuple literals.

Current implementation intentionally implements this as OperationKind.None

The right alternative seems to be introducing a specific operation kind and make visitor visit all the arguments of the literal.

Alternatively we may make tuple literals look like ObjectCreation (which it is indirectly), but it does not feel right conceptually.

@mavasani
Copy link
Contributor

mavasani commented Apr 19, 2017

Possible API shape:

    public interface ITupleExpression : IOperation
    {
        /// <summary>
        /// Arguments for tuple expression.
        /// </summary>
        ImmutableArray<IOperation> Arguments { get; }
    }

Points to discuss:

  1. Should the API expose the optional tuple argument names?
  2. Bound tree has a separate BoundConvertedTupleLiteral node, which is identical to BoundTupleLiteral, except it represents a tuple literal with source type and destianation. I don't believe we should have this in the IOperation API - instead we should just wrap an ITupleExpression with an IConversion for a BoundConvertedTupleLiteral.

@jinujoseph jinujoseph modified the milestones: 15.6, 15.3 May 8, 2017
mavasani added a commit that referenced this issue May 18, 2017
Fixes #10856

```
    public interface ITupleExpression : IOperation
    {
        /// <summary>
        /// Elements for tuple expression.
        /// </summary>
        ImmutableArray<IOperation> Elements { get; }
    }
```
@jinujoseph jinujoseph added the 4 - In Review A fix for the issue is submitted for review. label May 25, 2017
@mavasani
Copy link
Contributor

Adding some design issues raised by Aleksey in the PR to implement this:

  1. Need to expose declaration expression nodes as a special Operation type: Add IOperation support for tuple expressions #19636 (comment)
  2. Bunch of unexpected conversions: Add IOperation support for tuple expressions #19636 (comment). Is this a compiler issue or Operation factory needs to filter out unwanted conversion nodes? @AlekseyTs suggested we need to have a design discussion around tuple conversions: Add IOperation support for tuple expressions #19636 (comment)

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jun 1, 2017

Design Team Discussion

Keep tuple tests with semantic model tests around tuples.
Try to keep IOperation for tuples in sync with semanticmodel.GetTypeInfo. That way we don't have two separate APIs that people can use that have inconsistent results.

@mavasani
Copy link
Contributor

Fixed with #20749

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

No branches or pull requests

6 participants