dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.89k stars 4.01k forks source link

Feature: add a way to silence CS8618 and CS8625 #37975

Open jods4 opened 5 years ago

jods4 commented 5 years ago

CS8618 (Non-nullable field 'container' is uninitialized) is the first thing I ran into when trying out nullable references on my projects.

A quick google search shows this has already been asked several times on StackOverflow.

There are several common scenarios where a non-nullable field is not defined in constructor. Strictly speaking, yes it means that field can be seen null between ctor and its later initialization.

In practice, that class may be initialized by reflection (AutoMapper, Dapper, dependency injection, all serialization libs, etc.). Or it may have a Start() or Initialize() method that will always be called before using the class.

In these cases it's not practical to change the type to a nullable reference. You don't want to ! all references just because initialization is not seen by compiler.

Currently the solution to this problem seems to be:

public string nonNull = null!; // mute CS8618

This is a quite ugly pattern, with not-so-clear intention (so warrants a comment?) and uselessly initializes the field.

I'm suggesting C# provides a syntax to mute this warning, like Typescript does:

public string nonNull!;

Here I'm reusing the non-null operator to express: "this is uninitialized but I know better", which is in the spirit of that operator, I think.

RikkiGibson commented 5 years ago

I think this could be useful going by experience using the same in TypeScript. Classes which don't initialize their fields in the constructor are a very common, difficult case to migrate to nullable, IMO.

It does raise concerns about what should be done about the syntax for dotnet/csharplang#2145. If we do this it nudges more in the direction of "parameter null-checking shouldn't use '!' to denote a runtime check."

I would expect to be able to silence the uninitialized warning on properties and events in a similar way:

public string Prop1! { get; set; }
public event Action Event1!;

It also seems like it should be allowed regardless of whether the member has a 'readonly' associated field. (can be initialized via reflection, etc.)

public string Prop2! { get; } // ok
public readonly string Field1!; // ok
IdeaHunter commented 4 years ago

It would be good to have attribute for classes that have whole purpose as a intermediate object to pass a lot of arguments, that would tell compiler that this whole object should be checked when new is used on it Following piece of code is great example why im struggling (to make a reasonable decision) to do a field initialization with default value:

[BarkAtMeWhenIForgotToSetNonNullableOnNew]
public class MultivalueQuery
{
        public List<IFieldValueImpl> Values { get; set; }
        public string Key { get; set; }
        public string Operator { get; set; }
        public override string ToString()
        {
            return $"IN ({string.Join(", ", Values.Select(v => v.ToString()))})";
        }
}

The problem is for pick default value i need to: 1) I have to think what default value should be - should it be string.Empty , <No value had been set>, Invalid field and so on 2) The picked value most likely not fit in existing domain where this class is used 3) Setting default value would not stop me from making an error (forgetting to set something) and just transform null reference error for other error

In this context this feature(CS8618) looks more a sugar to make static analysis happy rather solve a domain specific problem.

ghost commented 4 years ago

Can we add something that indicates a property/field is initialized through reflection and cannot be null? (avoiding CS8618)

Joe4evr commented 4 years ago

Much of this could be solved if we could tell the compiler that the responsibility of initializing certain fields lies with whoever is constructing the instance: dotnet/csharplang#2328

jrmoreno1 commented 3 years ago

I would prefer to see an attribute on the constructor, which says to ignore the errors. You can do the same thing with pragma

pragma warning disable CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider declaring as nullable.

but that's extremely ugly even if you delete the comments.

It would be much nicer if there was a way to indicate either individually or collectively that if this constructor is used, don't worry about the nullability. This would of course be most commonly used for the empty constructor that is required if you have a non-empty constructor, where the empty constructor is required for populating the fields using reflection.

wilfriedb commented 1 year ago

I think this has been solved with the required modifier. This silences CS8618 but adds a error if you still forget to initialize, be it in the constructor or with an object initializer.

But the message is confusing, "Consider declaring the property as nullable" is often not the right thing to do. Most of the times, it's better to add the required modifier. I propose the message to be "Consider adding the required modifier or declaring the property as nullable".

IdeaHunter commented 1 year ago

The required indeed works for this case. The only downside I was able to find it makes it impossible to solve define a new() generic constraint if generic parameter has required props, but this is no blocker for me to increase adoption of nullability, absense of required was a huge blocker

wilfriedb commented 1 year ago

Yes, required isn't a one-size-fits-all solution. The non-nullable reference types still feels a bit bolted-on, but that makes sense considering they had to maintain backward compatibility. Still, it's a huge improvement in usability compared to NET 6, which doesn't have the required modifier.

Halofreak1990 commented 8 months ago

It would be nice to be able to silence CS8618 for properties that I know beforehand won't be nullable when used. We use a lot of POCO classes in our codebase to represent database tables, and since the properties are populated by the ORM, their actual nullability depends on the database schema. Obviously, any nullable properties are marked as such, but that leaves the non-nullable properties giving this warning. Intuitively, I tried to add ! around the affected properties in various places, but ReSharper (or Visual Studio, I don't know which of the two governs this) helpfully refused to add it behind either the type specifier or the property name, and the editor adds a red squiggly when I added it behind the accessor declaration, as it didn't expect it there