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

Use given model nullability in razor pages #37510

Closed
Jupakabra opened this issue Oct 13, 2021 · 26 comments
Closed

Use given model nullability in razor pages #37510

Jupakabra opened this issue Oct 13, 2021 · 26 comments
Assignees
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates cost: M Will take from 3 - 5 days to complete enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-razor-pages Priority:1 Work that is critical for the release, but we could probably ship without
Milestone

Comments

@Jupakabra
Copy link

Jupakabra commented Oct 13, 2021

We are migrating our 5.0 project to 6.0-rc2 and noticed that RazorPage.Model was changed to always be nullable, even though model type in razor file is specified as not null. Because of this, we are getting thousands of compile time errors on any model access outside of asp-for expressions like asp-route-id="@Model.Id".

Describe the solution you'd like

Would it be possible to change it to TModel from TModel??
So we can tell the razor page if model is possibly null or not in the view?

@model MyModel?

Additional context

My guess is that razor doesn't parse nullable models in views, so it wasn't made that way because of that?

@Pilchie Pilchie added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Oct 13, 2021
@mkArtakMSFT mkArtakMSFT added this to the Discussions milestone Oct 14, 2021
@pranavkm
Copy link
Contributor

In this particular case, it's valid for the model to be null i.e. you could do

// Index.cshtml
@model Person

// PersonController.cs
public IActionResult Get() => View(); // effectively View(model: null);

and the framework allows this. Introducing a null check in this code path would be a breaking change.

A possible workaround might be to introduce a base type for your view - https://docs.microsoft.com/en-us/aspnet/core/mvc/views/razor?view=aspnetcore-5.0#inherits that has the model property annotated for nullability differently and use that in your app. I'm leaving the issue open in case we'd like to try and address this in a later release.

@pranavkm
Copy link
Contributor

FYI @DamianEdwards

@pranavkm pranavkm modified the milestones: Discussions, Backlog Oct 14, 2021
@ghost
Copy link

ghost commented Oct 14, 2021

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@pranavkm pranavkm removed their assignment Oct 14, 2021
@Jupakabra
Copy link
Author

Ye, we are aware that it's just an annotation disconnected from the back end. Was just thinking that matching razor model with your backend would result in better experience compared to just disabling nullability all together to avoid compile errors.

Good idea about the base view will try to do that 👍

@pranavkm pranavkm added the Priority:1 Work that is critical for the release, but we could probably ship without label Oct 28, 2021
@pranavkm pranavkm modified the milestones: Backlog, .NET 7 Planning Oct 28, 2021
@mkArtakMSFT mkArtakMSFT added enhancement This issue represents an ask for new feature or an enhancement to an existing one cost: M Will take from 3 - 5 days to complete triaged labels Oct 28, 2021
@Jupakabra
Copy link
Author

Tried using custom razor page model, but as expected razor doesn't handle it well today. For example using

@model TestModel?

Doesn't compile at all, because nullability context is disabled? Even though the project has it enabled.

cshtml.g.cs(10,5,10,6): error CS8669: The annotation for nullable reference types should only be used in code
within a '#nullable' annotations context. Auto-generated code requires an explicit '#nullable' directive in source.

@Bartmax
Copy link
Contributor

Bartmax commented Dec 3, 2021

I'm having this issue too. I believe it was wrong to annotate TModel as ?. A real blocker to upgrade to net6.
For me it's impossible to upgrade to NET6. I have 1983 views in one project only.

We need a way to fix (opt out of nullable razor models) with one liner at startup.

@seangwright
Copy link
Contributor

seangwright commented Dec 22, 2021

This is a show-stopping breaking change if an app has <WarningsAsErrors>nullable</WarningsAsErrors>.

If an application has hundreds (or thousands) of Views, it's going to take awhile adding this 'unofficial' solution to get the app to compile again.

A possible workaround might be to introduce a base type for your view

Where was the planning for this feature? What was the official planned recommendation to resolve this issue for the many apps built before .NET 6.0?

