Fix #16761 #17047

vzarytovskii · 2024-04-15T16:24:17Z

Description

Fixes #16761. Still missing navigating to specific range (at least I think so, needs more testing). Also not very easy to test this thing, since interop suite is nightmare to write and run locally.

TL;DR - now roslyn uses different MEF interface for navigation for F#, which before didn't have an ability to navigate to externals, this adds it - by navigating to external doc explicitly and not returning back metadata to roslyn. It also makes sure we initiate navigation on the UI thread, so we can grab needed VS services.

It's kinda a dirty workaround for the issue, but hey, it fixes navigation.

github-actions · 2024-04-15T16:24:58Z

❗ Release notes required

@vzarytovskii,

Caution

No release notes found for the changed paths (see table below).

Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format.

The following format is recommended for this repository:

* <Informative description>. ([PR #XXXXX](https://github.com/dotnet/fsharp/pull/XXXXX))

See examples in the files, listed in the table below or in th full documentation at https://fsharp.github.io/fsharp-compiler-docs/release-notes/About.html.

If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

You can open this PR in browser to add release notes: open in github.dev

Change path	Release notes path	Description
`vsintegration/src`	docs/release-notes/.VisualStudio/17.11.md	No release notes found or release notes format is not correct

T-Gro · 2024-04-16T08:36:31Z

vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs

+                if not res then
+                    return ImmutableArray.empty
+                else
+                    // TODO: return something? Maybe cancelled task?


I don't think cancelled task would be any better here.

I'm not sure tbh what would be a good return value here.

The empty immarray feels appropriate.
It probably is an errornous situation, but might as well come from stale data.

Idea:
Send this situation to telemetry reporter.

The empty immarray feels appropriate.

Empty array will lead to message which will say something like "failed to find symbol", or it leads now.

It probably is an errornous situation, but might as well come from stale data.

Not really. We return empty array when we didn't find anything. In case of external navigation, we need to handle it ourselves and somehow tell Roslyn "nothing found but it's fine", that's why I thought of cancelled task - we cancel Roslyn navigation and deal with it ourselves.

Oh you are right, I misread to which branch this belongs.
I meant to log telementry in case we did not find anything ourselves.

If we did find it, cancelling it seems like a good approach, indeed.

vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs

vzarytovskii · 2024-04-17T15:08:26Z

So, two things

Returning empty result seems to work fine even when we managing navigation ourselves.
Using provided cancellation token seems to be "cancelled" once we navigating inside the document. Not using it seems to work fine.

@T-Gro @psfinaki @KevinRansom are you okay with points above? Meaning if I leave them as I described?

I'm personally okay with not using CT inside the document. LSP will have different implementations for both internal and external implementations.

Co-authored-by: vzarytovskii <1260985+vzarytovskii@users.noreply.github.com>

KevinRansom · 2024-04-17T17:57:38Z

Yes I'm fine with that.

* Disallow calling abstract methods directly on interfaces (#17021) * Disallow calling abstract methods directly on interfaces * More tests * IWSAMs are not supported by NET472 * Update src/Compiler/Checking/ConstraintSolver.fs Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com> * fix typos * looking for the right check * Add comments * move release notes * Add a new error number and message * Update docs/release-notes/.FSharp.Compiler.Service/8.0.400.md Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com> * Update docs/release-notes/.FSharp.Compiler.Service/8.0.400.md Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com> * Improve error message --------- Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com> Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com> * Always use `typeEquivAux EraseMeasures` (for integral range optimizations) (#17048) * Always use `typeEquivAux EraseMeasures` * Update release notes * Update baselines --------- Co-authored-by: Petr <psfinaki@users.noreply.github.com> * Error message that explicitly disallowed static abstract members in classes. (#17055) * WIP * Error message that explicitly disallowed static abstract methods in abstract classes * release notes * SynTypeDefnKind.Class * Fix #16761 (#17047) * Fix #16761 * Fully async version + ignore cancellation on external navigation * Automated command ran: fantomas Co-authored-by: vzarytovskii <1260985+vzarytovskii@users.noreply.github.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --------- Co-authored-by: Edgar Gonzalez <edgar.gonzalez@fundourselves.com> Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com> Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com> Co-authored-by: Petr <psfinaki@users.noreply.github.com> Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>

kerams · 2024-04-21T13:50:26Z

I'm getting unwanted navigation when holding Ctrl and hovering over operators like && or >. Not sure if caused by this PR or a bug in VS.

vzarytovskii · 2024-04-21T14:29:44Z

I'm getting unwanted navigation when holding Ctrl and hovering over operators like && or >. Not sure if caused by this PR or a bug in VS.

This pr didn't make it to any product preview yet. It should be 17.11

kerams · 2024-04-21T14:53:36Z

I have built my own VSIX from main.

vzarytovskii · 2024-04-22T10:42:50Z

I have built my own VSIX from main.

Then it might be happening if roslyn calls us in this case. We don't initiate any navigation ourselves. Might check it when I have time later this week. @kerams could you please create an issue?

Fix dotnet#16761

9264c4a

vzarytovskii requested a review from a team as a code owner April 15, 2024 16:24

T-Gro reviewed Apr 16, 2024

View reviewed changes

T-Gro approved these changes Apr 16, 2024

View reviewed changes

psfinaki approved these changes Apr 16, 2024

View reviewed changes

vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs Outdated Show resolved Hide resolved

KevinRansom approved these changes Apr 16, 2024

View reviewed changes

Fully async version + ignore cancellation on external navigation

ae085a8

vzarytovskii force-pushed the goto-ext-def-fix branch from f00c88d to ae085a8 Compare April 17, 2024 15:39

dotnet deleted a comment from github-actions bot Apr 17, 2024

vzarytovskii and others added 2 commits April 17, 2024 17:43

Merge branch 'main' into goto-ext-def-fix

67c19e5

Automated command ran: fantomas

b669b9b

Co-authored-by: vzarytovskii <1260985+vzarytovskii@users.noreply.github.com>

dotnet deleted a comment from github-actions bot Apr 17, 2024

KevinRansom merged commit 587bc1e into dotnet:main Apr 17, 2024

vzarytovskii mentioned this pull request May 22, 2024

I'm getting unwanted navigation when holding Ctrl and hovering over operators like && or >. Not sure if caused by this PR or a bug in VS. #17230

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix #16761 #17047

Fix #16761 #17047

Uh oh!

vzarytovskii commented Apr 15, 2024

Uh oh!

github-actions bot commented Apr 15, 2024 •

edited

Loading

Uh oh!

T-Gro Apr 16, 2024

Uh oh!

vzarytovskii Apr 16, 2024

Uh oh!

T-Gro Apr 16, 2024 •

edited

Loading

Uh oh!

vzarytovskii Apr 16, 2024

Uh oh!

T-Gro Apr 16, 2024

Uh oh!

Uh oh!

vzarytovskii commented Apr 17, 2024 •

edited

Loading

Uh oh!

KevinRansom commented Apr 17, 2024

Uh oh!

kerams commented Apr 21, 2024

Uh oh!

vzarytovskii commented Apr 21, 2024

Uh oh!

kerams commented Apr 21, 2024 •

edited

Loading

Uh oh!

vzarytovskii commented Apr 22, 2024

Uh oh!

Uh oh!

Fix #16761 #17047

Fix #16761 #17047

Uh oh!

Conversation

vzarytovskii commented Apr 15, 2024

Description

Uh oh!

github-actions bot commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❗ Release notes required

Uh oh!

T-Gro Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

vzarytovskii Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

T-Gro Apr 16, 2024 • 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!

vzarytovskii Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

T-Gro Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vzarytovskii commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KevinRansom commented Apr 17, 2024

Uh oh!

kerams commented Apr 21, 2024

Uh oh!

vzarytovskii commented Apr 21, 2024

Uh oh!

kerams commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vzarytovskii commented Apr 22, 2024

Uh oh!

Uh oh!

github-actions bot commented Apr 15, 2024 •

edited

Loading

T-Gro Apr 16, 2024 •

edited

Loading

vzarytovskii commented Apr 17, 2024 •

edited

Loading

kerams commented Apr 21, 2024 •

edited

Loading