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

Implements GetDeclaredSymbol for tuple literals and GetSymbolInfo for elements of tuple literals. #16659

Merged
merged 5 commits into from
Jan 26, 2017

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Jan 20, 2017

Implements GetDeclaredSymbol for tuple literals and GetSymbolInfo for elements of tuple literals.
Unimplemented GetDeclaredSymbol was responsible for erratic behavior and crashes in IDE.

Fixes:#14600
Fixes:#11013
Fixes:#14116
Fixes:#16168

Customer scenario

Even though tuple literals are expressions, they implicitly declare tuple types and elements.
Tuple literals do not provide semantic information about tuple literals.
(note: we do produce correct semantic info for tuple types we do not do so for tuple literals)

That leads to crashes in some "rename" scenarios, incorrect colorization, erratic behavior of "go to definition" and other misbehavior of IDE features built on top of "find all references" engine.

The root cause is that inferred references could be incorrectly computed - i.e.
In the following code "Customer" and "Orders" could be understood by IDE as references to types, members or locals as long as they have same name.

var x = (Customer: 1, Orders: 2);

Bugs this fixes:

Primarily #14600,
there are several other bugs caused by the same issue.
#11013
#14116
#16168

Workarounds, if any

User must ignore incorrect colorization and should not try renaming symbols that are erroneously considered references from the tuple element.

I.E. when IDE thinks that a tuple element name is actually a reference to a containing type, renaming the element will be allowed (even though renaming elements is otherwise blocked). That could lead to both the element and the incorrectly referred to type being renamed, leading to crashes.

Risk

Risk is low.

This fix does not enable renaming tuple elements. Enabling that is a 2.1 item.
It however makes the blocking to be more effective.
I.E. crashing scenarios that should be blocked will be blocked as intended.

Performance impact

Low.
Just correctly implementing an existing semantic info API. It is not any special in terms of complexity.

Is this a regression from a previous update?

New feature

Root cause analysis:

Tuple literal expression are special since they declare types and elements. An API for returning info on those was not implemented. The effects of not providing semantic info for those was not believed as potentially crashing, so the fix was pushed to 2.1
Now we know about crashing scenarios.

How was the bug found?

Ad hock testing. Customer reports.

@VSadov
Copy link
Member Author

VSadov commented Jan 23, 2017

@dotnet/roslyn-compiler - please review

{
static void Main()
{
bar x1 = (Alice: 1, ""hello"");
Copy link
Member

@jcouv jcouv Jan 23, 2017

Choose a reason for hiding this comment

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

s/bar/var/
Maybe add VerifyDiagnostics #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

It actually should work in the presence of errors. The error here was accidental, I think I will just make it more apparent.


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

Assert.Equal(SymbolKind.Field, sym.Symbol.Kind);
Assert.Equal("Alice", sym.Symbol.Name);
Assert.Equal(nc.Name.GetLocation(), sym.Symbol.Locations[0]);
}
Copy link
Member

@jcouv jcouv Jan 23, 2017

Choose a reason for hiding this comment

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

Nit: two empty lines #Resolved

}


[Fact]
Copy link
Member

@jcouv jcouv Jan 23, 2017

Choose a reason for hiding this comment

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

I'd suggest adding an IDE test on refactoring too (since you're listing #14116 as getting fixed). Cyrus can probably point to some examples. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Renaming is actually still blocked. I will add a test that we were missing and it was causing crashes.


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

