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

[WIP] Using MiscellaneousFilesWorkspace to handle single files #5733

Closed
wants to merge 4 commits into from

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Oct 4, 2018

Edited(New):
We now use the MiscellaneousFilesWorkspace properly for single files. This solves a lot of issues we had with script files and gets rid of cruft.

Original:
Refactoring how we handle single files in the workspace. Eventually, this will be switched over to Roslyn's Miscellaneous Workspace.

@dsyme
Copy link
Contributor

dsyme commented Oct 5, 2018

@TIHan What validation have you done for this one, e.g.

  • Create new .fsx script - both before and after loading an F# project - does it work, give errors, intellisense etc?
  • Open existing .fsx script - both before and after loading an F# project - does it work, give errors, intellisense etc? Does it work if the script is in a project with build action None and/or Compile?
  • Rename script - this is broken today so we don't expect it to start working

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Looks like a decent refactoring.

@cartermp can you run this through it's paces.

@TIHan
Copy link
Contributor Author

TIHan commented Oct 5, 2018

@dsyme

I've tested the scenarios:

  • Create new .fsx script, make sure tooling works. Then create a project, then create a .fsx to verify tooling works.
  • Rename script is still broken.

I will do more testing to verify more behavior.

The changes are just a refactoring rather than a re-design.

@smoothdeveloper
Copy link
Contributor

Feels like ground work to integrate #4122 is ongoing 🙂 the changes here are definitely not something I'd would have come up with given 0 exposure to roslyn.

@saul
Copy link
Contributor

saul commented Oct 8, 2018

@TIHan see my changes here: saul@eedfcea#diff-cdf0a4fbb7615b8029d78133924259c3

I fixed .fsx script opening and renaming a while back, by essentially copying/pasting MiscellaneousFilesWorkspace out of Roslyn. The pertinent parts are in the IVsRunningDocTableEvents implementation. I had to do this at the time as MiscellaneousFilesWorkspace was internal to Roslyn, but in 15.3 it got made public - we should definitely just use that. It removes a load of cruft that we currently have.

@TIHan
Copy link
Contributor Author

TIHan commented Oct 8, 2018

@saul that is some good work. Yes, we should use MiscellaneousFilesWorkspace. I'll see if we can just use it, but I have a feeling we might have to wait for dotnet/roslyn#26931.

@TIHan
Copy link
Contributor Author

TIHan commented Oct 8, 2018

Getting MiscellaneousFilesWorkspace to work was actually quite easy and fixes issues with script files rename as well. Actually, this solves a lot of problems.

This changed from a refactor to a different design (a much better one).

@cartermp
Copy link
Contributor

cartermp commented Oct 8, 2018

@TIHan and I validated:

  • Renaming script file
  • Renaming source to script
  • Renaming script to source

@TIHan TIHan changed the title Refactoring single file workspace handling out of language service Using MiscellaneousFilesWorkspace to handle single files Oct 8, 2018
@TIHan TIHan closed this Oct 8, 2018
@TIHan TIHan reopened this Oct 8, 2018
@TIHan TIHan changed the title Using MiscellaneousFilesWorkspace to handle single files [WIP] Using MiscellaneousFilesWorkspace to handle single files Oct 8, 2018
@TIHan
Copy link
Contributor Author

TIHan commented Oct 9, 2018

So, I've received some information regarding the MiscellaneousFilesWorkspace. It doesn't kick off analyzers. This means no error + warning reporting, unused declaration/opens, etc. This is a big problem for us. After some discussion, it may be that the MiscellaneousFilesWorkspace is actually not right for us and we were almost doing the right thing to begin with.

@saul
Copy link
Contributor

saul commented Oct 9, 2018

@TIHan You can replicate the miscellaneous files workspace using IVsRunningDocumentTable as I did in my PR.

@TIHan
Copy link
Contributor Author

TIHan commented Oct 9, 2018

@saul that is what I'm going to look into.

@TIHan
Copy link
Contributor Author

TIHan commented Oct 10, 2018

After a conversation with @tmat , it is better for us to change Roslyn's MiscellaneousFilesWorkspace to understand semantic analysis for our script files. That way, we are not re-implementing what is already there, such as using VS's running document table events; I'd rather not have to maintain that sort of fragile code for F#.

(I'm out on vacation for a week, so don't expect progress until I am back)

@cartermp cartermp closed this Oct 10, 2018
@cartermp cartermp reopened this Oct 10, 2018
@dsyme
Copy link
Contributor

dsyme commented Oct 12, 2018

My main request for this sort of thing is to make sure we haven't regressed performance on large solution load. And also scripts with large numbers of transitive references to other scripts and assemblies.

See tests\projects\stress for the former. We should add a stress test there for scripts with large numbers of transitive references to other scripts and assemblies

@TIHan
Copy link
Contributor Author

TIHan commented Oct 18, 2018

@dsyme I agree. We definitely shouldn't regress performance. Adding stress tests would help us ensure that.

@TIHan
Copy link
Contributor Author

TIHan commented Oct 23, 2018

I'm closing this as we are going to make similar changes in the dev16.0 branch.

@TIHan TIHan closed this Oct 23, 2018
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.

6 participants