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

Enforce C# 8 nullability for user code. #349

Merged
merged 3 commits into from
Mar 26, 2019
Merged

Enforce C# 8 nullability for user code. #349

merged 3 commits into from
Mar 26, 2019

Conversation

NTaylorMullen
Copy link
Contributor

  • Expanded the ProjectWorkspaceStateGenerator to extract the C# language version when building the ProjectWorkspaceState. This approach enables all platforms to get nullability support without any changes (as long as they support ProjectWorkspaceState, which they do). Also, Roslyn suggested that we avoid dealing with LangVersion directly because there are several factors that impact its "effective" value on a project when run in tooling.
  • Updated the LinePragma code generation to include #nullable restore and #nullable disable lines to allow for project restored nullability state for user code.
  • Added a new RazorProjectEngineBuilderExtensions class that adds Roslyn specific project engine modifications. In this case it allows us to set the C# language version for a project engine and configure underlying features accordingly.
  • Added a SuppressNullabilityEnforcement flag that only turns on if C# < 8 is specified.
  • Updated LiveShare, VS4Mac and RazorGenerate to understand CSharpLanguageVersion.
  • Added a single test output to show the change.

dotnet/aspnetcore#5092

pranavkm
pranavkm reviewed Mar 19, 2019
View reviewed changes
Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

Changes look ok. Is the presence of the pragma sufficient to enforce nullability when the code is marked as generated?

src/Razor/src/Microsoft.AspNetCore.Razor.Language/RazorCodeGenerationOptions.cs Outdated Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Tools/GenerateCommand.cs Show resolved Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor/RazorProjectEngineBuilderExtensions.cs Show resolved Hide resolved
@NTaylorMullen
Copy link
Contributor Author

Changes look ok. Is the presence of the pragma sufficient to enforce nullability when the code is marked as generated?

These are changes that @jcouv is working on adding 😄 . However, as of right now, no.

@jcouv jcouv mentioned this pull request Mar 19, 2019
Closed
rynowak
rynowak reviewed Mar 20, 2019
View reviewed changes
src/Razor/test/Microsoft.VisualStudio.LiveShare.Razor.Test/SerializationTest.cs
@@ -22,7 +22,7 @@ public void ProjectSnapshotHandleProxy_RoundTripsProperly()
TagHelperDescriptorBuilder.Create("TestTagHelper", "TestAssembly").Build(),
TagHelperDescriptorBuilder.Create("TestTagHelper2", "TestAssembly2").Build(),
};
var projectWorkspaceState = new ProjectWorkspaceState(tagHelpers);
var projectWorkspaceState = new ProjectWorkspaceState(tagHelpers, default);
Copy link
Member

Choose a reason for hiding this comment

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

Can we add an integration tests for the E2E of using this at build-time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, this is just a design PR 😄

pranavkm
pranavkm reviewed Mar 23, 2019
View reviewed changes
src/Razor/src/Microsoft.AspNetCore.Razor.Tools/GenerateCommand.cs Show resolved Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor/RazorProjectEngineBuilderExtensions.cs Outdated Show resolved Hide resolved
...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets Show resolved Hide resolved
...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets Outdated Show resolved Hide resolved
...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets
@@ -120,6 +133,8 @@ Copyright (c) .NET Foundation. All rights reserved.
DefineConstants="$(DefineConstants)"
DelaySign="$(DelaySign)"
DisabledWarnings="$(NoWarn)"
DisableSdkPath="$(DisableSdkPath)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea what this does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No clue.

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets Outdated Show resolved Hide resolved
NTaylorMullen
NTaylorMullen commented Mar 24, 2019
View reviewed changes
eng/Versions.props
<MicrosoftCodeAnalysisCommonPackageVersion>2.11.0-beta1-final</MicrosoftCodeAnalysisCommonPackageVersion>
<MicrosoftCodeAnalysisCSharpPackageVersion>2.11.0-beta1-final</MicrosoftCodeAnalysisCSharpPackageVersion>
<MicrosoftCodeAnalysisWorkspacesCommonPackageVersion>2.11.0-beta1-final</MicrosoftCodeAnalysisWorkspacesCommonPackageVersion>
<MicrosoftCodeAnalysisCommonPackageVersion>3.0.0-beta4-final</MicrosoftCodeAnalysisCommonPackageVersion>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the latest publicly available Roslyn bits on NuGet. Since we now generate code that's only understood by a newer Roslyn (#nullable restore/preview) I had to update our dependency to correspond with it. Thoughts? @rynowak