@@ -256,6 +256,7 @@ override Microsoft.CodeAnalysis.CSharp.Syntax.WhenClauseSyntax.Accept(Microsoft.
override Microsoft.CodeAnalysis.CSharp.Syntax.WhenClauseSyntax.Accept<TResult>(Microsoft.CodeAnalysis.CSharp.CSharpSyntaxVisitor<TResult> visitor) -> TResult
static Microsoft.CodeAnalysis.CSharp.CSharpExtensions.GetDeclaredSymbol(this Microsoft.CodeAnalysis.SemanticModel semanticModel, Microsoft.CodeAnalysis.CSharp.Syntax.SingleVariableDesignationSyntax designationSyntax, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> Microsoft.CodeAnalysis.ISymbol
static Microsoft.CodeAnalysis.CSharp.CSharpExtensions.GetDeclaredSymbol(this Microsoft.CodeAnalysis.SemanticModel semanticModel, Microsoft.CodeAnalysis.CSharp.Syntax.TupleElementSyntax declarationSyntax, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> Microsoft.CodeAnalysis.ISymbol
static Microsoft.CodeAnalysis.CSharp.CSharpExtensions.GetDeclaredSymbol(this Microsoft.CodeAnalysis.SemanticModel semanticModel, Microsoft.CodeAnalysis.CSharp.Syntax.TupleExpressionSyntax declaratorSyntax, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> Microsoft.CodeAnalysis.INamedTypeSymbol
Copy link
Member

@jcouv jcouv Jan 23, 2017

Choose a reason for hiding this comment

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

FYI for @gafter @jaredpar on public API change. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

I will write a short description of expected behavior here - to be sure we are on the same page.
Overall, it mostly follows the existing behavior of similar scenarios where anonymous types are involved.


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

Copy link
Member Author

Choose a reason for hiding this comment

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

CC @AnthonyDGreen


In reply to: 97446605 [](ancestors = 97446605,97433588)

@@ -217,23 +217,13 @@ internal TupleTypeSymbol WithUnderlyingType(NamedTypeSymbol newUnderlyingType)
/// <summary>
/// Copy this tuple, but modify it to use the new element names.
/// </summary>
internal TupleTypeSymbol WithElementNames(ImmutableArray<string> newElementNames)
internal TupleTypeSymbol WithElementNames(ImmutableArray<string> newElementNames,
Location location,
Copy link
Member

@jcouv jcouv Jan 23, 2017

Choose a reason for hiding this comment

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

Nit: Naming inconsistency. Some parameters are "new" but others aren't. #Resolved

{
static void Main()
{
(short X, string Y) t = (Alice: 1, Bob: null);
Copy link
Member

@jcouv jcouv Jan 23, 2017

Choose a reason for hiding this comment

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

Consider testing a long and partially-named tuple. #Resolved

Copy link
Member

Choose a reason for hiding this comment

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

Consider testing with ValueTuple too.


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

Copy link
Member

Choose a reason for hiding this comment

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

Consider checking a deconstruction (the left-hand-side is a tuple expression with no element names).


In reply to: 97436485 [](ancestors = 97436485,97434497)

Assert.Equal(element.ToDisplayString(SymbolDisplayFormat.CSharpErrorMessageFormat), "(short Alice, string Bob).Bob");
Assert.Equal(element.DeclaringSyntaxReferences.Single().GetSyntax().ToString(), "Bob");
Assert.Equal(element.Locations.Single().IsInSource, true);
}
Copy link
Member

@jcouv jcouv Jan 23, 2017

Choose a reason for hiding this comment

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

Do we have a similar problem with GetDeclaredSymbol in VB? #Resolved

Copy link
Member Author

@VSadov VSadov Jan 23, 2017

Choose a reason for hiding this comment

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

Yes, added #16697 to triage/track that separately #Resolved

@@ -353,12 +354,36 @@ private BoundExpression CreateTupleLiteralConversion(SyntaxNode syntax, BoundTup
if (targetType.IsTupleType)
{
var destTupleType = (TupleTypeSymbol)targetType;
// do not lose the original element names in the literal if different from names in the target

TupleTypeSymbol.ReportNamesMismatchesIfAny(targetType, sourceTuple, diagnostics);

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

@jcouv jcouv Jan 23, 2017

Choose a reason for hiding this comment

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

Should this comment be removed now? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so.


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


TupleTypeSymbol.ReportNamesMismatchesIfAny(targetType, sourceTuple, diagnostics);

// Come back to this, what about locations? (https://github.com/dotnet/roslyn/issues/11013)
targetType = destTupleType.WithElementNames(sourceTuple.ArgumentNamesOpt);

// do not lose the original element names and locatins in the literal if different from names in the target
Copy link
Member

@jcouv jcouv Jan 23, 2017

Choose a reason for hiding this comment

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

locatins [](start = 62, length = 8)

s/locatins/locations #Resolved

if (parent3.IsKind(SyntaxKind.TupleExpression))
{
var tupleArgument = (ArgumentSyntax)identifierNameSyntax.Parent.Parent;
var typleElement = GetDeclaredSymbol(tupleArgument, cancellationToken);
Copy link
Member

@jcouv jcouv Jan 23, 2017

Choose a reason for hiding this comment

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

typleElement [](start = 20, length = 12)

s/typleElement/tupleElement/ #Resolved

@gafter
Copy link
Member

gafter commented Jan 23, 2017

I am not sure I agree with the premise of this fix. I don't believe that a tuple expression such as (x: 0, y: 1) "declares" a tuple type. It refers to an implicitly declared tuple type. This is syntactically analogous to named arguments in a method invocation: they are references to the parameters. As such, GetSymbolInfo is an appropriate API to expect to use to get the tuple element symbol. #Resolved

@VSadov
Copy link
Member Author

VSadov commented Jan 23, 2017

The tuple syntax declares the tuple type/fields in exactly the same way as an anonymous type creation declares its type/fields.
In both cases the syntax clearly "brings the type into being" and as such declares it.

If the syntax only refers to an existing type, where is the syntax that declares the type?
What syntax would I need to modify to rename the fields of the tuple, if that is not the syntax of the literal?

I do not think this should be any different than the existing solution for the anonymous type case.


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

@VSadov
Copy link
Member Author

VSadov commented Jan 24, 2017

Added a comment describing how and why GetDeclaredSymbol works for tuple literals as well as for anonymous type creation syntax.

In short: In 'var x = (Alice:2, Bob:3)' The 'Alice: 2' syntax declares a field (similar to anonymous type fields), and 'Alice' refers to that field by its name (similar to anon types and named parameters).


In reply to: 274656187 [](ancestors = 274656187,274648495)

@jcouv jcouv added this to the 2.0 (RTM) milestone Jan 24, 2017
@gafter
Copy link
Member

gafter commented Jan 24, 2017

I do not think this should be any different than the existing solution for the anonymous type case.

Yes, that makes sense to me. #Resolved

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

Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

LGTM

// This way IDEs can unambiguously implement such services as "Go to definition"
//
// I.E. GetSymbolInfo for "tuple.Alice" should point to the same field as returned by GetDeclaredSymbol when applied to
// the ArgumentSyntax "Bob: 2", since that is where the field was declared, where renames should be applied and so on.
Copy link
Member

@gafter gafter Jan 24, 2017

Choose a reason for hiding this comment

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

Bob: 2 should be Alice: 1. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, these were supposed to be the same element.


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

public abstract INamedTypeSymbol GetDeclaredSymbol(TupleExpressionSyntax declaratorSyntax, CancellationToken cancellationToken = default(CancellationToken));

/// <summary>
/// Given a syntax node of an argument expression, get the declared symbol.
Copy link
Member

@gafter gafter Jan 24, 2017

Choose a reason for hiding this comment

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

Please add more explanation here (i.e. explain that an ArgumentSyntax declares an element in a tuple literal). #Resolved

@@ -259,7 +259,13 @@ public static bool IsNamedArgumentName(SyntaxNode node)
return false;

var parent3 = parent2.Parent;
if (parent3 == null || !(parent3 is BaseArgumentListSyntax || parent3.IsKind(AttributeArgumentList)))
if (parent3 == null)
Copy link
Member

@gafter gafter Jan 24, 2017

Choose a reason for hiding this comment

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

Please add {} per coding conventions where missing in this method. #Resolved

@VSadov
Copy link
Member Author

VSadov commented Jan 25, 2017

@MattGertz - need an approval for the next step towards RTW merge. Thanks!!

@jcouv
Copy link
Member

jcouv commented Jan 25, 2017

@jaredpar
Copy link
Member

Adding more testing per IDE request.

@jaredpar
Copy link
Member

CC @MattGertz for escrow approval now that IDE has signed off on the additional testing.

Copy link
Contributor

@dpoeschl dpoeschl left a comment

Choose a reason for hiding this comment

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

Late to the party, but 👍 on the unit tests.

@natidea
Copy link
Contributor

natidea commented Jan 26, 2017

Approved to merge via 373649

@VSadov VSadov merged commit f3717ae into dotnet:master Jan 26, 2017
VSadov added a commit to VSadov/roslyn that referenced this pull request Jan 27, 2017
dotnet#16659

added tests, made the included tuple2 chunk a CData
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Got to this one late, but was curious to take a look and have some comments. Nothing critical, but a few typos and other suggestions.

@@ -1206,6 +1206,24 @@ public static INamedTypeSymbol GetDeclaredSymbol(this SemanticModel semanticMode
}

/// <summary>
/// Given a syntax node of tuple expression, get the tuple type symbol.
/// </summary>
public static INamedTypeSymbol GetDeclaredSymbol(this SemanticModel semanticModel, TupleExpressionSyntax declaratorSyntax, CancellationToken cancellationToken = default(CancellationToken))
Copy link
Member

Choose a reason for hiding this comment

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

Incorrect parameter name.

/// <summary>
/// Given a syntax node of a tuple argument, get the tuple element symbol.
/// </summary>
public static ISymbol GetDeclaredSymbol(this SemanticModel semanticModel, ArgumentSyntax declaratorSyntax, CancellationToken cancellationToken = default(CancellationToken))
Copy link
Member

Choose a reason for hiding this comment

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

Incorrect parameter name.

/// <param name="cancellationToken">The cancellation token.</param>
/// <returns>The symbol that was declared.</returns>
/// <remarks>
/// Generally ArgumentSyntax nodes do not declare symbols, except when used as aarguments of a tuple literal.
Copy link
Member

Choose a reason for hiding this comment

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

Typo: "aarguments"

@@ -22,4 +22,6 @@
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("ApiDesign", "RS0026:Do not add multiple public overloads with optional parameters", Justification = "<Pending>", Scope = "member", Target = "~M:Microsoft.CodeAnalysis.CSharp.CSharpExtensions.GetDeclaredSymbol(Microsoft.CodeAnalysis.SemanticModel,Microsoft.CodeAnalysis.CSharp.Syntax.TupleElementSyntax,System.Threading.CancellationToken)~Microsoft.CodeAnalysis.ISymbol")]
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("ApiDesign", "RS0027:Public API with optional parameter(s) should have the most parameters amongst its public overloads.", Justification = "<Pending>", Scope = "member", Target = "~M:Microsoft.CodeAnalysis.CSharp.SyntaxFactory.ParenthesizedVariableComponent(Microsoft.CodeAnalysis.SeparatedSyntaxList{Microsoft.CodeAnalysis.CSharp.Syntax.VariableComponentSyntax})~Microsoft.CodeAnalysis.CSharp.Syntax.ParenthesizedVariableComponentSyntax")]
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("ApiDesign", "RS0027:Public API with optional parameter(s) should have the most parameters amongst its public overloads.", Justification = "<Pending>", Scope = "member", Target = "~M:Microsoft.CodeAnalysis.CSharp.SyntaxFactory.ParenthesizedVariableDesignation(Microsoft.CodeAnalysis.SeparatedSyntaxList{Microsoft.CodeAnalysis.CSharp.Syntax.VariableDesignationSyntax})~Microsoft.CodeAnalysis.CSharp.Syntax.ParenthesizedVariableDesignationSyntax")]
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("ApiDesign", "RS0026:Do not add multiple public overloads with optional parameters", Justification = "<Pending>", Scope = "member", Target = "~M:Microsoft.CodeAnalysis.CSharp.CSharpExtensions.GetDeclaredSymbol(Microsoft.CodeAnalysis.SemanticModel,Microsoft.CodeAnalysis.CSharp.Syntax.TupleExpressionSyntax,System.Threading.CancellationToken)~Microsoft.CodeAnalysis.INamedTypeSymbol")]
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("ApiDesign", "RS0026:Do not add multiple public overloads with optional parameters", Justification = "<Pending>", Scope = "member", Target = "~M:Microsoft.CodeAnalysis.CSharp.CSharpExtensions.GetDeclaredSymbol(Microsoft.CodeAnalysis.SemanticModel,Microsoft.CodeAnalysis.CSharp.Syntax.ArgumentSyntax,System.Threading.CancellationToken)~Microsoft.CodeAnalysis.ISymbol")]
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a bug tracking updating the diagnostic, since clearly it's OK for some "pending" reason?

return false;
}

if (parent3.IsKind(SyntaxKind.TupleExpression))
Copy link
Member

Choose a reason for hiding this comment

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

I believe the doc comment should have been updated here to include this case?

Assert.Equal(type.ToDisplayString(SymbolDisplayFormat.CSharpErrorMessageFormat), "(int, int)");
Assert.Equal(type.DeclaringSyntaxReferences.Single().GetSyntax().ToString(), "(1, 2)");
Assert.Equal(type.Locations.Single().IsInSource, true);

Copy link
Member

Choose a reason for hiding this comment

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

Blank line.


Namespace Microsoft.CodeAnalysis.Editor.UnitTests.FindReferences
Partial Public Class FindReferencesTests
Dim tuple2Doc As XElement =
Copy link
Member

Choose a reason for hiding this comment

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

@CyrusNajmabadi Didn't we add some support in core TestWorkspace to introduce this type?

Dim workspace =
<Workspace>
<Project Language="C#" CommonReferences="true">
<!-- intentionally not including tuple2, shoudl still work -->
Copy link
Member

Choose a reason for hiding this comment

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

Just curious: how? if you're missing the type the compiler at least still creates tuple symbols, but just can't emit them?

Public Sub RenameTupleFiledInLiteralRegress14600()
Using workspace = CreateWorkspaceWithWaiter(
<Workspace>
<Project Language="C#" CommonReferences="true" PreprocessorSymbols="__DEMO__">
Copy link
Member

Choose a reason for hiding this comment

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

We have better ways of turning these on, I think. @CyrusNajmabadi?

</Workspace>)

' NOTE: this is currently intentionally blocked
' see https://github.com/dotnet/roslyn/issues/10898
Copy link
Member

Choose a reason for hiding this comment

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

I believe this should be updated to #16566?

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