Skip to content

Port JetBrains' nullability annotations and clean-up code. #2018

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

Merged
merged 16 commits into from
Dec 5, 2022
Merged

Port JetBrains' nullability annotations and clean-up code. #2018

merged 16 commits into from
Dec 5, 2022

Conversation

teo-tsirpanis
Copy link
Contributor

@teo-tsirpanis teo-tsirpanis commented Jun 12, 2022

The project was using JetBrains.Annotations' nullability annotations that predate C# 8.0's nullability annotations. I enabled <Nullable>annotations</Nullable>, removed [JetBrains.Annotations.NotNullAttribute], and replaced [JetBrains.Annotations.CanBeNull] with ?. Entirely annotating the project is a much bigger undertaking which is left for another time.

I also cleaned-up other parts of the code.

@martincostello
Copy link
Member

Might be some overlap between this and #2012.

@teo-tsirpanis
Copy link
Contributor Author

Yeah, I noticed it after opening this PR, not the first time it happens. 😂

Since mine is bigger and more comprehensive, I recommend to merge it first, and leave the .NET 6 porting work to you.

@teo-tsirpanis
Copy link
Contributor Author

@adamsitnik I synched the PR with the changes #2012 brought. Can you take a look?

@teo-tsirpanis
Copy link
Contributor Author

teo-tsirpanis commented Nov 22, 2022

@adamsitnik friendly reminder. It's getting conflicts.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

The nullability annotations look very good, I wonder how many warnings we get right now? cc @AndreyAkinshin who introduced the JetBrains annotations

When it comes to adding .NET Standard 2.1 target, I am against as in 3 weeks net6.0 will the oldest officially supported .NET Standard 2.1 runtime and we already target net6.0.

Thank you for your contribution @teo-tsirpanis !

Copy link
Member

@AndreyAkinshin AndreyAkinshin left a comment

Choose a reason for hiding this comment

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

I like it!

@adamsitnik once we agree on the set of target frameworks, we can merge it.

@teo-tsirpanis
Copy link
Contributor Author

Please wait before merging, I have to change the title and description, and add a couple more changes I thought.

@teo-tsirpanis teo-tsirpanis marked this pull request as draft November 29, 2022 15:45
@YegorStepanov
Copy link
Contributor

EnableNETAnalyzers adds many new warnings, shouldn't the build fail because of TreatWarningsAsErrors?

common.props:

<EnableNETAnalyzers>true</EnableNETAnalyzers>
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>

@teo-tsirpanis teo-tsirpanis marked this pull request as ready for review December 4, 2022 18:52
@teo-tsirpanis
Copy link
Contributor Author

@YegorStepanov these warnings were already enabled but with the obsolete Microsoft.CodeAnalysis.FxCopAnalyzers package. This PR does it the right way.

@teo-tsirpanis teo-tsirpanis changed the title Multi-target to .NET Standard 2.1, port JetBrains' nullability annotations et. al. Port JetBrains' nullability annotations and clean-up code. Dec 5, 2022
@adamsitnik adamsitnik merged commit 3531c12 into dotnet:master Dec 5, 2022
@adamsitnik adamsitnik added this to the v0.13.3 milestone Dec 5, 2022
@teo-tsirpanis teo-tsirpanis deleted the ns2.1 branch December 5, 2022 11:27
@AndreyAkinshin
Copy link
Member

@teo-tsirpanis @adamsitnik all of our CI builds are red now. Here is the error:

/home/runner/work/BenchmarkDotNet/BenchmarkDotNet/tests/BenchmarkDotNet.IntegrationTests/BenchmarkDotNet.IntegrationTests.csproj : error NU1504: Duplicate 'PackageReference' items found. Remove the duplicate items or use the Update functionality to ensure a consistent restore behavior. The duplicate 'PackageReference' items are: System.Runtime.CompilerServices.Unsafe 6.0.0, System.Runtime.CompilerServices.Unsafe 6.0.0. [TargetFramework=net462]

@YegorStepanov
Copy link
Contributor

I will send a PR soon

@adamsitnik
Copy link
Member

@AndreyAkinshin @YegorStepanov appologies for breaking the build, thanks for fixing it!

@YegorStepanov
Copy link
Contributor

The code looks pretty messy now:
image

I changed to <Nullable>enable</Nullable> to count NRT issues. It gives me 908 issues.

Is there a simple way to suppress them all?
If not, maybe revert NRT until the whole library is annotated?
Also, I think there are some public API that can accept/return nullable types. Wrong annotation can be annoying for users.

If @teo-tsirpanis isn't working on it, I will be happy to do it (~4 months).

@teo-tsirpanis
Copy link
Contributor Author

I set it to annotations to minimize the work I had to do. Feel free to change it to enable @YegorStepanov, I don't have time for it.

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.

6 participants