Enforce C# 8 nullability for user code. by NTaylorMullen · Pull Request #349 · dotnet/razor

NTaylorMullen · 2019-03-19T19:04:30Z

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

...st/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/DesignTime_DesignTime.codegen.cs

pranavkm

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

src/Razor/src/Microsoft.AspNetCore.Razor.Tools/GenerateCommand.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor/RazorProjectEngineBuilderExtensions.cs

NTaylorMullen · 2019-03-19T19:17:23Z

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.

src/Razor/src/Microsoft.AspNetCore.Razor.Tools/GenerateCommand.cs

...st/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/DesignTime_DesignTime.codegen.cs

.../Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.CodeGeneration.targets

rynowak · 2019-03-20T00:06:38Z

src/Razor/test/Microsoft.VisualStudio.LiveShare.Razor.Test/SerializationTest.cs

                TagHelperDescriptorBuilder.Create("TestTagHelper2", "TestAssembly2").Build(),
            };
-            var projectWorkspaceState = new ProjectWorkspaceState(tagHelpers);
+            var projectWorkspaceState = new ProjectWorkspaceState(tagHelpers, default);


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

Absolutely, this is just a design PR 😄

src/Razor/src/Microsoft.AspNetCore.Razor.Tools/GenerateCommand.cs

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets

src/Razor/src/Microsoft.AspNetCore.Razor.Tools/GenerateCommand.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor/RazorProjectEngineBuilderExtensions.cs

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets

pranavkm · 2019-03-23T21:02:22Z

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets

         DefineConstants="$(DefineConstants)"
         DelaySign="$(DelaySign)"
         DisabledWarnings="$(NoWarn)"
+         DisableSdkPath="$(DisableSdkPath)"


Any idea what this does?

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets

NTaylorMullen · 2019-03-24T17:16:24Z

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>


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

Seems right

NTaylorMullen · 2019-03-24T17:17:22Z

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>


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)

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets

...r/src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Component.targets

NTaylorMullen · 2019-03-24T17:46:04Z

🆙 📅 out of design.

dougbu

eng/Versions.props change looks fine

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets

- 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 · 2019-03-25T22:16:32Z

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:

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

@rynowak what would you prefer?

NTaylorMullen · 2019-03-25T23:24:52Z

This good to go in?

NTaylorMullen requested review from ajaybhargavb, pranavkm and rynowak March 19, 2019 19:04

NTaylorMullen commented Mar 19, 2019

View reviewed changes

...st/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/DesignTime_DesignTime.codegen.cs Show resolved Hide resolved

pranavkm reviewed Mar 19, 2019

View reviewed changes

jcouv mentioned this pull request Mar 19, 2019

#nullable restore in generated file should restore nullable context from project dotnet/roslyn#34094

Closed

rynowak reviewed Mar 19, 2019

View reviewed changes

src/Razor/src/Microsoft.AspNetCore.Razor.Tools/GenerateCommand.cs Show resolved Hide resolved

rynowak reviewed Mar 20, 2019

View reviewed changes

...st/TestFiles/IntegrationTests/CodeGenerationIntegrationTest/DesignTime_DesignTime.codegen.cs Show resolved Hide resolved

rynowak reviewed Mar 20, 2019

View reviewed changes

.../Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.CodeGeneration.targets Show resolved Hide resolved

rynowak reviewed Mar 20, 2019

View reviewed changes

NTaylorMullen commented Mar 20, 2019

View reviewed changes

src/Razor/src/Microsoft.AspNetCore.Razor.Tools/GenerateCommand.cs Show resolved Hide resolved

NTaylorMullen force-pushed the nimullen/5092 branch from 913ae59 to 519d67b Compare March 22, 2019 17:42

NTaylorMullen commented Mar 22, 2019

View reviewed changes

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets Show resolved Hide resolved

NTaylorMullen commented Mar 22, 2019

View reviewed changes

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets Outdated Show resolved Hide resolved

rynowak reviewed Mar 22, 2019

View reviewed changes

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets Outdated Show resolved Hide resolved

rynowak reviewed Mar 22, 2019

View reviewed changes

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets Outdated Show resolved Hide resolved

rynowak reviewed Mar 22, 2019

View reviewed changes

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets Outdated Show resolved Hide resolved

rynowak reviewed Mar 22, 2019

View reviewed changes

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets Show resolved Hide resolved

NTaylorMullen commented Mar 23, 2019

View reviewed changes

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets Outdated Show resolved Hide resolved

pranavkm reviewed Mar 23, 2019

View reviewed changes

NTaylorMullen force-pushed the nimullen/5092 branch from 519d67b to 1dbde78 Compare March 24, 2019 17:11

NTaylorMullen commented Mar 24, 2019

View reviewed changes

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets Show resolved Hide resolved

NTaylorMullen commented Mar 24, 2019

View reviewed changes

...r/src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Component.targets Show resolved Hide resolved

NTaylorMullen commented Mar 24, 2019

View reviewed changes

...r/src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Component.targets Show resolved Hide resolved

NTaylorMullen force-pushed the nimullen/5092 branch 2 times, most recently from 578137d to 1add775 Compare March 24, 2019 17:36

NTaylorMullen marked this pull request as ready for review March 24, 2019 17:45

NTaylorMullen requested a review from dougbu as a code owner March 24, 2019 17:45

dougbu changed the title ~~[Design] Enforce C# 8 nullability for user code.~~ Enforce C# 8 nullability for user code. Mar 24, 2019

dougbu approved these changes Mar 24, 2019

View reviewed changes

...src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Compilation.targets Show resolved Hide resolved

N. Taylor Mullen added 3 commits March 25, 2019 14:34

Design update.

6f22b1c

Update more test files.

9f929e6

NTaylorMullen force-pushed the nimullen/5092 branch from 8eb35c9 to b36198e Compare March 25, 2019 21:34

NTaylorMullen force-pushed the nimullen/5092 branch from 8b786f0 to 9f929e6 Compare March 25, 2019 23:18

rynowak approved these changes Mar 26, 2019

View reviewed changes

NTaylorMullen merged commit 76c1352 into master Mar 26, 2019

NTaylorMullen deleted the nimullen/5092 branch March 26, 2019 03:26

benaadams mentioned this pull request Apr 1, 2019

Chain coherent dependencies up through Extensions to EF Core packages dotnet/aspnetcore#8877

Merged

Conversation

NTaylorMullen commented Mar 19, 2019

Uh oh!

Uh oh!

pranavkm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NTaylorMullen commented Mar 19, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rynowak Mar 20, 2019

Choose a reason for hiding this comment

Uh oh!

NTaylorMullen Mar 20, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pranavkm Mar 23, 2019

Choose a reason for hiding this comment

Uh oh!

NTaylorMullen Mar 24, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NTaylorMullen Mar 24, 2019

Choose a reason for hiding this comment

Uh oh!

rynowak Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

NTaylorMullen Mar 24, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NTaylorMullen commented Mar 24, 2019

Uh oh!

dougbu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NTaylorMullen commented Mar 25, 2019

Uh oh!

NTaylorMullen commented Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

NTaylorMullen commented Mar 25, 2019 •

edited

Loading