Fix declaration name completion when local functions present. by dibarbet · Pull Request #35914 · dotnet/roslyn

dibarbet · 2019-05-23T18:36:05Z

Resolves #35891

CyrusNajmabadi · 2019-05-23T18:42:37Z

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs

this all seems like something that GenerateUniqueName should do. That's basically teh point of it. You say "here's the name I'd like, and i'm in this location" and it contains all the smarts.

We'd only want the logic here if it would only apply to this feature.

Generate unique name does something similar, the main difference being it doesn't look at local functions.

GenerateUniqueName is used for both VB and C#, so I can't directly move this check into there since as far as I know VB doesn't have local functions.

I can have the logic in the declaration name completion provider (as it is), or I could potentially pass another filter to GenerateUniqueName and pass it along to semanticModel.GetExistingSymbols(...)

@CyrusNajmabadi any thoughts on which is better?

GenerateUniqueName is used for both VB and C#, so I can't directly move this check into there since as far as I know VB doesn't have local functions.

You def can update it. it's already in an inheritance hierarchy in case there's some VB or C# specific behavior that is needed.

Also... this logic doesn't seem correct to me... you're filteirng out local functions. But it's certainly the case that a local function has a name, and you shouldn't introduce a variable with the same name as that local function.

looking at the bug you're trying to fix, the root cause is that we're taking the declared symbols for the parameters and treating it as if that means those names are rserved in other scopes. that's really not how local function parameters work. First, it's acceptable for there to be a collision with them and outer variables (i.e. local function parameters were cahnged to hide outer variables). Second, local function params are only in scope inside the local funciton, not in the containing block (like how other symbols work). We really need to fixup GenerateUniqueName (or whatever helper it is using) to understand this.

GenerateUniqueName is used for both VB and C#, so I can't directly move this check into there since as far as I know VB doesn't have local functions.

You def can update it. it's already in an inheritance hierarchy in case there's some VB or C# specific behavior that is needed.

Hmm it looks possible, I'll have to mess around with it a bit. Both the the C# and VB implementation delegate to the same implementation in AbstractSemanticFactsService

To get C# specific behavior, I'll need to make the methods AbstractSemanticFactsService virtual and then override them in the C# implementation. I'll try that out.

Note: once you do, please do a FAR on GenerateUniqueName to find the feautres that use it. Then try to write at least one test per feature that uses local-functions/parameters in a sensible manner. It will help ensure that we haven't regressed anything for those features (and perhaps will show that we've fixed a bug for them where they used to do things incorrectly).

dibarbet · 2019-05-24T02:07:33Z

@CyrusNajmabadi I refactored a tiny bit and added the check to the csharp generate unique name method. I additionally added tests that should address the concern around local function method names and other references to generate unique name.

CyrusNajmabadi · 2019-05-24T04:45:59Z

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs

            var rules = await document.GetNamingRulesAsync(FallbackNamingRules.CompletionOfferingRules, cancellationToken).ConfigureAwait(false);
            var result = new Dictionary<string, SymbolKind>();
            var semanticFactsService = context.GetLanguageService<ISemanticFactsService>();
+            var syntaxFactsService = context.GetLanguageService<ISyntaxFactsService>();


CyrusNajmabadi · 2019-05-24T04:46:09Z