Hundreds or thousands of errors after upgrading to .NET 6.0 and the best we have at the moment is "a possible workaround"?

🤦‍♂️🤦‍♂️🤦‍♂️🤦‍♂️

@pranavkm pranavkm self-assigned this Jan 5, 2022
@pranavkm
Copy link
Contributor

pranavkm commented Jan 5, 2022

Thanks for the feedback. Addressing this is two parts -

My hope is to try and get these tackled as part of an upcoming VS release / .NET servicing release.

pranavkm added a commit to dotnet/razor-compiler that referenced this issue Jan 12, 2022
Currently the Razor compiler suppresses nullable annotations within most generated code. https://github.com/dotnet/aspnetcore/issues/39360 tracks addressing this, but that would need a fair bit of work to get right. In the meantime, dotnet/aspnetcore#37510 is affecting a lot of users attempting to turn on nullability in their apps. 

The value of the `@model` directive appears as part of the class declaration for a Razor View: 

```C#
@model MyModel?

// Results in 
#nullable disable
internal sealed class _Views_Home_Index : RazorPage<MyModel?> // This will produce a warning since it's not within a nullable context
```

The `#nullable disable` precludes the ability of the user to be able to specify a nullable `model`. This change updates the compiler and generates the class declaration within a nullable context, as long as the C# version is recent enough to support nullability. The generated code now looks like:

```C#
@model MyModel?

// Results in 
...
#nullable restore // Use the nullability based on the project's global settings
internal sealed class _Views_Home_Index : RazorPage<MyModel?>
#nullable disable
```

In addition to the class declaration, we also need to generate the properties for `@inject` within a nullable context. This is because all views have an implicit `@inject IHtmlHelper<TModel>` where `TModel` is the model type.
pranavkm added a commit to dotnet/razor-compiler that referenced this issue Jan 12, 2022
)

Currently the Razor compiler suppresses nullable annotations within most generated code. https://github.com/dotnet/aspnetcore/issues/39360 tracks addressing this, but that would need a fair bit of work to get right. In the meantime, dotnet/aspnetcore#37510 is affecting a lot of users attempting to turn on nullability in their apps. 

The value of the `@model` directive appears as part of the class declaration for a Razor View: 

```C#
@model MyModel?

// Results in 
#nullable disable
internal sealed class _Views_Home_Index : RazorPage<MyModel?> // This will produce a warning since it's not within a nullable context
```

The `#nullable disable` precludes the ability of the user to be able to specify a nullable `model`. This change updates the compiler and generates the class declaration within a nullable context, as long as the C# version is recent enough to support nullability. The generated code now looks like:

```C#
@model MyModel?

// Results in 
...
#nullable restore // Use the nullability based on the project's global settings
internal sealed class _Views_Home_Index : RazorPage<MyModel?>
#nullable disable
```

In addition to the class declaration, we also need to generate the properties for `@inject` within a nullable context. This is because all views have an implicit `@inject IHtmlHelper<TModel>` where `TModel` is the model type.
@pranavkm
Copy link
Contributor

This is fixed for the 6.0.2 release of the runtime and a compiler change that allows models to be declared as nullable would be available in 17.1 / 6.0.200 version of the .NET SDK. Thanks for all of your feedback!

@kingmotley
Copy link

@pranavkm Since runtime 6.0.2 seems to be included in the 17.0.6/6.0.102 SDK would those be appropriate, or is there additional work coming in 17.1.x/6.0.2xx?

@dotnetshadow
Copy link

dotnetshadow commented Feb 8, 2022

I'm a little confused, I just updated my app to 6.0.2 but I'm still getting the analyser message:

Severity Code Description Project File Line Suppression State
Warning (active) CS8602 Dereference of a possibly null reference. Foo.Web d:\Programming\Code.....\Dashboard\Index.cshtml 70

<AnalysisMode>AllEnabledByDefault</AnalysisMode>

This is how I'm using it in index.cshtml

