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

Opt-In for nullable warnings on mutable properties and fields #36951

Closed
Danielku15 opened this issue Jul 3, 2019 · 14 comments

Comments

@Danielku15
Copy link

commented Jul 3, 2019

Type of issue: feature request
Version Used: 3.2.0-beta1-final

Steps to Reproduce:

  1. Enable Nullable Reference Types
  2. Make a null check on a mutable property/field without local copy and use it
class Test
{
    public string? MayBeSet {get;set;}
    public override string ToString() 
    {
        if(MayBeSet != null)
        {
              return MayBeSet.ToLowerInvariant();
        }
    }
}

Expected Behavior:
A warning should be shown on the ToLowerInvariant() call because on multi-threaded environments the MayBeSet property might have been changed from not-null to null between the if and the method execution.

Kotlin has this kind of check, and does not even provide an opt-out. The error they show is:

Smart cast to 'string' is impossible, because 'this.MayBeSet' is a mutable property that could have been changed by this time.

To overcome the error/warning you have to take a local copy or use the other null-operators (!. or ?. to mute the warning)

class Test
{
    public string? MayBeSet {get;set;}
    public override string ToString() 
    {
        var mayBeSet = MayBeSet;
        if(mayBeSet != null)
        {
              return mayBeSet.ToLowerInvariant(); 
        }
        // or 
        if(MayBeSet != null)
        {
              return MayBeSet!.ToLowerInvariant();
        }
        // or 
        if(MayBeSet != null)
        {
              return MayBeSet?.ToLowerInvariant() ?? string.Empty;
        }
    }
}

Please note that this is a simplified example to show the expectation. I am aware that in this example-case you could use ?. to eliminate the null check but there are various scenarios where you will have multiple accesses to the property/field within your if-block.

Actual Behavior:
No warning is shown and a smart-cast from string? to string is done within the if-block.

Proposal:
There should be an opt-in setting to enable mutable property/field checks.

Additional Examples:

class Test
{
    public string[]? MayBeSet {get;set;}
    public string CenterString() 
    {
        if(MayBeSet != null)
        {
              return MayBeSet[MayBeSet.Length / 2];
        }
    }
}

class Test2
{
    public string[]? MayBeSet {get;set;}
    public string CenterString() 
    {
        if(MayBeSet != null)
        {
              Cleanup();
              return MayBeSet[MayBeSet.Length / 2];
        }
    }
    private string Cleanup() 
    {
        MayBeSet = null;
    }
}
@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

This can be provided by a third party analyzer. It's something that doesn't need anything roslyn.

@jcouv

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

This is by-design per current spec for nullable:

For certain expressions denoting variables or properties, the null state is tracked between occurrences, based on assignments to them, tests performed on them and the control flow between them. This is similar to how definite assignment is tracked for variables.

@jcouv jcouv added this to the Compiler.Next milestone Jul 8, 2019

@Danielku15

This comment has been minimized.

Copy link
Author

commented Jul 9, 2019

This can be provided by a third party analyzer. It's something that doesn't need anything roslyn.

Not sure if I can agree with that. Considering the goal to have deterministic knowledge about the null state of expressions having needs considerations of multi-threaded environments. Already my last example shows that you do not even need threads to cause issues. The flow-analysis can only partly cover those cases. You might end up crawling your whole codebase if somebody might call back to your class and could cause a modification of the field/property. The possibility of flow-analysis stops when calling somewhere external that might call back to your instance.

IMHO Kotlin is way more mature on nullable-reference-types as it was part of the language design from the beginning. And I think they are right with considering properties and fields as unsafe when accessed multiple times. They can be changed by threads or simply by other method calls. Already my simple examples show that the null-check of Roslyn today are only covering a part of what needs to be null-safe.

I still think it should be opt-in as for some areas you know it better than the compiler that the code is safe. Always making local copies, !. or ?. accesses produce noise in the code for such cases.

Yes, theoretically a Analyzer would be possible. But I think it is not wise to rely on custom-implementations and potential performance issues to be introduced by additional homebrew checks. Making it part of the core engine sounds more reasonable to me. Already from the whole performance aspects it makes sense to have 1 engine doing the nullability codeflow checks, not multiple.

This is by-design per current spec for nullable:

Can you please extend this statement a little bit? I am not sure if you are confirming my request or if you rather claim that the current spec doesn't foresee my request and the current implementaiton is as-per-spec. 😅

From the spec it is mentioned that the nullability for class members are tracked. And they are. If you have a nullable member and access it without null-safety checks, it will trigger a warning. But subsequent accesses are currently considered safe if a single null-check happened. But I think the "assignments to them" and "the control flow" part of the spec is not precise in respect to modifications that happen.