...Features/CSharp/Portable/Completion/CompletionProviders/DeclarationNameCompletionProvider.cs

                                var uniqueName = semanticFactsService.GenerateUniqueName(
                                    context.SemanticModel,
-                                    context.TargetToken.Parent,
+                                    targetToken.Parent,


can be reverted? (up to you).

CyrusNajmabadi · 2019-05-24T04:46:36Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSemanticFactsService.cs

+            //     b) Local function symbols are only in scope inside the local function.
+            //        Relevant ones (e.g. method name) would be returned by a).
+            var symbolsInBlock = semanticModel.GetExistingSymbols(container, cancellationToken,
+                                    filter: (SyntaxNode n) => !n.IsKind(SyntaxKind.LocalFunctionStatement));


odd indentation. consider just doing +4 off of the var

can just say n =>

i haven't read the rest of this, but 'filter' reads weird. is this supposed to mean somethnig more like 'descendsInto'.

CyrusNajmabadi · 2019-05-24T04:50:07Z

src/Workspaces/Core/Portable/Shared/Extensions/SemanticModelExtensions.cs

                {
-                    GetAllDeclaredSymbols(semanticModel, child.AsNode(), symbols, cancellationToken);
+                    var childNode = child.AsNode();
+                    if (ApplyFilter(childNode, filter))


ah, based on thsi, we should really call the variable descendInto. I would also update the comment on the first file to say that we basically don't want to dive into a local function as they are not affected by outer variables, and do not contribute to the outer scope as well.

Yep, I couldn't think of a better name at the time, but descendInto is good. Will update!

CyrusNajmabadi · 2019-05-24T17:31:23Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSemanticFactsService.cs

+            // Walk through the enclosing block to find them, but avoid exploring local functions because
+            //     a) Visible local function symbols would be returned from LookupSymbols
+            //        (e.g. location is inside a local function or the local function method name).
+            //     b) Local function symbols are not affected by outer variables and do not contribute to the outer scope.


i think this could be worded slightly better. when you say "local function symbols" it sounds like you're talking about the local function itself. I think we should be clearer and say that "symbols declared inside a local function do not cause collisions with symbols declared outside teh local function. And so, should not be considered here.

Note: GetUsedSymbols is not a great name either. it should be something like GetCollidableSymbols since htat's what we're really asking for her.e We should also doc teh GetCollidableSymbls method to state what the intent of it is. That will help ensure that we have a clear path going forward in the future for how to fix it up if we get something wrong (or as the langauge changes).

CyrusNajmabadi

Have some tiny doc/naming suggestions. but otherwise things LGTM!

Approve
Submit feedback approving these changes.

mavasani · 2019-05-24T18:05:45Z

src/Workspaces/CSharp/Portable/LanguageServices/CSharpSemanticFactsService.cs

+            //        (e.g. location is inside a local function or the local function method name).
+            //     b) Symbols declared inside a local function do not cause collisions with symbols declared outside the local function, so avoid considering them.
+            var symbolsInBlock = semanticModel.GetExistingSymbols(container, cancellationToken,
+                descendInto: n => !n.IsKind(SyntaxKind.LocalFunctionStatement));


How about lambda functions?

Yeah, it looks like those don't collide either. So we should do the same. Note: i just realized this collision logic depends on language version. So we need to actually check the lang version and only do this if allowed.

How about lambda functions?

THis should be handled here - http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/Shared/Extensions/SemanticModelExtensions.cs,240 but I am working on writing a test for it. Encountering issues with have a lambda next to a markup span in test files...

Yeah, it looks like those don't collide either. So we should do the same. Note: i just realized this collision logic depends on language version. So we need to actually check the lang version and only do this if allowed.

I'm don't quite get this - do you mean that local functions only apply to C# 7? Wouldn't that automatically be handled because we'd never see the SyntaxKind.LocalFunctionStatement?

Feel free to hit me up on gitter, but allow me to explain:

For the purposes of explanation i'm going to make up a version number (since i don't actually know when things cahnged). So, in C# 6.0 this was illegal:

int i; Action a = () => { int i; }

This causes a collision between the two int i variables that hte language explicitly disallows. At some point (maybe 7.0, 7.1, 7.2, 7.3 or 8.0, i honestly don't know) we decided to relax this and state that there would be no collision here.

However, this helper function is being used to say "what are the symbols i woud collide with" so that features can decide what names are available. So, if you have this code:

$$ // <-- location of interest Action a = () => { int i; }

then in C# 6.0 we need to say "int i would collide here, so you can't use i". But in c# 7.0 we would say that "int i is not in scope here, so you can use i".

In effect, the decision we make (and what the APi returns) is language version dependent. The rules literally changed between versions, and that impacts what features are allowed to do.

Let me know if you need more info. Cheers!

Ahhh, I didn't realize that the first case used to be illegal. So yup it makes sense that we only want to exclude lambda functions based on the language version. I'll go ahead and figure out when that happened and update the PR.

That was a perfect explanation, thanks!

Glad to help! Note: to make your life easier, we have an IsAnonymousOrLocalFunction helper that you can use for this purpose.

According to my testing on VS, it would be C# 8 that relaxes those restrictions.

CyrusNajmabadi · 2019-05-24T22:12:54Z

will re-use it because I don't want to duplicate stuff, but unfortunately I think you just made my life much harder 😂
... since GenerateUniqueName only has the semantic model and syntax nodes, I'm going to have to pass in ISyntaxFactsService. It looks like there's a nice long chain I have to update from where I can retrieve the service to where it gets passed into GenerateUniqueName

You can just reference CSharpSyntaxFactsService.Instance. It's also reachable by this.SyntaxFactsService inside the CSharpSemanticsFactsService. Cheers!

dibarbet · 2019-05-24T22:14:25Z

will re-use it because I don't want to duplicate stuff, but unfortunately I think you just made my life much harder 😂
... since GenerateUniqueName only has the semantic model and syntax nodes, I'm going to have to pass in ISyntaxFactsService. It looks like there's a nice long chain I have to update from where I can retrieve the service to where it gets passed into GenerateUniqueName

You can just reference CSharpSyntaxFactsService.Instance. It's also reachable by this.SyntaxFactsService inside the CSharpSemanticsFactsService. Cheers!

Yep, I just realized that as soon as I posted my comment, so I deleted it. I'm just blind.

CyrusNajmabadi · 2019-05-24T22:16:03Z

I'm just blind.

👓 . Happens all the time. I swear i've gone looking for functions or code that were literally right on the original screen i started on :D

…is greater than or equal to c# 8.

dibarbet added the Area-IDE label May 23, 2019

dibarbet added this to the 16.2.P2 milestone May 23, 2019

dibarbet requested review from a team, heejaechang and mavasani May 23, 2019 18:36

CyrusNajmabadi reviewed May 23, 2019

View reviewed changes

dibarbet force-pushed the fix_completion_in_local_functions branch from cbacb18 to b4be2dc Compare May 24, 2019 02:03

Fix declaration name completion when local functions present.

64be2a3

dibarbet force-pushed the fix_completion_in_local_functions branch from b4be2dc to 64be2a3 Compare May 24, 2019 02:06

CyrusNajmabadi reviewed May 24, 2019

View reviewed changes

Rename filter to descendInto to better convey its meaning.

17a9a68

CyrusNajmabadi reviewed May 24, 2019

View reviewed changes

CyrusNajmabadi approved these changes May 24, 2019

View reviewed changes

Update documentation/naming for retrieving collidable symbols.

d8184b7

mavasani reviewed May 24, 2019

View reviewed changes

Include lambdas in generate unique name filter when language version …

b142192

…is greater than or equal to c# 8.

mavasani approved these changes May 24, 2019

View reviewed changes

dibarbet merged commit 5774395 into dotnet:master May 29, 2019

dibarbet deleted the fix_completion_in_local_functions branch May 29, 2019 17:12

Conversation

dibarbet commented May 23, 2019

Uh oh!

CyrusNajmabadi May 23, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dibarbet commented May 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 24, 2019 • 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!

dibarbet May 24, 2019 • 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 commented May 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dibarbet commented May 24, 2019

Uh oh!

CyrusNajmabadi commented May 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CyrusNajmabadi May 23, 2019 •

edited

Loading

CyrusNajmabadi May 24, 2019 •

edited

Loading

dibarbet May 24, 2019 •

edited

Loading

CyrusNajmabadi commented May 24, 2019 •

edited

Loading

CyrusNajmabadi commented May 24, 2019 •

edited

Loading