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

Consider enabling nullable references #724

Open
Barsonax opened this issue Jun 6, 2019 · 8 comments
Open

Consider enabling nullable references #724

Barsonax opened this issue Jun 6, 2019 · 8 comments
Labels
Cleanup Improving form, keeping function Core Area: Duality runtime or launcher DevTool Area: Development tools and environment Discussion Nothing to be done until decided otherwise Feature It doesn't exist yet, but I want it

Comments

@Barsonax
Copy link
Member

Barsonax commented Jun 6, 2019

Summary

Enabling nullable references will help spot null reference bugs while also providing better self documenting code to endusers.

Analysis

  • Statically checked null reference errors (which show up as warnings by default)
  • Requires C# 8.0 and thus VS2019. C# 8.0 is not completely finished yet. Duality is currently using C# 7.3.
  • For endusers that do not work with duality source code nothing changes except that if they use nullable references they can make use of the annotations we added.
  • Adding the annotations can be done step by step. Its not necessary to add these to the whole codebase in one go.
  • Enabling nullable references can be done at project level or even source file level if desired.
@Barsonax
Copy link
Member Author

Barsonax commented Jun 6, 2019

#698 is a prerequisite of this

@ilexp
Copy link
Member

ilexp commented Jun 10, 2019

I'm all for nullable reference tagging 👍 There will be some work to do regarding the C# update, as I'm not a fan of introducing all the modern C# features into the codebase from a style perspective, but as soon as that's out of the way, we can start annotating.

@ilexp ilexp added Cleanup Improving form, keeping function Core Area: Duality runtime or launcher DevTool Area: Development tools and environment Discussion Nothing to be done until decided otherwise Feature It doesn't exist yet, but I want it labels Jun 10, 2019
@ilexp ilexp added this to the General milestone Jun 10, 2019
@ilexp
Copy link
Member

ilexp commented Aug 2, 2019

Since this requires C# 8.0, which requires .NET Core 3.0, I'm moving this to the future upgrade milestone.

@Barsonax
Copy link
Member Author

Barsonax commented Aug 2, 2019

C#8.0 does not require .NET Core 3.0. Msbuild 16.x is enough.

@ilexp
Copy link
Member

ilexp commented Aug 2, 2019

I've so far only found sources that state it does, at least regarding official support, see here for example:

Because C# 8.0 has been built for .NET Core 3.0 and .NET Standard 2.1, it will not be supported outside of .NET Core 3.0 and any platform implementing .NET Standard 2.1.

Do you have an official docs / blog statement that clears things up, or updates this with newer info?

@Barsonax
Copy link
Member Author

Barsonax commented Aug 4, 2019

I believe thats because some features rely on some framework types and runtime enhancements. However a feature like nullable references does not (though its still in preview and subject to change). Iam using C# 8.0 in Singularity https://github.com/Barsonax/Singularity/blob/master/src/Singularity/Singularity.csproj

I think the best course of action is to wait a bit more for now since we have more work to do and nullable references are still in a bit of a preview state (even though previously I stated it was not) but my guess is that you can use C# 8.0 without problems except that not all language features are available but I believe that nullable references alone are enough reason to eventually adopt C# 8.0.

@ilexp
Copy link
Member

ilexp commented Aug 4, 2019

I think the best course of action is to wait a bit more for now since we have more work to do and nullable references are still in a bit of a preview state [..]

Yeah, I'm thinking something similar. I've extracted all C# 8.0 items into a separate milestone that we can tackle in the future. The C# 7.3 / .NET Standard one can be continued and done immediately and is a prerequisite for all the rest anyway.

[..] but my guess is that you can use C# 8.0 without problems except that not all language features are available [..]

I'm worried about maintenance implications though, since we'd depart from what's officially supported and can't easily take it back once we're depending on C# 8.0 features. An alternative that might be preferable would be to actually switch to .NET Core 3.0 for launcher and editor alongside the C# 8.0 upgrade once they're all released and stable - needs to be done "at some point" anyway with .NET Framework being superseded by .NET 5. Either way, I think we can just see about that when we get to it.

@Barsonax
Copy link
Member Author

Barsonax commented Aug 4, 2019

I'm worried about maintenance implications though, since we'd depart from what's officially supported and can't easily take it back once we're depending on C# 8.0 features.

Agreed thats why I think its better to wait a bit for now. Lots of other stuff to fix/improve anyway and maybe by that time its more clear if this is a safe route to take or not or else we just wait for .NET Core 3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup Improving form, keeping function Core Area: Duality runtime or launcher DevTool Area: Development tools and environment Discussion Nothing to be done until decided otherwise Feature It doesn't exist yet, but I want it
Development

No branches or pull requests

2 participants