@canton7

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Always making local copies, !. or ?. accesses produce noise in the code for such cases.

Note that ?. on a nullable value type also assumes that there isn't another thread writing to the field at the same time: SharpLab. This is a necessary assumption, as if Method in field?.Method() mutates field, that needs to be reflected in the actual field and not in a local copy of it.

@canton7

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Also, the only times that you should be writing code which accesses the same field/property multiple times in succession is a) in non-threaded code, and b) where the field/property is protected by a lock. In both of those cases, your suggestion is an annoyance.

@Danielku15

This comment has been minimized.

Copy link
Author

commented Jul 9, 2019

@canton7 I think there is no realistic way of ensuring a) Nowdays almost every application you make in .net/C# is multithreaded in some kind and often you do not even realize that. Also in the world of DI you easily can have dependency graphs that might modify your values on the way.

interface IValueProvider { IEnumerable<int>? Values { get; set; } }   
interface IOtherComponent { void DoSomething(); }
class OtherComponent : IOtherComponent {
    private IValueProvider _provider;
    public void DoSomething() { _provider.Values = null; }
}

class Test
{
    private IValueProvider _provider;
    private IOtherComponent _other;
    public int Run() 
    {
        if(_provider.Values != null) 
        {
            _other.DoSomething(); // you cannot know that _other might change the values unless you know everything about the implementation of IOtherComponent
            return _provider.Values.Sum(); // NullReferenceException
        }
    }
}

As of today the call to .Sum() is considered safe due to the null check but it was modified. Kotlin would enforce you to make this code safe by taking a local copy of .Values and reuse it. C# would simply hide the error and fail on runtime. Also you might just do a null check on a property and pass it on, it would be considered not null on compile time, but is suddenly null on runtime:

interface IValueProvider { IEnumerable<int>? Values { get; set; } }
class WorkerThread {
    private IValueProvider _valueProvider;
    public async Task Run() { // started somewhere
        while(true) { 
            UpdateValue();
            await Task.Delay(100);
        } 
    }    
    public void UpdateValue() {
        _valueProvider.Value = Random() ? null : new int[] {1,2,3};// could be caching, could be calculations that failed etc. 
    }
}

class Test {
    private IValueProvider _valueProvider;
    public void Run() {
        if(_valueProvider.Values != null) {
            DoSomething(_valueProvider.Values);
        }
    }
    private void DoSomething(IEnumerable<int> values) {
        var sum = values.Sum(); // potential NullReferenceException 
    }
}

In Test you do not know about the nature of IValueProvider. If it is used by other threads or not. Locking is also not easily possible unless IValueProvider would expose an API to lock on a common resource. If you work in a small code environment access to whole code area that you know and understand you might be able to handle the nullability. But with DI, libraries taking over anything, different teams working on same modules, TPL,.. you simply cannot be sure. It might be annoyance/noise in the code, but as most of the nullability checks it simply enforces you to write null-safe code.

I think it is a very common pattern to check if something is null and then pass it on to another method.

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

This can be provided by a third party analyzer. It's something that doesn't need anything roslyn.

Not sure if I can agree with that. Considering the goal to have deterministic knowledge about the null state of expressions having needs considerations of multi-threaded environments.

You can have deterministic knowledge that you "do know know" teh null state of any value that is outside of your control. i.e. any value that is not a local/parameter is something that could potentially change out from underneath you. So you can simply mark them all as being "possibly null". The analyzer can tell you that and you can write your code to be safe against those nulls.

@canton7

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

@Danielku15

Either you should make your class fully thread-safe and document it as such, or you should document it as non-thread-safe

That statement is non-controversial, and is followed by every class in the BCL. Either it is thread-safe, which means that it is safe against any multi-threaded usage, or it is not thread-safe, and does not make half-hearted attempts to guard against some random race conditions.

I can only speak for myself and the company for which I work, but I have never been in any doubt about whether the class I'm working on is thread-safe or not. That's nothing to do with DI or TPL or anything else, it's about designing your threading model.

If you're worried about other threads setting properties to null, you also need to worry about:

  1. Properties being set to different values (not null)
  2. Properties which are value types tearing
  3. A set of related properties being changed, but you read a partial view of those
  4. Reads and writes being re-ordered
  5. Many more

You're trying to shoe-horn a guard against one very specific and limited sort of race condition into a feature which has nothing to do with threading.

If you're going to try and warn people when their code isn't thread-safe, that's a much much larger job than just "could this property have been changed to null?". I agree that an analyzer is a much better place to attempt that sort of thing.