@model Foo.Dashboard.DashboardViewModel

<span class="counter-number-related text-capitalize">@("people".ToQuantity(Model.UserOnlineCount, ShowQuantityAs.None))</span>

Other people having the same issue:
https://stackoverflow.com/questions/69962866/pragma-8602-warning-in-razor-markup

@pranavkm
Copy link
Contributor

pranavkm commented Feb 8, 2022

So I just tried this with the 6.0.102 SDK installed and the runtime changes work - doing @Model.UserOnlineCount does not produce a nullable warning. I'm still unable to do @model TestModel? since that relies on a change in the 6.0.200 SDK which hasn't shipped.

Could you confirm that's the SDK your app is using to build? SDKs typically roll forward, so if you happen to have a 6.0.200 preview SDK installed side-by-side, it'll end up using that instead and not use these changes. You can confirm the SDK that is being used by running dotnet --info. You can also use a global.json to specify an exact SDK to use to build your project. You could consider using that until a RTM 6.0.200 rolls out:

Global.json contents:

{
  "sdk": {
    "version": "6.0.102"
  }
}

@pranavkm pranavkm reopened this Feb 8, 2022
@dotnetshadow
Copy link

dotnetshadow commented Feb 8, 2022

@pranavkm Thanks for getting back to me, have you set the analysis mode to <AnalysisMode>AllEnabledByDefault</AnalysisMode>

This is my runtime info
image

This is what I see in visual studio 2022
image

@pranavkm
Copy link
Contributor

pranavkm commented Feb 8, 2022

@dotnetshadow setting the AnalysisMode does not change the result for me. Do you see the same error if you build the app from the CLI? If you do, could I bother you to share msbuild binary logs by running dotnet msbuild /bl and including the MSBuild.binlog file. It might provide some hints about what's happening here.

Note that binlogs will capture ambient environment variables.

@dotnetshadow
Copy link

dotnetshadow commented Feb 8, 2022

Do you see the same error if you build the app from the CLI?

Funny you should ask, I just tried that and I am getting the warning
To always get the error use: dotnet build --no-incremental

b\Features\Admin\Dashboard\Index.cshtml(70,97): warning CS8602: Dereference of a possibly null reference. [D:\Programming\Code\Company\StockWatch\src\Foo.Web\Foo.Web.csproj]

msbuild binary log
https://www.sendspace.com/file/r2y1cu

@pranavkm
Copy link
Contributor

pranavkm commented Feb 9, 2022

@dotnetshadow looks like the file was removed.

@dotnetshadow
Copy link

@dotnetshadow looks like the file was removed.

Sorry about that I uploaded to a new host
https://www.sendspace.com/file/r2y1cu

@Diego-T
Copy link

Diego-T commented Feb 9, 2022

I was getting the same warning for razor files in Visual Studio and CLI build. dotnet --info also showed version 6.0.102 SDK.
Initially I installed it from Windows Update but after downloading and installing the SDK manually, the warning was gone.

Before reinstalling, when I used the "Go to definition" command over Model, I saw it still had the nullable annotation:

public TModel? Model { get; }

, but not after. I'm hoping that helps with the diagnostic.

@pranavkm
Copy link
Contributor

pranavkm commented Feb 9, 2022

Thanks @Diego-T. Looking at the build logs, it looks like it's compiling against the right assembly. Could you try installing the SDK from https://dotnet.microsoft.com/en-us/download/dotnet/6.0? It would help narrow down if this is a setup / installer issue.

@Diego-T
Copy link

Diego-T commented Feb 9, 2022

@pranavkm That's what I did, I installed the SDK from that link and the warning was cleared. I'm on Windows 11 (22000.493) and used the x64 installer.
That compilation log is not mine though.

@dotnetshadow
Copy link

dotnetshadow commented Feb 9, 2022

Thanks @Diego-T. Looking at the build logs, it looks like it's compiling against the right assembly. Could you try installing the SDK from https://dotnet.microsoft.com/en-us/download/dotnet/6.0? It would help narrow down if this is a setup / installer issue.

