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

Design Question : Should we have a special node for Out Var Declaration #18303

Closed
jinujoseph opened this issue Mar 29, 2017 · 7 comments
Closed
Assignees
Labels
Area-Analyzers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature - IOperation IOperation Urgency-Soon
Milestone

Comments

@jinujoseph
Copy link
Contributor

Example here : #18300

@jinujoseph jinujoseph added this to the 15.3 milestone Mar 29, 2017
@jinujoseph jinujoseph added the Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality label Apr 4, 2017
@jinujoseph jinujoseph added Concept-API This issue involves adding, removing, clarification, or modification of an API. and removed Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality labels Apr 18, 2017
@jinujoseph jinujoseph modified the milestones: 15.later, 15.3 Apr 18, 2017
@jinujoseph
Copy link
Contributor Author

we need a way to represent this

@jinujoseph
Copy link
Contributor Author

@333fred , can you come up with the API proposal here

@333fred
Copy link
Member

333fred commented Aug 8, 2017

Some example code for context:

class C1
{
    public void M1()
    {
        M2(out var i);
    }
    public void M2(out int i)
    {
        i = 1;
    }
}

@333fred
Copy link
Member

333fred commented Aug 8, 2017

In this scenario, the underlying BoundNode for out var i is actually just a BoundLocal, which we already map to ILocalReferenceExpression. The underlying BoundLocal has a field on it, IsDeclaration, that is true here and false when not in an out var declaration or in a deconstruction statement. The simple solution here would be to just add a new field to ILocalReferenceExpression:

/// <summary>
/// True if this reference is also the declaration site of this variable. This is true in out variable declarations
/// and in deconstruction operations where a new variable is being declared.
/// </summary>
public bool IsDeclaration { get; }

The BoundLocal node for these declarations doesn't have anything else interesting in it that isn't already captured by the ILocalReferenceExpression.

Alternatively, we could create a new class ILocalDeclarationAndReferenceExpression that is exactly the same as ILocalReferenceExpression. We would then modify the CSharpOperationFactory's handling of BoundLocal to return the correct type.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Aug 10, 2017

Design decision: We will do things as @333fred outlined. Note that we may also need to add .IsDeclaration to "IFieldReferenceExpression" (because of how scripting works).

We should also test and validate that this all works for "out var" as well as deconstructions like "(var x, var y) = ..."

@333fred
Copy link
Member

333fred commented Aug 22, 2017

The underlying BoundFieldReference has no concept of whether or not the field was declared here, so I don't think that there's a good way of getting that information.

@CyrusNajmabadi
Copy link
Member

The underlying BoundFieldReference has no concept of whether or not the field was declared here

We can either go back to syntax (as an implementation detail, not for the client to have to do). Or we can add this info to BoundFieldReference.

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 Urgency-Soon
Projects
None yet
Development

No branches or pull requests

3 participants