Copy link
Member

Choose a reason for hiding this comment

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

Seems right

NTaylorMullen
NTaylorMullen commented Mar 24, 2019
View reviewed changes
eng/Versions.props
<VSIX_MicrosoftCodeAnalysisWorkspacesCommonPackageVersion>3.0.0-beta2-18612-05</VSIX_MicrosoftCodeAnalysisWorkspacesCommonPackageVersion>
<VSIX_MicrosoftVisualStudioLanguageServicesPackageVersion>3.0.0-beta2-18612-05</VSIX_MicrosoftVisualStudioLanguageServicesPackageVersion>
<VSIX_MicrosoftVisualStudioLanguageServicesRazorRemoteClientPackageVersion>3.0.0-beta2-18612-05</VSIX_MicrosoftVisualStudioLanguageServicesRazorRemoteClientPackageVersion>
<VSIX_MicrosoftCodeAnalysisPackageVersion>3.1.0-beta1-19156-03</VSIX_MicrosoftCodeAnalysisPackageVersion>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the current version that is available for the next VS release. Had to update for similar reasons to above (need a newer Roslyn to understand #nullable restore / preview)

@NTaylorMullen NTaylorMullen force-pushed the nimullen/5092 branch 2 times, most recently from 578137d to 1add775 Compare March 24, 2019 17:36
@NTaylorMullen NTaylorMullen marked this pull request as ready for review March 24, 2019 17:45
@NTaylorMullen
Copy link
Contributor Author

🆙 📅 out of design.

@dougbu dougbu changed the title [Design] Enforce C# 8 nullability for user code. Enforce C# 8 nullability for user code. Mar 24, 2019
dougbu
dougbu approved these changes Mar 24, 2019
View reviewed changes
Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

eng/Versions.props change looks fine

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets Show resolved Hide resolved
@NTaylorMullen
Enforce nullability for user code.
7c09677 
- Expanded the `ProjectWorkspaceStateGenerator` to extract the C# language version when building the `ProjectWorkspaceState`. This approach enables all platforms to get nullability support without any changes (as long as they support `ProjectWorkspaceState`, which they do). Also, Roslyn suggested that we avoid dealing with LangVersion directly because there are several factors that impact its "effective" value on a project when run in tooling.
- Updated the `LinePragma` code generation to include `#nullable restore` and `#nullable disable` lines to allow for project restored nullability state for user code.
- Added a new `RazorProjectEngineBuilderExtensions` class that adds Roslyn specific project engine modifications. In this case it allows us to set the C# language version for a project engine and configure underlying features accordingly.
- Added a `SuppressNullabilityEnforcement` flag that only turns on if C# < 8 is specified.
- Updated LiveShare, VS4Mac and RazorGenerate to understand CSharpLanguageVersion.
- Added a single test output to show the change.

#5092
@NTaylorMullen
Design update.
6f22b1c
@NTaylorMullen
Update more test files.
9f929e6
@NTaylorMullen
Copy link
Contributor Author

Ugh, scratch that. AspNetCore looks to be stuck on a preview4 SDK that doesn't have support for "preview" Roslyn LangVersion. Justin is working on getting a new SDK but that could take some time.

So our options are:

  1. Wait for AspNetCore to update its SDK (we'd definitely miss the Wed VS insertion deadline)
  2. Force LangVersion to 8.0 instead of preview.

@rynowak what would you prefer?

@NTaylorMullen
Copy link
Contributor Author

NTaylorMullen commented Mar 25, 2019 •
edited
Loading

This good to go in?

@NTaylorMullen NTaylorMullen merged commit 76c1352 into master Mar 26, 2019
@NTaylorMullen NTaylorMullen deleted the nimullen/5092 branch March 26, 2019 03:26
@benaadams benaadams mentioned this pull request Apr 1, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pranavkm pranavkm pranavkm left review comments

@dougbu dougbu dougbu approved these changes

@rynowak rynowak rynowak approved these changes

@ajaybhargavb ajaybhargavb Awaiting requested review from ajaybhargavb

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NTaylorMullen @pranavkm @rynowak @dougbu