Skip to content

Fix inference when tuple names differ#25738

Merged
jcouv merged 3 commits intodotnet:dev15.7.xfrom
jcouv:tuple-inference
Mar 27, 2018
Merged

Fix inference when tuple names differ#25738
jcouv merged 3 commits intodotnet:dev15.7.xfrom
jcouv:tuple-inference

Conversation

@jcouv
Copy link
Copy Markdown
Member

@jcouv jcouv commented Mar 26, 2018

Customer scenario

Use a lambda in a method type inference, but have the element names between the lambda and the Func<... tuple ...> differ. This will fail the inference.
There is a similar bug with difference in dynamic-ness (ie. dynamic vs. object), which this PR also fixes.

Bugs this fixes

Fixes #24781

Workarounds, if any

Specify type arguments to avoid relying on type inference.

Risk

Performance impact

Low. One line fix.

Is this a regression from a previous update?

No.

Root cause analysis

This bug has been there since the tuples feature shipped. It stems from using the == comparison between type symbols (which compares everything), instead of the Equals method with comparison options.
We have an issue to remove usage of this == (and !=) in the compiler code: #22067

How was the bug found?

Reported by customer.

@jcouv jcouv added this to the 15.7 milestone Mar 26, 2018
@jcouv jcouv self-assigned this Mar 26, 2018
@jcouv jcouv requested a review from a team as a code owner March 26, 2018 23:14

var tree = comp.SyntaxTrees.First();
var model = comp.GetSemanticModel(tree);
var varType = tree.GetCompilationUnitRoot().DescendantNodes().OfType<LocalDeclarationStatementSyntax>().Single().Declaration.Type;
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Mar 27, 2018

Choose a reason for hiding this comment

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

var varType [](start = 12, length = 11)

it is hard to reason what this node is. #Closed

var tree = comp.SyntaxTrees.First();
var model = comp.GetSemanticModel(tree);
var varType = tree.GetCompilationUnitRoot().DescendantNodes().OfType<LocalDeclarationStatementSyntax>().Single().Declaration.Type;
Assert.Equal("IResult<System.Int32>", model.GetTypeInfo(varType).Type.ToTestDisplayString());
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Mar 27, 2018

Choose a reason for hiding this comment

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

GetTypeInfo [](start = 56, length = 11)

It we are testing method type argument inference, why not assert the method symbol? #Closed

var tree = comp.SyntaxTrees.First();
var model = comp.GetSemanticModel(tree);
var varType = tree.GetCompilationUnitRoot().DescendantNodes().OfType<LocalDeclarationStatementSyntax>().Single().Declaration.Type;
Assert.Equal("IResult<System.Int32>", model.GetTypeInfo(varType).Type.ToTestDisplayString());
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Mar 27, 2018

Choose a reason for hiding this comment

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

Assert.Equal("IResult<System.Int32>", model.GetTypeInfo(varType).Type.ToTestDisplayString()); [](start = 12, length = 93)

Same comments #Closed


public class Class1
{
public void Test1(IDo<(int, int)> impl)
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Mar 27, 2018

Choose a reason for hiding this comment

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

(int, int) [](start = 26, length = 10)

Consider also testing scenario when there are names here and they do not match those specified in a lambda #Closed

Dim nodes = tree.GetCompilationUnitRoot().DescendantNodes()
Dim case3 = nodes.OfType(Of LocalDeclarationStatementSyntax)().Single().Declarators.Single().Names.Single()

Assert.Equal("case3 As IResult(Of System.Int32)", model.GetDeclaredSymbol(case3).ToTestDisplayString())
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Mar 27, 2018

Choose a reason for hiding this comment

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

Assert.Equal("case3 As IResult(Of System.Int32)", model.GetDeclaredSymbol(case3).ToTestDisplayString()) [](start = 12, length = 103)

Same comments #Closed

@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented Mar 27, 2018

Done with review pass (iteration 1) #Closed

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Mar 27, 2018

@jaredpar for ask-mode approval for 15.7. Thanks

The CI errors are not related to this PR (PDBUsingTests issue) and I'll re-run tests once the workaround is merged.

@jaredpar
Copy link
Copy Markdown
Member

approved

Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (iteration 3)

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Mar 27, 2018

test windows_debug_unit32_prtest please

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Mar 27, 2018

test windows_release_unit64_prtest please

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Mar 27, 2018

Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense.VisualBasicCompletionCommandHandlerTests_Projections.TestInObjectCreationExpression failed in windows_debug_unit32_prtest (details)

@dotnet/roslyn-infrastructure FYI I'll re-run

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Mar 27, 2018

test windows_debug_unit32_prtest please

@jcouv jcouv merged commit ce4d020 into dotnet:dev15.7.x Mar 27, 2018
@jcouv jcouv deleted the tuple-inference branch March 27, 2018 19:58
333fred added a commit to 333fred/roslyn that referenced this pull request Mar 28, 2018
…features/dataflow

* dotnet/dev15.7.x:
  Fix inference when tuple names differ (dotnet#25738)
  Avoid crash with 'new ref[]' (dotnet#25505)
  Skip some PDBUsingTests tests to unblock CI (dotnet#25753)
  Disable Spanish queue
  Tuple equality: Optimize cases where nullable always has value (dotnet#25644)
  Bump prerelease version to beta4
  Fix failure to initialize RemoteWorkspace in the out-of-process host
  remove whitespace
  Removing package dependency for Microsoft.VisualStudio.Text.UI.Wpf in EditorFeatures
  Removing package dependency for Microsoft.VisualStudio.Text.UI.Wpf in CSharpEditorFeatures
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.

4 participants