Skip to content

Conversation

@KevinRansom
Copy link
Contributor

Fixes dotnet/project-system#3195 (comment)

When deleting a file for a newly created F# project causes a Watson, and VS crashes and restarts.

The error happens because VS is in the process of loading and instantiating project, and at the same time starts the editor. Our implementation of SetupNewTextView, queries the workspace to see if the project contains this document. Unfortunately since the project is not yet fully instantiated, the workspace doesn't have information about the document. The document will be added to the workspace and project as the project load proceeds normally, and workspace events will cause us to wire up intellisense correctly.

We need to ensure that we still enable the miscellaneous files behavior for loose F# documents.

The fix is to verify that hierarchy retrieved from the running document table for the document has the capability "CPS". I.e. is a cps project.

If the document is associated with a CPS project we can be sure that necessary workspace events are on their way, and we can leave SetupNewTextView without doing any intellisense wireup, because it will be handled in the workspace events.

@brettfo, @Pilchie --- this fixes an AV does it meet the bar for 15.6

Will, brett, we need to try this out to make sure there are no other holes.

Kevin

@KevinRansom KevinRansom requested review from Pilchie and TIHan February 7, 2018 03:30
| null ->
let fileContents = VsTextLines.GetFileContents(textLines, textViewAdapter)
this.SetupStandAloneFile(filename, fileContents, this.Workspace, hier)
if not (h.IsCapabilityMatch("CPS")) then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this is as narrow as possible. We could widen this so that we do nothing even if the file is in the project. However, this will always occur before any events, I believe even before the solution load event, which means we can start intellisense at the earliest possible moment.

I think it's marginal, and so I could be persuaded to widen this.

Copy link
Member

Choose a reason for hiding this comment

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

I believe that VS's solution events have fired, but Roslyn's VisualStudioProjectTracker hasn't pushed the project to the Workspace yet, causing the check above to fail.

With this fix, how/when does the project system get updated for CPS projects?

Can/should we be calling AbstractProject.StartPushingToWorkspaceAndNotifyOfOpenDocuments like Roslyn does when a file is opened?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OnDocumentOpened is not invoked for C# when a project is opened with an open file. It occurs when a File is specifically opened with the open command.

A longer term approach is to move to the MiscellaneousFileWorkspace events for loose files. And remove the need for SetupNewTextView for any of our CPS based files. But that is a riskier approach, and should probably happen alongside moving to CPS even for our legacy project files, but that has implications for flavoured projects.

Anyway as you can see there is a lot further to go with F# project loading and initialization. This particular fix is the narrowest fix that addresses the issue and doesn't take back any currently "working scenarios".

Kevin

@Pilchie
Copy link
Member

Pilchie commented Feb 7, 2018

From the bug, this isn't an AV, it's an error dialog that is handled, and only on project creation, not re-open, right?

Copy link
Contributor

@TIHan TIHan left a comment

Choose a reason for hiding this comment

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

This fix is a small change and does indeed solve our issue. At some point we need to re-visit this file later to cleanup some of this stuff.

Otherwise, looks good.

| null ->
let fileContents = VsTextLines.GetFileContents(textLines, textViewAdapter)
this.SetupStandAloneFile(filename, fileContents, this.Workspace, hier)
if not (h.IsCapabilityMatch("CPS")) then
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest a comment describing why we are doing this so we don't forget :)

@KevinRansom KevinRansom merged commit 5d666fa into dotnet:dev15.6 Feb 9, 2018
KevinRansom added a commit that referenced this pull request Feb 9, 2018
This reverts commit 5d666fa.
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
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.

3 participants