You're never going to achieve thread-safety by accident, or following a single guideline. It takes some thought and design. And that's beyond the scope of NRTs.

@canton7

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

I mean, really what you're after is something that tells you whether you read a property twice in the same method, period. Not just whether it might have become null between reads, but whether there is more than one read.

@Danielku15

This comment has been minimized.

Copy link
Author

commented Jul 10, 2019

For me your counter arguments are not fully understandable. The way the compiler behaves today simply is not complete, and I the Kotlin behavior seems way more reasonable to me. Making a null-check on a mutable property should not promote it to not-null because on the next access you cannot be sure if it was changed. If you, as a developer, know it better you need to enforce not-null with the ! postfix-operator or make a local copy in case of doubts. It will not solve the issues in the code that you might have generally because changing values might lead to other unexpected issues. I am fully on your side on that and any other if condition is also prone to this issue (let it be a simple value comparison that is not given anymore). A compiler will never be able to give you warnings on that (well maybe if AI is advancing one day it will? 😄).

But if we limit this scope of the discussion to the NRT and leave full thread-safety out of the game, there are still problematic cases where the simple code flow spoils the nullability. Isn't this compiler feature exactly supposed to assist the devs to guide them not introducing null-bugs to their codebase? You could also claim that null-safety is not achieved by accident and that you need to think it through, design and document it. But yet, the compiler aims to assist the devs there, because it makes sense and there are possibilities to tell the devs they are doing something wrong.

@canton7

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

But if we limit this scope of the discussion to the NRT and leave full thread-safety out of the game, there are still problematic cases where the simple code flow spoils the nullability

Even if you just think about re-entrancy, what you want is much more than "this property might have changed to null".

You also need to think about:

  1. Your method calls into a different type, which calls back into your type and tries to acquire a lock which you already hold
  2. Your method calls into a different type, which calls back into your type and mutates something that you own, such as :
    a. Modifying a collection that you're iterating over
    b. Emptying a list, which you'd previously checked is not empty
    c. etc

Your argument is "If we expand the scope of NRTs a bit, it will solve all of these other problems!". That's patently not the case. I agree that issues like thread-safety and re-entrancy are real problems, but your proposal to expand the scope of NRTs only covers a very very small part of those problems. A threading issue setting a property to null is a very tiny part of all threading issues. A re-entrancy issue setting a property to null is a very tiny part of all re-entrancy issues.

My argument is that while you're correct in that expanding NRTs will catch some extra problems, in fact you catch only a very very small part of the problems you're trying to address. You don't get anywhere near solving threading issues, and you don't get anywhere near solving re-entrancy issues.

The cost is a lot of warnings on code which is in fact perfectly safe. The vast, vast majority of the time, your suggested warnings will not indicate a problem which will happen in practice in the code, and they will just annoy the developer. That's a bad thing.

You're hijacking NRTs to try and solve some other issues, but in doing so you make NRTs more painful to use, and you don't actually get anywhere close to solving those other issues. Trying to solve those other issues requires something more than NRTs, and should not be confused with NRTs.

If you're serious about solving threading or re-entrancy issues, you really need an ownership system like Rust's. That's its own issue, and should not be confused with NRTs.


In all of my years writing code, I have never had to deal with a race condition which set a property to null and caused an NRE, and I have never had to deal with a re-entrancy case which set a property to null and caused an NRE. I have however written if (field != null) field... many times.

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

For me your counter arguments are not fully understandable. The way the compiler behaves today simply is not complete

Correct. And that is intentional. It specifically balances the desire to have total certainty, with the need to not inundate hte user with tons of warnings that are not relevant to them. Warning fatigue is a real thing, and the design here is around the idea of trying to make sure that the most relevant and most likely bugs are the ones that users see.

As i've mentioned a few times, if you want the compiler to be 'complete' it is not hard to do. Just write an analyzer and check for the language considers too noisy today. I assure you, you'll be able to write 100% null-correct code. But at the cost of likely several orders of magnitude more warnings about potential problems. You sound like you're ok with that, so feel free to go that path. The language (and compiler) were written with a different set of values that don't align with yours.

@jmarolf

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

this issue should be moved to dotnet/csharplang where changes to the C# language can be discussed. As @jcouv points out, the compiler is behaving per the spec defined on dotnet/csharplang.

@jcouv jcouv added this to Out Of Scope in Nullable Board Jul 11, 2019

@jcouv

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

I'll go ahead and close this. This is a compromise to deal with existing code which was made explicitly by LDM. Please file a csharplang issue for a language discussion. Thanks

@jcouv jcouv closed this Jul 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.