I did install from dot.net
image

.NET SDK (reflecting any global.json):
 Version:   6.0.102
 Commit:    02d5242ed7

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19044
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\6.0.102\

Host (useful for support):
  Version: 6.0.2
  Commit:  839cdfb0ec

.NET SDKs installed:
  3.1.201 [C:\Program Files\dotnet\sdk]
  3.1.202 [C:\Program Files\dotnet\sdk]
  3.1.301 [C:\Program Files\dotnet\sdk]
  3.1.416 [C:\Program Files\dotnet\sdk]
  5.0.101 [C:\Program Files\dotnet\sdk]
  5.0.102 [C:\Program Files\dotnet\sdk]
  5.0.103 [C:\Program Files\dotnet\sdk]
  5.0.203 [C:\Program Files\dotnet\sdk]
  5.0.211 [C:\Program Files\dotnet\sdk]
  5.0.400 [C:\Program Files\dotnet\sdk]
  5.0.401 [C:\Program Files\dotnet\sdk]
  5.0.403 [C:\Program Files\dotnet\sdk]
  5.0.405 [C:\Program Files\dotnet\sdk]
  6.0.100 [C:\Program Files\dotnet\sdk]
  6.0.101 [C:\Program Files\dotnet\sdk]
  6.0.102 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 3.1.20 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.21 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.22 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.11 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.12 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.14 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.1 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.21 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.22 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.11 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.12 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.14 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.1 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.1.21 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 3.1.22 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.12 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.14 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.1 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.2 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

To install additional .NET runtimes or SDKs:
  https://aka.ms/dotnet-download

I will try to reinstall again

@dotnetshadow
Copy link

dotnetshadow commented Feb 9, 2022

I was getting the same warning for razor files in Visual Studio and CLI build. dotnet --info also showed version 6.0.102 SDK. Initially I installed it from Windows Update but after downloading and installing the SDK manually, the warning was gone.

Before reinstalling, when I used the "Go to definition" command over Model, I saw it still had the nullable annotation:

public TModel? Model { get; }

, but not after. I'm hoping that helps with the diagnostic.

Hi @Diego-T Did you try compiling with the following? I initially didn't have the error but that's because dotnet build didn't try to rebuild properly
```dotnet build --no-incremental``

My 'Go to Definition' looks like this

image

@Diego-T
Copy link

Diego-T commented Feb 9, 2022

@dotnetshadow I just tried with that flag --no-incremental and I don't get the warning. I can no longer reproduce the issue.

@dotnetshadow
Copy link

dotnetshadow commented Feb 9, 2022

@dotnetshadow I just tried with that flag --no-incremental and I don't get the warning. I can no longer reproduce the issue.

Thanks @Diego-T @pranavkm. I just reinstalled dotnet-sdk-6.0.102-win-x64.exe seems to work now. Could of been a point in time thing. I rebooted and looks to be working now thanks for all your help guys. This issue can be closed

@pranavkm pranavkm closed this as completed Feb 9, 2022
@dotnetshadow
Copy link

dotnetshadow commented Feb 9, 2022

@pranavkm Just a note, I did notice the following peculiar issue.

Say you have as your model

// Index.cshtml
@model Person

Then later in the page you might have the following

// Index.cshtml
<p>@Model?.Age</p> // notice the ? here (even if it's by accident)
<p>@Model.Age</p>  // causes  Warning (active)	CS8602	Dereference of a possibly null reference.	

@pranavkm
Copy link
Contributor

pranavkm commented Feb 9, 2022

@dotnetshadow that's a behavior for the C# compiler and works the same in regular C# code. You could follow up at https://github.com/dotnet/roslyn if you'd like further clarification.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates cost: M Will take from 3 - 5 days to complete enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-razor-pages Priority:1 Work that is critical for the release, but we could probably ship without
Projects
None yet
Development

No branches or pull requests

9 participants