Use IVsSolution to look up IVsHierarchy by project GUID#35746
Use IVsSolution to look up IVsHierarchy by project GUID#35746tmat merged 6 commits intodotnet:masterfrom
Conversation
|
@jasonmalinowski Looks reasonable? |
jasonmalinowski
left a comment
There was a problem hiding this comment.
High level this seems reasonable. I haven't done a careful analysis of things to see if there's small bugs.
Testing this will be fun. Things to test:
- ASP.NET Web Sites, both script blocks and code in App_Code. Maybe also things like generic handlers.
- ASP.NET Web Applications, both script blocks and regular code files.
- ASP.NET MVC/Razor of however many flavors.
I'm also not sure if flavored csproj projects can do silly stuff here either. I hope not, more just because flavoring is insane in general.
src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs
Outdated
Show resolved
Hide resolved
jasonmalinowski
left a comment
There was a problem hiding this comment.
Some questions, but mostly either stylistic things or just "wanted to confirm that's what you meant" things. I think this is fine, modulo heavy testing of course.
| // *DO NOT DELETE* | ||
| // This is used by Ruleset Editor from ManagedSourceCodeAnalysis.dll. | ||
| public IReadOnlyDictionary<string, IEnumerable<DiagnosticDescriptor>> GetAllDiagnosticDescriptors(IVsHierarchy hierarchyOpt) | ||
| => GetAllDiagnosticDescriptors(hierarchyOpt != null && hierarchyOpt.TryGetProjectGuid(out var guid) ? guid : Guid.Empty); |
There was a problem hiding this comment.
Can we split this into multiple lines?
There was a problem hiding this comment.
We can, but it doesn't seem to be too long or too complex to me.
| } | ||
|
|
||
| private ProjectId GetActiveContextProjectIdAndWatchHierarchies(uint cookie, IEnumerable<ProjectId> projectIds) | ||
| private ProjectId GetActiveContextProjectIdAndWatchHierarchies(uint cookie, ImmutableArray<DocumentId> documentIds) |
There was a problem hiding this comment.
Why the change? It makes the other code a bit stranger to follow...
There was a problem hiding this comment.
(I actually originally wrote it this way, and then switched it over to ProjectId because the logic inside was a bit less clear...)
There was a problem hiding this comment.
I think i needed it this way at some point, now it's not needed anymore, although it saves some allocations. I can revert it if you prefer.
src/VisualStudio/TestUtilities2/ProjectSystemShim/Framework/MockVsSolution.vb
Show resolved
Hide resolved
| ' Return a loose mock that just is a big no-op | ||
| Dim solutionMock As New Mock(Of IVsSolution)(MockBehavior.Loose) | ||
| Return solutionMock.Object | ||
| Case GetType(IVsSolution), GetType(SVsSolution) |
There was a problem hiding this comment.
Something is directly asking as IVsSolution? That seems fishy.
There was a problem hiding this comment.
The request should have been for SVsSolution, not IVsSolution.
| { | ||
| AssemblyName = project.AssemblyName, | ||
| FilePath = project.ProjectFilePath, | ||
| Hierarchy = project.Hierarchy, |
There was a problem hiding this comment.
@tmat @jasonmalinowski What's the new way for TS to thread this through? Some of our refactorings are broken since the IVsHierarchy isn't being returned by IVsSolution.
As an aside, can you please ping me/us when changing a TS shim?
There was a problem hiding this comment.
Are you currently calling VisualStudioProject.GetHierarchy to get such IVsHierarchy objects?
There was a problem hiding this comment.
Which VisualStudioProject? I'm assuming you don't mean the one in Microsoft.VisualStudio.LanguageServices.TypeScript.ScriptContexts. Either way, we're trying to specify hierarchies - not retrieve them.
…ID (dotnet#35746)"" This reverts commit ba62b25.
* dotnet/master: (85 commits) Don't complete statement when typing semicolon inside comments in an argument list (dotnet#36521) Set focus to editor before finding text Add a bunch of nullability support to some code generation helpers Add 'annotations' and 'warnings' support to nullable directive (dotnet#36464) Fixed IDE services touching `notnull` constraint (dotnet#36508) Update compiler toolset to arcade version (dotnet#36549) Fix for 923157 Do not include value types in NullableAttribute byte[] (dotnet#36519) Fix a deadlock in InvokeOnUIThread Apply a hang mitigating timeout to UI thread operations Move to a different lowering from for nullable value types to work around a bug in TransformCompoundAssignmentLHS. Addressed PR feedback. Fix stack overflow in requesting syntax directives (dotnet#36347) crash on ClassifyUpdate for EventFields (dotnet#35962) fixing bad merge with refactoring that was checked in later added basic completion statement telemetry Remove duplication in AbstractSymbolCompletionProvider.CreateItems Disable move type when the options service isn't present (dotnet#36334) Fix crash where type inference doing method inference needs to drop nullability Revert "Use IVsSolution to look up IVsHierarchy by project GUID (dotnet#35746)" Fix parsing bug in invalid using statements (dotnet#36428) ...
No description provided.