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

Nullability #3287

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Conversation

Applesauce314
Copy link
Contributor

The issue in #3280 would never have happened if reference type nullability was enabled in the project.

decided to do something about it.

this focuses mainly on updating annotations (but a very small amount of behavioral changes were made), after I realized the scope of this effort, i focused mainly on this as the annotations should not affect the behavior of the application.

This is the sort of thing i should have asked if it was desired before dumping this on your la, but before i realized i was doing it i was quite a bit into it, so i'm going to throw it and see if it sticks.

Problem

Nullability errors/warnings were not processed.

Solution

  • primarily focused on annotating obvious errors in nullability annotations e.g. return null - make the function return type nullable, annotating out parameters with [NotNullWhen(true)] where appropriate, matching nullability between base classes/ interfaces,
  • Which part of this PR is most in need of attention/improvement?
  • At least one test covering the code changed

@Applesauce314 Applesauce314 marked this pull request as ready for review September 23, 2024 22:28
@Applesauce314
Copy link
Contributor Author

as mentioned above, decided to add some nullability annotations,
should have discussed first, but its done now...

…ks that do not have the nullability attributes
@siegfriedpammer
Copy link
Member

should have discussed first, but its done now...

Yes, indeed... :(... 554 files is too much. Even more so, as we recently have merged a larger refactoring PR and I would like to make sure that we are stable before merging "the next big thing". But even if we weren't in that situation, I don't think I would be sensibly able to review this in the next 10 years. It's either take it or not and I don't want to take that responsibility, if something breaks. Not now.

Can we find a solution by breaking this into smaller pieces (per project, per feature, ...)?

Just to be clear: the huge amount of work is what deterred us from going full NRT in the past and I think that the complexity of introducing NRT in existing codebases (especially ones that are tied to older frameworks and you have to f*** around with ReferenceAssemblyAnnotator, which has not received updates anymore recently) unfortunately currently outweighs its utility in finding bugs.

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.

2 participants