Add resolvers to misc workspace script projects #17263
Conversation
|
@dotnet/roslyn-ide I couldn't find any MiscellaneousFilesWorkspace tests. Do we have any? |
tmat
force-pushed
the
ScriptWorkspace
branch
from
bd42c0f to
52bfbab
Compare
|
@tmat: we test it in integration tests. |
| var compilationFactory = languageServices.GetService<ICompilationFactoryService>(); | ||
| var compilationOptions = compilationFactory.GetDefaultCompilationOptions(); | ||
| var syntaxTreeFactory = languageServices.GetService<ISyntaxTreeFactoryService>(); | ||
| var parseOptions = syntaxTreeFactory.GetDefaultParseOptions(); |
There was a problem hiding this comment.
We should check if TypeScript or F# or others are using this workspace. I'm unsure, but it looks terrifyingly possible that they could have allowed ParseOptions to be null in that case and now we'd be crashing for not having these services. @brettfo? @paulvanbrenk?
There was a problem hiding this comment.
It doesn't look like we use the MiscFilesWorkspace in VS. However we don't implement the ICompilationFactoryService, or the ISyntaxTreeFactoryService, so I suggest bailing from this method early in case either is null or at least put in a bunch of asserts so we can discover what's happening when there is a crash.
FYI @vladima and @minestarks
|
|
||
| if (Path.GetExtension(moniker) == languageInformation.ScriptExtension) | ||
| var languageServices = Services.GetLanguageServices(languageInformation.LanguageName); | ||
| var compilationFactory = languageServices.GetService<ICompilationFactoryService>(); |
There was a problem hiding this comment.
GetRequiredService so we get useful exceptions.
| var syntaxTreeFactory = languageServices.GetService<ISyntaxTreeFactoryService>(); | ||
| var parseOptions = syntaxTreeFactory.GetDefaultParseOptions(); | ||
|
|
||
| if (PathUtilities.GetExtension(moniker) == languageInformation.ScriptExtension) | ||
| { | ||
| parseOptions = parseOptions.WithKind(SourceCodeKind.Script); |
There was a problem hiding this comment.
VB CompilationOptions also contain the ParseOptions, so I suspect you need to update CompilationOptions too, or leave a comment why the stuff that is impacted by that scenario doesn't matter here. I suspect it doesn't, but the comment would be good.
|
Overall this looks good, but we need confirmation this doesn't break F# and TypeScript before I'll hit approve. We should also make it build too! |
|
Ditto @jasonmalinowski's concerns. |
tmat
force-pushed
the
ScriptWorkspace
branch
from
52bfbab to
ef07f4b
Compare
|
I changed the code to work in absence of ICompilationFactoryService and ISyntaxTreeFactoryService as well. |
|
@jasonmalinowski Looks good? |
| { | ||
| parseOptions = parseOptions.WithKind(SourceCodeKind.Script); | ||
| parseOptionsOpt = parseOptionsOpt.WithKind(SourceCodeKind.Script); |
There was a problem hiding this comment.
Does this need a ?.WithKind?
| Debug.Assert(newProject.Solution.Workspace.Kind == "Interactive"); | ||
| continue; | ||
| Debug.Assert(newProject.Solution.Workspace.Kind == WorkspaceKind.Interactive || newProject.Solution.Workspace.Kind == WorkspaceKind.MiscellaneousFiles); | ||
| continue; |
|
When are we expecting this to be available to F#? We need the changes to remove the ParseOptions argument from RegisterLanguage to be able to use MiscellaneousFilesWorkspace from F#. (ParseOptions has an internal constructor and the F# language service is not on the IVT list.) |
|
@jasonmalinowski is this now available in the 15.2 preview? |
|
Note: 15.2 is not a preview, it's the current release of Visual Studio 2017. This fix is NOT included in the 15.2 release (we took only a tiny handful of crashing bugs in 15.2). It is in the Visual Studio 2017 Preview version 15.3 that was released to the preview channel on Wednesday though. |
|
Apologies - I meant 15.3! Thanks @Pilchie - this means we can use the miscellaneous files workspace in F# now, so we can get rid of the kludge we have for script files. |
|
Do you know where I can find the 15.3 Microsoft.VisualStudio.LanguageServices binaries? NuGet's latest version is 2.1.0 (I've no idea if that's the 15.3 binary) |
|
@saul It's not; we weren't expecting to upload these but makes sense, so we've started the internal process now. (FYI @shyamnamboodiripad.) |
|
@jasonmalinowski is that to say that Pilchie was correct, in that this code will be in 15.3? And that you're saying these changes aren't in the latest NuGet binaries? |
Misc workspace was using default compilation options without metadata and source resolvers, which caused lack of intellisense in .csx files.
This change just adds basic resolvers -- more work is needed to set them up in the same way the csi and Interactive Window do, but this is a start.
Fixes #13888