Bring back naive implementation of FAR for target-typed new by jcouv · Pull Request #43645 · dotnet/roslyn

jcouv · 2020-04-24T15:54:02Z

This reverts commit 6808612.

Relates to #28489

CyrusNajmabadi · 2020-04-24T16:55:34Z

I thought we were going to go with an approach using a background incremental analyzer to build a special index for these. #Closed

jcouv · 2020-04-24T16:58:53Z

@CyrusNajmabadi Yes, but we also discussed making progress by enabling the scenario and getting a sense of the perf problem. This PR would allow us to modify Roslyn with some target-typed new and get a feel. What do you think? #Closed

CyrusNajmabadi · 2020-04-24T17:51:20Z

This PR would allow us to modify Roslyn with some target-typed new and get a feel. What do you think?

Got it. For me, i thought we were saying that we woudl more do it as an experiment. i.e. it's fine to have this is draft, and use it to get some perf numbers. But i'm not sure i would want to preemptively merge things until we had that info. Is that ok with you? #Closed

jcouv · 2020-05-07T17:15:40Z

Re-opened PR following experiment we discussed.

In short:
I changed about half of object creations in CodeAnalysis.CSharp to use target-type new.
Then did a FindAllReferences on a few constructors. Also added some edits (adding a new type) in the mix.
The performance seems similar to invoking FindAllReferences on a popular method name (like Equals), which is what we’d expect. #Closed

This reverts commit 6808612.

jcouv · 2020-05-08T17:07:37Z

@CyrusNajmabadi I rebased to resolve conflicts. Feel free to take another look when you have time. #Closed

CyrusNajmabadi · 2020-05-08T18:32:18Z

src/EditorFeatures/Test2/FindReferences/FindReferencesTests.ConstructorSymbols.vb

+</Workspace>
+            Await TestAPIAndFeature(input, kind, host)
+        End Function
+


Suggested change

