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

Nullable annotations metadata on non-public members #32804

Closed
JamesNK opened this issue May 18, 2021 · 8 comments
Closed

Nullable annotations metadata on non-public members #32804

JamesNK opened this issue May 18, 2021 · 8 comments
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-minimal-actions Controller-like actions for endpoint routing
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented May 18, 2021

Description

In ASP.NET Core we want to use nullable annotations metadata to make decisions about how to call user-defined methods and local-methods/delegates. For more information, see #32375

Today that metadata is removed from private members and compiler-generated methods: dotnet/runtime#29723 (comment)

Also we set the flag for the compiler to not emit nullable metadata for non visible outside the assembly APIs; I don't know how interesting it would be for apps to figure out nullability for internal/private APIs of the framework, I guess that is a non-goal for this API but something to add on the docs?

I'm guessing this was done to reduce metadata in assemblies.

Unfortunately, it means that today we can't use that information when calling compiler-generated methods and private methods.

// Nullable IService means if IService is not resolved from DI
// then the method will be called will a null value instead of throwing an error
app.MapPost("/todos", (IService? service) => {
    // stuff
});
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@JamesNK
Copy link
Member Author

JamesNK commented May 18, 2021

@halter73 @davidfowl

@jkotas
Copy link
Member

jkotas commented May 18, 2021

This option is called nullablePublicOnly and it is set here: https://github.com/dotnet/runtime/blob/main/Directory.Build.props#L268

This option is right for dotnet/runtime since we generally try to avoid reflection and we do not support 3rd party code using reflection against internal implementation details.

If you would like to not set this for ASP.NET and take the size hit, that's your call to make on whether it is the right tradeoff.

@jkotas jkotas transferred this issue from dotnet/runtime May 18, 2021
@JamesNK
Copy link
Member Author

JamesNK commented May 18, 2021

The private and compiler-generated methods in this situation aren't written by us. They're from user's applications and we're reflecting on them.

Would that mean the nullablePublicOnly setting must be not set in the ASP.NET Core SDK?

@jkotas
Copy link
Member

jkotas commented May 18, 2021

I do not see anyting setting nullablePublicOnly in any SDK. Do you have a repro for the problem that you are trying to solve? Maybe you are getting confused by nullablePublicOnly being set for ASP.NET Core repo here

<Features>strict;nullablePublicOnly</Features>
?

@JamesNK
Copy link
Member Author

JamesNK commented May 18, 2021

Ahh, I get what is going on now. This is something that we've opted into for assemblies produced by runtime and aspnetcore.

In user app assemblies this flag isn't on so the nullable metadata will always be available.

My confusion was because I was testing this in an aspnetcore test assembly, which has the nullablePublicOnly flag applied to it.

I think the only change here will be to turn nullablePublicOnly on in just the assemblies that are deployed in the SDK, and leave it off in test assemblies. That will allow test assemblies to simulate user's apps more closely.

@javiercn javiercn added feature-minimal-actions Controller-like actions for endpoint routing area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels May 18, 2021
@mkArtakMSFT mkArtakMSFT removed the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 18, 2021
@BrennanConroy BrennanConroy added this to the Next sprint planning milestone May 19, 2021
@ghost
Copy link

ghost commented May 19, 2021

Thanks for contacting us.

We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@halter73
Copy link
Member

halter73 commented Jul 8, 2021

Closing but the context might be relevant for #32375

@halter73 halter73 closed this as completed Jul 8, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 7, 2021
@amcasey amcasey added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-minimal-actions Controller-like actions for endpoint routing
Projects
None yet
Development

No branches or pull requests

8 participants