Skip to content

Include local-functions in the go-to index.#61568

Merged
CyrusNajmabadi merged 20 commits intodotnet:mainfrom
CyrusNajmabadi:localFuncindexing
Jun 4, 2022
Merged

Include local-functions in the go-to index.#61568
CyrusNajmabadi merged 20 commits intodotnet:mainfrom
CyrusNajmabadi:localFuncindexing

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi commented May 27, 2022 •
edited
Loading

Fixes #39083

These have risen in priority, esp. as they may be used a lot in a top-level program.

@ghost ghost added the Area-IDE label May 27, 2022
@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review May 27, 2022 21:56
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner May 27, 2022 21:56
@ryzngard
Copy link
Copy Markdown
Contributor

ryzngard commented May 27, 2022

Perf concerns aside, I have some usability concerns here. There's currently no way to filter out local functions from other members. That could easily mean that common local function names that coincide with member names will be hard to sort through...

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

CyrusNajmabadi commented May 31, 2022

Perf concerns aside, I have some usability concerns here. There's currently no way to filter out local functions from other members. That could easily mean that common local function names that coincide with member names will be hard to sort through...

I'm not hugely concerned here as i don't expect local function names to have a particularly higher coincidence of collision with member names vs just having overloads. We could try this out and see if there's an issue :)

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

CyrusNajmabadi commented Jun 1, 2022

@dotnet/roslyn-ide this is ready for review.

akhera99
akhera99 reviewed Jun 3, 2022
View reviewed changes
...le/LanguageServices/DeclaredSymbolFactoryService/AbstractDeclaredSymbolInfoFactoryService.cs
protected abstract void AddMemberDeclarationInfos(
SyntaxNode container, TMemberDeclarationSyntax memberDeclaration, StringTable stringTable, ArrayBuilder<DeclaredSymbolInfo> declaredSymbolInfos, string containerDisplayName, string fullyQualifiedContainerName);
protected abstract void AddLocalFunctionInfos(
TMemberDeclarationSyntax memberDeclaration, StringTable stringTable, ArrayBuilder<DeclaredSymbolInfo> declaredSymbolInfos, string containerDisplayName, string fullyQualifiedContainerName, CancellationToken cancellationToken);
Copy link
Copy Markdown
Member

@akhera99 akhera99 Jun 3, 2022

Choose a reason for hiding this comment

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

I think this makes a lot more sense to read than what it was previously

akhera99
akhera99 approved these changes Jun 3, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@akhera99 akhera99 left a comment

Choose a reason for hiding this comment

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

Looks alright to me

@CyrusNajmabadi CyrusNajmabadi merged commit b1fbb44 into dotnet:main Jun 4, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the localFuncindexing branch June 4, 2022 00:03
@ghost ghost added this to the Next milestone Jun 4, 2022
@RikkiGibson RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022
dibarbet added a commit to dibarbet/roslyn that referenced this pull request Aug 11, 2022
@dibarbet
Revert "Merge pull request dotnet#61568 from CyrusNajmabadi/localFunc…
2fe5202 
…indexing"

This reverts commit b1fbb44, reversing
changes made to d79fe4e.
JoeRobich added a commit that referenced this pull request Aug 12, 2022
@JoeRobich
Merge pull request #63344 from dibarbet/revert_local_func_index
6ad672d 
Revert "Merge pull request #61568 from CyrusNajmabadi/localFuncindexing"
333fred added a commit that referenced this pull request Aug 12, 2022
@333fred
Merge remote-tracking branch 'upstream/release/dev17.3' into merges/r…
38e0c67 
…elease/dev17.3-to-release/dev17.4

* upstream/release/dev17.3:
  Revert "Merge pull request #61568 from CyrusNajmabadi/localFuncindexing"
  Backport some integration test skips to 17.3
  Mark shipped APIs (#63244)
333fred added a commit that referenced this pull request Aug 13, 2022
@333fred
Merge remote-tracking branch 'upstream/release/dev17.4' into merges/r…
8d7675e 
…elease/dev17.4-to-main

* upstream/release/dev17.4:
  Revert "Merge pull request #61568 from CyrusNajmabadi/localFuncindexing"
  Backport some integration test skips to 17.3
  Mark shipped APIs (#63244)
Cosifne added a commit that referenced this pull request Aug 18, 2022
@Cosifne
Revert "Revert "Merge pull request #61568 from CyrusNajmabadi/localFu…
92d8333 
…ncindexing""
Cosifne added a commit to Cosifne/roslyn that referenced this pull request Aug 18, 2022
@Cosifne
Revert "Revert "Merge pull request dotnet#61568 from CyrusNajmabadi/l…
2f617ae 
…ocalFuncindexing""

This reverts commit 2fe5202.
Cosifne added a commit that referenced this pull request Aug 18, 2022
@Cosifne
Revert and fix the stackoverflow when search local function. (#63473)
fe84827 
* Revert "Revert "Merge pull request #61568 from CyrusNajmabadi/localFuncindexing""

This reverts commit 2fe5202.

* Change to use BFS

* Use pool

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

Reviewers

@akhera99 akhera99 akhera99 approved these changes

@davidwengier davidwengier Awaiting requested review from davidwengier

@ryzngard ryzngard Awaiting requested review from ryzngard

Assignees

No one assigned

Labels

Area-IDE

Projects

None yet

Milestone

17.3 P3

Development

Successfully merging this pull request may close these issues.

'Go to all' can't find local functions

4 participants

@CyrusNajmabadi @ryzngard @akhera99 @RikkiGibson