``` #Resolved

CyrusNajmabadi · 2020-05-08T18:33:14Z

src/EditorFeatures/Test2/FindReferences/FindReferencesTests.ConstructorSymbols.vb

+        End Function
+
    End Class
 End Namespace


we should have some additional tests. for example, we should verify that reorder parameters (and all our other sig-change features) doesn't crash. if it does the right ting, great! if it doesn't open bug.

@dpoeschl can give you a full list. #Resolved

ChangeSignature indeed has some problems. Opened #44126 #Resolved

src/Workspaces/Core/Portable/FindSymbols/FindReferences/Finders/AbstractReferenceFinder.cs

CyrusNajmabadi · 2020-05-08T18:35:44Z

...kspaces/Core/Portable/FindSymbols/FindReferences/Finders/ConstructorSymbolReferenceFinder.cs

        }

+        private bool IsConstructor(IMethodSymbol methodSymbol)
+            => methodSymbol.MethodKind == MethodKind.Constructor;


don't know if this method feels warranted :)

static #Resolved

CyrusNajmabadi · 2020-05-08T18:36:37Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/Rules/SyntaxKindEx.cs

    [SuppressMessage("CodeQuality", "IDE0051:Remove unused private members", Justification = "Private constants are used for static assertions.")]
    internal static class SyntaxKindEx
    {
+        public const SyntaxKind ImplicitObjectCreationExpression = (SyntaxKind)8659;


can you add teh same overflow check below in #!CODE_STYLE? #Resolved

CyrusNajmabadi · 2020-05-08T18:37:44Z

...SharedUtilitiesAndExtensions/Workspace/CSharp/LanguageServices/CSharpSemanticFactsService.cs

+            if (node.Kind() == Formatting.SyntaxKindEx.ImplicitObjectCreationExpression)
+            {
+                var info = semanticModel.GetSymbolInfo(node);
+                return (IMethodSymbol)info.Symbol;


is this what we do for normal constructors? i.e. this is ignoring candidates in the case of errors? if that is what we do normally, grat. but if we normally use any/all of the symbols we get back for a normal constructor, then we should follow that here. #Resolved

also, i don't think this method is needed. it is necessary when we'll need some sort of funky semantic model call to get specific information (like foreach-info), but for FAR, it could just call .GetSymbolInfo(node) since it already knows it's an implici-obje-expr. #Resolved

I had not thought of that. As far as I understand, the logic for FAR on constructors will only consider non-error symbols (uses GetSymbolInfo(...).Symbol only, ignoring candidates):

protected static Func<SyntaxNode, SemanticModel, (bool matched, CandidateReason reason)> GetStandardSymbolsNodeMatchFunction( ISymbol searchSymbol, Solution solution, CancellationToken cancellationToken) { return (node, model) => { var symbolInfoToMatch = FindReferenceCache.GetSymbolInfo(model, node, cancellationToken); var symbolToMatch = symbolInfoToMatch.Symbol; var symbolToMatchCompilation = model.Compilation; if (SymbolFinder.OriginalSymbolsMatch(searchSymbol, symbolInfoToMatch.Symbol, solution, null, symbolToMatchCompilation, cancellationToken)) { return (matched: true, CandidateReason.None); } else if (symbolInfoToMatch.CandidateSymbols.Any(s => SymbolFinder.OriginalSymbolsMatch(searchSymbol, s, solution, null, symbolToMatchCompilation, cancellationToken))) { return (matched: true, symbolInfoToMatch.CandidateReason); } else { return (matched: false, CandidateReason.None); } }; }

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

CyrusNajmabadi · 2020-05-11T17:14:05Z

...kspaces/Core/Portable/FindSymbols/FindReferences/Finders/ConstructorSymbolReferenceFinder.cs

            var ordinaryRefs = await FindOrdinaryReferencesAsync(symbol, document, semanticModel, findParentNode, cancellationToken).ConfigureAwait(false);
            var attributeRefs = await FindAttributeReferencesAsync(symbol, document, semanticModel, cancellationToken).ConfigureAwait(false);
            var predefinedTypeRefs = await FindPredefinedTypeReferencesAsync(symbol, document, semanticModel, cancellationToken).ConfigureAwait(false);
+            var implicitOjbectCreationMatches = await FindReferencesInImplicitObjectCreationExpressionAsync(symbol, document, semanticModel, cancellationToken).ConfigureAwait(false);


Ojbect #Resolved

CyrusNajmabadi · 2020-05-11T17:14:40Z

...kspaces/Core/Portable/FindSymbols/FindReferences/Finders/ConstructorSymbolReferenceFinder.cs

+            var implicitOjbectCreationMatches = await FindReferencesInImplicitObjectCreationExpressionAsync(symbol, document, semanticModel, cancellationToken).ConfigureAwait(false);

-            return ordinaryRefs.Concat(attributeRefs).Concat(predefinedTypeRefs);
+            return ordinaryRefs.Concat(attributeRefs).Concat(predefinedTypeRefs).Concat(implicitOjbectCreationMatches);


aside, do we have a Concat that takes in multiple Immutable arrays. it's a pity about all these copies. #Resolved

We don't have such a Concat overload. Added #Resolved

CyrusNajmabadi

LGTM. can you fix ojbect? (either in this PR or a followup).

jcouv added the Area-IDE label Apr 24, 2020

jcouv added this to the 16.7 milestone Apr 24, 2020

jcouv self-assigned this Apr 24, 2020

jcouv marked this pull request as ready for review April 24, 2020 19:13

jcouv requested a review from a team as a code owner April 24, 2020 19:13

jcouv requested a review from CyrusNajmabadi April 24, 2020 19:13

jcouv marked this pull request as draft April 24, 2020 21:49

jcouv marked this pull request as ready for review May 7, 2020 17:15

Bring back naive implementation of FAR for target-typed new

9f3378a

This reverts commit 6808612.

jcouv force-pushed the far-new branch from 1cbcb04 to 96ed443 Compare May 8, 2020 16:57

Fix by conflict resolution

ea3aa8c

jcouv force-pushed the far-new branch from 96ed443 to ea3aa8c Compare May 8, 2020 17:07

CyrusNajmabadi reviewed May 8, 2020

View reviewed changes

src/Workspaces/Core/Portable/FindSymbols/FindReferences/Finders/AbstractReferenceFinder.cs Show resolved Hide resolved

CyrusNajmabadi reviewed May 8, 2020

View reviewed changes

jcouv mentioned this pull request May 11, 2020

ChangeSignature doesn't update constructor usages with target-type new #44126

Closed

Address feedback

add4cc5

jcouv requested a review from CyrusNajmabadi May 11, 2020 16:24

CyrusNajmabadi reviewed May 11, 2020

View reviewed changes

CyrusNajmabadi approved these changes May 11, 2020

View reviewed changes

Fix typo and add Concat overloads

0091fa1

jcouv requested a review from a team as a code owner May 11, 2020 20:51

jcouv merged commit 7179688 into dotnet:master May 11, 2020

ghost modified the milestones: 16.7, Next May 11, 2020

jcouv deleted the far-new branch May 11, 2020 23:15

jcouv mentioned this pull request May 11, 2020

Test plan for "target-typed new" #28489

Open

50 tasks

JoeRobich modified the milestones: Next, 16.7.P2 May 18, 2020

Conversation

jcouv commented Apr 24, 2020 • edited by sharwell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Apr 24, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Apr 24, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi May 8, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 8, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyrusNajmabadi May 8, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 8, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 8, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 8, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv May 10, 2020

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 11, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 11, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jcouv commented Apr 24, 2020 •

edited by sharwell

Loading

CyrusNajmabadi commented Apr 24, 2020 •

edited by jcouv

Loading

jcouv commented Apr 24, 2020 •

edited

Loading

CyrusNajmabadi commented Apr 24, 2020 •

edited by jcouv

Loading

jcouv commented May 7, 2020 •

edited

Loading

jcouv commented May 8, 2020 •

edited

Loading

CyrusNajmabadi May 8, 2020 •

edited by jcouv

Loading

CyrusNajmabadi May 8, 2020 •

edited by jcouv

Loading

jcouv May 11, 2020 •

edited

Loading

CyrusNajmabadi May 8, 2020 •

edited by jcouv

Loading

CyrusNajmabadi May 8, 2020 •

edited by jcouv

Loading

CyrusNajmabadi May 8, 2020 •

edited by jcouv

Loading

CyrusNajmabadi May 8, 2020 •

edited by jcouv

Loading

CyrusNajmabadi May 11, 2020 •

edited by jcouv

Loading

CyrusNajmabadi May 11, 2020 •

edited by jcouv

Loading

jcouv May 11, 2020 •

edited

Loading