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

[VS] Get source text of an Entity symbol's signature for viewing metadata #11325

Merged
merged 9 commits into from
Mar 31, 2021

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Mar 30, 2021

Related to this: #11303

The PR from @baronfel : #11201 will probably be useful too.

At the moment, when trying to navigate to an external symbol, we decompile the DLL as C# code that the symbol exists in. This isn't particularly useful when you have a decompiled C# view of F# metadata; it isn't as helpful compared to what a F# signature view looks like.

What we really want to do is generate a signature (.fsi) file to be used as metadata viewing in VS for any symbol, C# or F#, as long as it was navigated from a F# document.

@runfoapp runfoapp bot mentioned this pull request Mar 30, 2021
@TIHan TIHan changed the title [VS][WIP] Generate signature of an Entity symbol for viewing metadata [VS] Generate signature of an Entity symbol for viewing metadata Mar 31, 2021
@TIHan
Copy link
Contributor Author

TIHan commented Mar 31, 2021

This is ready. We will iterate more on improving the signature generation in separate PRs, but this is already much more useful than decompiling to C#.

@TIHan TIHan changed the title [VS] Generate signature of an Entity symbol for viewing metadata [VS] Get signature source text of an Entity symbol for viewing metadata Mar 31, 2021
@TIHan TIHan changed the title [VS] Get signature source text of an Entity symbol for viewing metadata [VS] Get metadata source text of an Entity symbol's signature for viewing metadata Mar 31, 2021
@TIHan TIHan changed the title [VS] Get metadata source text of an Entity symbol's signature for viewing metadata [VS] Get source text of an Entity symbol's signature for viewing metadata Mar 31, 2021
Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Looks good overall, some questions though

@@ -1424,7 +1424,7 @@ module private TastDefinitionPrinting =

let nameL = eventTag |> wordL
let typL = layoutType denv (e.GetDelegateType(amap, m))
staticL ^^ WordL.keywordEvent ^^ nameL ^^ WordL.colon ^^ typL
staticL ^^ WordL.keywordMember ^^ nameL ^^ WordL.colon ^^ typL
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice find

@@ -125,14 +125,14 @@ type internal FSharpDocumentDiagnosticAnalyzer
|> RoslynHelpers.StartAsyncAsTask cancellationToken

member this.AnalyzeSemanticsAsync(document: Document, cancellationToken: CancellationToken): Task<ImmutableArray<Diagnostic>> =
if document.Project.IsFSharpMiscellaneousOrMetadata && not document.IsFSharpScript then Task.FromResult(ImmutableArray.Empty)
Copy link
Contributor

Choose a reason for hiding this comment

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

Typically here we would move this into the computation expression and say

do! (not document.Project.IsFSharpMiscellaneousOrMetadata || document.IsFSharpScript)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

asyncMaybe doesn't support a bind operation for just bool, though we could add it.

I would also find it confusing for it to be used in that way. A simple if/then is very clear comparatively.

asyncMaybe itself is a good abstraction so you don't have to constantly keep handling the None case, but that's all that it should do.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does, it's do! Option.guard expr where expr is a bool-returning thing. This is the canonical way that invariants are held in this layer of the tools. I'll submit a separate PR for this, since we should be consistent

@@ -34,6 +34,9 @@ type internal SimplifyNameDiagnosticAnalyzer
interface IFSharpSimplifyNameDiagnosticAnalyzer with

member _.AnalyzeSemanticsAsync(descriptor, document: Document, cancellationToken: CancellationToken) =
if document.Project.IsFSharpMiscellaneousOrMetadata && not document.IsFSharpScript then Tasks.Task.FromResult(ImmutableArray.Empty)
Copy link
Contributor

Choose a reason for hiding this comment

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

Typically here we would move this into the computation expression and say

do! (not document.Project.IsFSharpMiscellaneousOrMetadata || document.IsFSharpScript)

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops, this should be Option.guard (...)

@@ -24,6 +24,9 @@ type internal UnusedDeclarationsAnalyzer
interface IFSharpUnusedDeclarationsDiagnosticAnalyzer with

member _.AnalyzeSemanticsAsync(descriptor, document, cancellationToken) =
if document.Project.IsFSharpMiscellaneousOrMetadata && not document.IsFSharpScript then Threading.Tasks.Task.FromResult(ImmutableArray.Empty)
Copy link
Contributor

Choose a reason for hiding this comment

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

Typically here we would move this into the computation expression and say

do! (not document.Project.IsFSharpMiscellaneousOrMetadata || document.IsFSharpScript)

@@ -38,6 +38,9 @@ type internal UnusedOpensDiagnosticAnalyzer
interface IFSharpUnusedOpensDiagnosticAnalyzer with

member _.AnalyzeSemanticsAsync(descriptor, document: Document, cancellationToken: CancellationToken) =
if document.Project.IsFSharpMiscellaneousOrMetadata && not document.IsFSharpScript then Tasks.Task.FromResult(ImmutableArray.Empty)
Copy link
Contributor

Choose a reason for hiding this comment

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

Typically here we would move this into the computation expression and say

do! (not document.Project.IsFSharpMiscellaneousOrMetadata || document.IsFSharpScript)

let! _, _, projectOptions = projectInfoManager.TryGetOptionsForDocumentOrProject (tmpShownDoc, cancellationToken, userOpName)
let! _, _, checkResults = checkerProvider.Checker.ParseAndCheckDocument(tmpShownDoc, projectOptions, userOpName)
let! r =
let rec areTypesEqual (ty1: FSharpType) (ty2: FSharpType) =
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with the commment below. We should file an issue to figure out a way to lift this into FCS proper.

Copy link
Contributor Author

@TIHan TIHan Mar 31, 2021

Choose a reason for hiding this comment

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

Equality does exist for symbols, but the symbols from the generated metadata may be malformed, or just not exactly match the requirements for the existing equality in FCS. Basically, we need NicePrint to generate signatures that have symbols that are equatable. I think we will get there, especially when we start tweaking it for generating better signatures of implementation files

}

let span =
match Async.RunSynchronously(goToAsync, cancellationToken = cancellationToken) with
Copy link
Contributor

Choose a reason for hiding this comment

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

This has my ears raised a bit. Is there an awaitable thing we could potentially plug this into? Naiive question and I know that this whole thing runs async anyways, but I fear that something in VS will cause us to run in the UI thread and we'd introduce a UI freeze. I don't have anything concrete to test this with though, and so my concern there might be meaningless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TryGoToDefinition is not awaitable, even in Roslyn from last I checked. It must block, but while it's blocked it will show the animating status bar which is cancellable.

The calls made in the asyncMaybe block don't call into anything that requires the UI thread, it just calls FCS async functions and gets the source text from the document.

@TIHan TIHan merged commit 2d4a8fc into dotnet:main Mar 31, 2021
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
…data (dotnet#11325)

* Initial work for generating a signature of a given symbol for viewing metadata

* Do not output the 'event' keyword

* Adding open paths and fixing surface area

* Fixing tests. Adding more extensions to Project and Document. Better handling of determining if we should do semantic diagnostics for a given file.

* Trying to go to definition on metadata. Managing metadata as source projects.

* Navigating to external symbol

* ignoring tests

* Renaming TryGenerateSignatureText to TryGetMetadataText

* Fixing namespace and some parts of module
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants