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

Fully update CompilationOptions when project files change. #1774

Merged

Conversation

SirIntruder
Copy link
Contributor

@SirIntruder SirIntruder commented Apr 26, 2020

When .csproj files change, o# gets fully updated CSharpCompilationOptions from MSBuild, but it doesn't apply them to the workspace. Because of this, o# completely ignores changes to AllowUnsafeCode, Nullable, DefineConstants, LangVersion etc. after the initial project load.

Fortunately, o# was updating CSharpCompilationOptions already, for the purpose of updating analyzer rulesets. So fix amounted to passing

projectFileInfo.CreateCompilationOptions()

instead of

project.CompilationOptions.WithSpecificDiagnosticOptions(projectFileInfo.GetDiagnosticOptions())

Problem with the old code was that project.CompilationOptions was essentially old compiler options. ProjectFileInfo is fresh data, and with this PR the whole CompilerOptions object will be fresh, not just the DiagnosticsOptions part.

@SirIntruder
Copy link
Contributor Author

I will make a test when I catch some more time this week, need to check how FileChanges are handled in the testHost.

@SirIntruder SirIntruder changed the title Fully update CompilationOptions when project files get changed. Fully update CompilationOptions when project files change. Apr 28, 2020
@filipw
Copy link
Member

filipw commented Apr 30, 2020

thanks a lot this very helpful.

Because of this, o# completely ignores changes to AllowUnsafeCode, Nullable, DefineConstants, LangVersion etc. after the initial project load.

this is not entirely true, because OmniSharp already automatically updates parse options (like DefineConstants, LangVersion). Only compilation options are ignored.

Copy link
Member

@filipw filipw left a comment

Choose a reason for hiding this comment

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

IMHO this is ok to take as-is, we can add the test in a separate PR

@JoeRobich JoeRobich merged commit a9ea76f into OmniSharp:master Apr 30, 2020
@JoeRobich
Copy link
Member

@SirIntruder Thanks!

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