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.

Sign up for GitHub

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

Jump to bottom

Fix IDE remote analyzer loader on .Net Core #57407

Merged
merged 1 commit into from
Oct 29, 2021
Merged

Fix IDE remote analyzer loader on .Net Core #57407

merged 1 commit into from
Oct 29, 2021

Conversation

genlu
Copy link
Member

@genlu genlu commented Oct 27, 2021 •
edited
Loading

We ran into an issue that "unnecessary usings" are not fading when IDE analyzers are hosted on .Net Core host. Further investigation shows that the issue might lie in here

The way analyzer loader work on .Net core, MS.CA.Features is loaded into a separate DirectoryLoadContext when used as an IDE analyzer (# 2 in the screenshot), but it's also loaded into Roslyn's ALC when our service is created in ServiceHub host (# 0) therefore analyzerOptions is not WorkspaceAnalyzerOptions workspaceAnalyzerOptions) is evaluated to true, even though analyzerOptions is indeed of type WorkspaceAnalyzerOptions, but because the type is from another ALC.

image

Fix #57395

@RikkiGibson RikkiGibson self-assigned this Oct 27, 2021
@RikkiGibson
Copy link
Contributor

@jaredpar I think we had discussed eliminating the CompilerAssemblySimpleNames. I don't recall exactly the thinking here. But it feels like this scenario might be solved by always delegating to the compiler ALC first. It's not clear to me if doing that does not introduce new problems.

@genlu genlu marked this pull request as ready for review October 27, 2021 21:21
@genlu genlu requested review from a team as code owners October 27, 2021 21:21
jasonmalinowski
jasonmalinowski previously approved these changes Oct 27, 2021
View reviewed changes
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Fix seems reasonable, as long as we still feel the listing of assemblies is still the right mechanism. But this isn't doing anything worse than it already is.

src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzerAssemblyLoader.Core.cs Outdated Show resolved Hide resolved
jasonmalinowski
jasonmalinowski reviewed Oct 27, 2021
View reviewed changes
src/Workspaces/Remote/ServiceHub/Host/RemoteAnalyzerAssemblyLoaderService.cs Outdated
Comment on lines 59 to 62
// The following are special assemblies since they contain IDE analyzers and/or their dependencies,
// but in the meantime, they also contain the host of compiler in remote process. Therefore on coreclr,
// we must ensure they are only loaded once and in the same ALC compiler asemblies are loaded into.
// Otherwise these analyzers will fail to interoperate with the host due to mismatch in assembly identity.
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a bug tracking the removal of the WorkspaceAnalyzerOptions that's the cause of all of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about this. If this is the intention, could you please file a bug for it? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

#8605 tracks removal of WorkspaceAnalyzerOptions and potentially seal AnalyzerOptions

@jasonmalinowski jasonmalinowski dismissed their stale review October 27, 2021 22:08

Unsure if this is right after all.

jasonmalinowski
jasonmalinowski approved these changes Oct 27, 2021
View reviewed changes
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Oh right, it is right. I hate this part of our codebase.

@RikkiGibson
Copy link
Contributor

I see some shiproom related labels. Please be sure this is targeted to the appropriate branch.

@genlu genlu changed the base branch from main to release/dev17.1 October 29, 2021 00:10
@genlu
Copy link
Member Author

genlu commented Oct 29, 2021

@RikkiGibson I have rebased against 17.1, thanks

genlu
genlu commented Oct 29, 2021
View reviewed changes
src/Workspaces/Remote/ServiceHubTest/Microsoft.CodeAnalysis.Remote.ServiceHub.UnitTests.csproj
@@ -0,0 +1,16 @@
<?xml version="1.0" encoding="utf-8"?>
<!-- Licensed to the .NET Foundation under one or more agreements. The .NET Foundation licenses this file to you under the MIT license. See the LICENSE file in the project root for more information. -->
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Member Author

Choose a reason for hiding this comment

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

I have to add a new test project because we currently don't have any IDE tests runs on .Net Core above workspaces layer

@chsienki chsienki self-assigned this Oct 29, 2021
chsienki
chsienki approved these changes Oct 29, 2021
View reviewed changes
Copy link
Contributor

@chsienki chsienki left a comment

Choose a reason for hiding this comment

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

LGTM

JoeRobich
JoeRobich reviewed Oct 29, 2021
View reviewed changes
Roslyn.sln
@@ -1,4 +1,5 @@
Microsoft Visual Studio Solution File, Format Version 12.00

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change


@genlu genlu enabled auto-merge October 29, 2021 04:56
@genlu genlu merged commit 282dec5 into dotnet:release/dev17.1 Oct 29, 2021
@genlu genlu deleted the FixLoader branch October 29, 2021 07:44
@genlu genlu restored the FixLoader branch October 29, 2021 20:27
@genlu genlu deleted the FixLoader branch October 29, 2021 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chsienki chsienki chsienki approved these changes

@mavasani mavasani mavasani approved these changes

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@JoeRobich JoeRobich JoeRobich approved these changes

@jasonmalinowski jasonmalinowski jasonmalinowski approved these changes

@jaredpar jaredpar Awaiting requested review from jaredpar

Assignees

@RikkiGibson RikkiGibson

@chsienki chsienki

Labels
Approved to merge Area-Analyzers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CSharpSquiggles tests are failing on .Net Core Servicehub host
7 participants
@genlu @RikkiGibson @jasonmalinowski @JoeRobich @mavasani @chsienki @vatsalyaagrawal