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

Fix finding references to a type when there's a module with the same name #16081

Merged
merged 14 commits into from
Nov 2, 2023

Conversation

0101
Copy link
Contributor

@0101 0101 commented Oct 5, 2023

Fixes #14411

No questions allowed.

@0101 0101 requested a review from a team as a code owner October 5, 2023 13:33
@0101
Copy link
Contributor Author

0101 commented Oct 5, 2023

Ok, looks like it's not going to be that easy :)

@0101 0101 changed the title fix Fix finding references to a type when there's a module with the same name Oct 5, 2023
@0101
Copy link
Contributor Author

0101 commented Oct 27, 2023

The Problem

So what's happening here is, when searching for modules we report what we find to the name resolution sink. Even when the identifier could still refer to a type with the same name. And from there it's written to ItemKeyStore.

// Otherwise modules are searched first. REVIEW: modules and types should be searched together.
// For each module referenced by 'id', search the module as if it were an F# module and/or a .NET namespace.
let moduleSearch ad () =
ResolveLongIdentAsModuleOrNamespaceThen sink ResultCollectionSettings.AtMostOneResult ncenv.amap m fullyQualified nenv ad id rest isOpenDecl
(ResolveExprLongIdentInModuleOrNamespace ncenv nenv typeNameResInfo ad)

Attempt 1

There is a comment there already suggesting that modules and types should be searched together. I tried that with the existing mechanisms - to continue the search even when we've found a module by that name. But that seems to be neither necessary nor fix the issue. Which is that once we call the sink with whatever we identified first, it stays there.

Attempt 2

Another thing I tried is to replace the module item with the type when we've found it by using CallNameResolutionSinkReplacing here:

CallNameResolutionSink sink (m, nenv, item, emptyTyparInst, occ, ad))

However that did have a bunch of side effects one of them being that type names got replaced by new in some tooltips. Maybe this would be feasible if we somehow narrow down the scope of what we replace...?

Attempt 3

The alternative, which is what's currently in this PR, is to suppress calling the name resolution sink from here when we're searching for module names:

notifyNameResolution modref id.idRange

This fixes the problem and only has 2 side effects:

  1. The ordering of GetAllUsesOfAllSymbolsInFile is slightly changed in some cases. But as far as I can tell there was no specific ordering to begin with.
  2. In case of invalid long identifiers, parts of them which are theoretically valid can be missing from GetAllUsesOfAllSymbolsInFile - which manifests in this test - the last occurrence of Threading is missing there. Though it still works fine in the editor:
    Screenshot 2023-10-27 at 9 27 07

So I think this is a good trade off and it doesn't require any drastic changes. It is a bit ugly though.

Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Mhm yeah let's see if those are the only side effects :D

src/Compiler/Checking/NameResolution.fsi Outdated Show resolved Hide resolved
tests/service/EditorTests.fs Outdated Show resolved Hide resolved
@0101
Copy link
Contributor Author

0101 commented Oct 31, 2023

Mhm yeah let's see if those are the only side effects :D

Yeah, I should probably clarify that those are the only side effects covered by our test suite :)

@0101 0101 merged commit 70bd177 into dotnet:main Nov 2, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

FindReferencesInFile misses references to type when a module with the same name exists
3 participants