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

IConditionalAccessInstanceExpression API Review #21279

Closed
jinujoseph opened this issue Aug 3, 2017 · 4 comments
Closed

IConditionalAccessInstanceExpression API Review #21279

jinujoseph opened this issue Aug 3, 2017 · 4 comments
Assignees
Labels
Area-Analyzers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature - IOperation IOperation Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Milestone

Comments

@jinujoseph
Copy link
Contributor

jinujoseph commented Aug 3, 2017

https://github.com/dotnet/roslyn/blob/features/ioperation/src/Compilers/Core/Portable/Operations/IConditionalAccessInstanceExpression.cs

  • Do we need this at all ?
  • Make sure to check if Analyzers use this

cc @dotnet/analyzer-ioperation

@jinujoseph jinujoseph added Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature - IOperation IOperation Urgency-Soon labels Aug 3, 2017
@jinujoseph jinujoseph added this to the 15.5 milestone Aug 3, 2017
@sharwell
Copy link
Member

sharwell commented Aug 3, 2017

This seems unnecessary since we can just use IConditionalAccessExpression.Expression.

@333fred
Copy link
Member

333fred commented Aug 17, 2017

@mavasani, I want to bring this to discussion today. Some example code:

    public void M()
    {
        var o = new object();
        o?.ToString();
    }

This generates the following tree:

IConditionalAccessExpression (OperationKind.ConditionalAccessExpression, Type: System.String) (Syntax: 'o?.ToString()')
  Left: ILocalReferenceExpression: o (OperationKind.LocalReferenceExpression, Type: System.Object) (Syntax: 'o')
  Right: IInvocationExpression (virtual System.String System.Object.ToString()) (OperationKind.InvocationExpression, Type: System.String) (Syntax: '.ToString()')
      Instance Receiver: IConditionalAccessInstanceExpression (OperationKind.ConditionalAccessInstanceExpression, Type: System.Object) (Syntax: 'o')
      Arguments(0)

The problem with removing this is that the instance receiver, in the bound nodes, is a BoundConditionalReceiver. That node doesn't have information about the actual receiver, in this case the ILocalReferenceExpression. It gets even trickier if you consider something like new object()?.ToString(): What would we put as the receiver here? We can't just copy the IObjectCreationExpression, as that would make it seem like we're instantiating two different objects. I don't think we can remove this API.

@333fred
Copy link
Member

333fred commented Aug 17, 2017

I do think we should add an extension method here though, something like GetInstance(), that will walk the parent tree to the actual receiver of the call. Seems like it would be a common thing that users would want to do.

@333fred
Copy link
Member

333fred commented Aug 18, 2017

Design Decision:
This API is necessary. However, we will add an extension method that will walk the parent tree to retrieve the expression that the IConditionalAccessInstanceExpression is referencing.

@tmat tmat removed the Urgency-Soon label Sep 6, 2017
@jinujoseph jinujoseph modified the milestones: 15.5, 15.later Sep 29, 2017
@jinujoseph jinujoseph modified the milestones: 15.6, Unknown Nov 2, 2017
@jinujoseph jinujoseph added the Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented label Jun 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Analyzers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature - IOperation IOperation Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Projects
None yet
Development

No branches or pull requests

6 participants