Skip to content

Nullability leftovers#24428

Merged
roji merged 1 commit intomainfrom
NullabilityLeftovers
Mar 17, 2021
Merged

Nullability leftovers#24428
roji merged 1 commit intomainfrom
NullabilityLeftovers

Conversation

@roji
Copy link
Copy Markdown
Member

@roji roji commented Mar 17, 2021

No description provided.

@roji roji requested a review from a team March 17, 2021 12:31
Comment thread src/Directory.Build.props
<IncludeSymbols>True</IncludeSymbols>
<CodeAnalysisRuleSet>$(MSBuildThisFileDirectory)..\rulesets\EFCore.ruleset</CodeAnalysisRuleSet>
<Nullable>enable</Nullable>
<Feature>nullablePublicOnly</Feature>
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This will reduce our assembly IL size by not generating nullability attributes for non-public APIs.

See dotnet/aspnetcore#29908 for discussion on the same thing for ASP.NET.

#nullable disable
// Need unconstrainted nullable T
private static T ParameterValueExtractor<T>(QueryContext context, string baseParameterName, IProperty property)
private static T? ParameterValueExtractor<T>(QueryContext context, string baseParameterName, IProperty property)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This may look a bit confusing, but remember that T? means defaultable and not nullable in unconstrained generic contexts. So the code below will still throw NRE if T is a value type and no base parameter is found - as today.

@roji roji force-pushed the NullabilityLeftovers branch from 2e6ec9e to 53dcead Compare March 17, 2021 12:37
@roji roji merged commit d568c44 into main Mar 17, 2021
@roji roji deleted the NullabilityLeftovers branch March 17, 2021 18:23
using System.Linq;
using JetBrains.Annotations;

#nullable enable
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why this is still here?
everything under src is nullable enable no?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

These Shared files also get included by the tests, where nullability isn't enabled, so that generates a warning... We can remove it after all the tests are enabled for nullability too :trollface:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

tests shouldn't be using shared files from src/shared folder. If they need then they should be under /shared in root.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We can do that change if we care about it enough...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I find this inconsistent - i.e. test projects using files from src folder through relative path. :trollface:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

But adding a whole new folder at the project root... That's tough too... :trollface:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This documentation may help you.

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