VB.NET ".?" Syntax OR GreaterThan Breaking Change

[BlythMeister]

Version Used: Somewhere between the version in msbuild 16.1 and 16.7

Steps to Reproduce:

Code example:

Sub Main()

        Dim map As Map = Nothing

        Console.WriteLine($"1 (Expect Empty): {IsHit(map, 0, 0)}")

        map = New Map() With {.Positions = Nothing}

        Console.WriteLine($"2 (Expect Empty): {IsHit(map, 0, 0)}")

        map = New Map() With {.Positions = New List(Of List(Of String))()}
        map.Positions.Add(Nothing)

        Console.WriteLine($"3 (Expect Empty): {IsHit(map, 0, 0)}")

        map = New Map() With {.Positions = New List(Of List(Of String))()}
        map.Positions.Add(New List(Of String)())
        map.Positions(0).Add(Nothing)

        Console.WriteLine($"4 (Expect Empty): {IsHit(map, 0, 0)}")

        map = New Map() With {.Positions = New List(Of List(Of String))()}
        map.Positions.Add(New List(Of String)())
        map.Positions(0).Add("")

        Console.WriteLine($"5 (Expect Miss): {IsHit(map, 0, 0)}")

        map = New Map() With {.Positions = New List(Of List(Of String))()}
        map.Positions.Add(New List(Of String)())
        map.Positions(0).Add("X")

        Console.WriteLine($"6 (Expect Hit): {IsHit(map, 0, 0)}")

        Console.ReadLine()

    End Sub

    Function IsHit(map As Map, x As Integer, y As Integer) As String

        Try
            If map?.Positions?.Count() > x AndAlso map.Positions(x)?.Count() > y AndAlso Not map.Positions(x)(y) Is Nothing Then

                If map.Positions(x)(y).Equals("X", StringComparison.InvariantCultureIgnoreCase) Then
                    Return "Hit"
                Else
                    Return "Miss"
                End If
            Else
                Return "Empty"
            End If
        Catch ex As Exception
            Return $"Error - {ex.Message}"
        End Try

    End Function

    Class Map

        Public Property Positions As List(Of List(Of String))

    End Class

Expected Behavior:

Output:

1 (Expect Empty): Empty
2 (Expect Empty): Empty
3 (Expect Empty): Empty
4 (Expect Empty): Empty
5 (Expect Miss): Miss
6 (Expect Hit): Hit

Actual Behavior:

Output:

1 (Expect Empty): Error - Object reference not set to an instance of an object.
2 (Expect Empty): Error - Object reference not set to an instance of an object.
3 (Expect Empty): Error - Object reference not set to an instance of an object.
4 (Expect Empty): Empty
5 (Expect Miss): Miss
6 (Expect Hit): Hit

When running code compiled using an older version of the compiler, the map?.Positions?.Count() > x AndAlso map.Positions(x)?.Count() > y AndAlso Not map.Positions(x)(y) Is Nothing If check will return False and the output is as expected. However, on newer versions, this same if will throw a null reference exception.

[Youssef1313]

A simplified repro:

Imports System
Imports System.Collections.Generic

Public Module Module1
Sub Main()
        Dim map As Map = Nothing
        Console.WriteLine($"1 (Expect Empty): {IsHit(map, 0, 0)}")
    End Sub

    Function IsHit(map As Map, x As Integer, y As Integer) As String
            If map?.Positions?.Count() > x AndAlso map.Positions(x)?.Count() > y AndAlso Not map.Positions(x)(y) Is Nothing Then
                Return "DONT PRINT ME"
            Else
                Return "Empty"
            End If
    End Function

    Public Class Map
        Public Property Positions As List(Of List(Of String))
    End Class
End Module

The strange behavior is the following code prints "Empty" (which is expected):

Imports System
Imports System.Collections.Generic

Public Module Module1
Sub Main()
        Dim map As Map = Nothing
        Console.WriteLine($"1 (Expect Empty): {IsHit(map, 0, 0)}")
    End Sub

    Function IsHit(map As Map, x As Integer, y As Integer) As String
            If map?.Positions?.Count() > x Then
                Return "DONT PRINT ME"
            Else
                Return "Empty"
            End If
    End Function

    Public Class Map
        Public Property Positions As List(Of List(Of String))
    End Class
End Module

Given that the previous code prints Empty, that means the condition is false. How an "AndAlso" affects the result? I think that should be a bug, the AndAlso part shouldn't be evaluated.

[BlythMeister]

Oh thanks.

So the issue is actually that the AndAlso is being treated like an And ?

[Youssef1313]

Oh thanks.

So the issue is actually that the AndAlso is being treated like an And ?

Probably something more deeper than that. Let's wait for the compiler team.

[richard-green]

This example writes Fail, which should be impossible:

Module Program

    Sub Main()

        Dim test As Test = Nothing

        Dim result As Boolean = test?.Str?.Length > 0 AndAlso Fail()

    End Sub

    Function Fail() As Boolean
        Console.WriteLine("Fail")
        Return False
    End Function

    Public Class Test
        Public Property Str As String
    End Class

End Module

[richard-green]

Also even this writes Fail:

    Module Program

        Sub Main()

            Dim test As Test = Nothing

            Dim result As Boolean = test?.Bool AndAlso Fail()

        End Sub

        Function Fail() As Boolean
            Console.WriteLine("Fail")
            Return False
        End Function

        Public Class Test
            Public Property Bool As Boolean
        End Class

    End Module

[Youssef1313]

After giving this another look, shouldn't the AndAlso operator give a compiler error in this case? It should be applicable to bool but not Nullable<bool> (i.e. bool?)

[richard-green]

It doesn't give a compiler error - but also even if you compare the Bool to True, it still results in Fail:

Option Explicit On

Module Program

    Sub Main()

        Dim test As Test = Nothing

        Dim result As Boolean = test?.Bool = True AndAlso Fail()

    End Sub

    Function Fail() As Boolean
        Console.WriteLine("Fail")
        Return False
    End Function

    Public Class Test
        Public Property Bool As Boolean
    End Class

End Module

[Youssef1313]

Yes it doesn't give a compiler error, but I was expecting it to as the C# compiler does.

To summarize:

• Case 1: Operand of AndAlso is Nullable<bool>: The compiler behavior is correct based on @CyrusNajmabadi comment below.

  The ideal behavior is compiler error saying that AndAlso operand cannot be Nullable<bool>. But this would be a breaking change, so this is probably a candidate for the first warning wave in VB? Let the compiler team decides. But in anyways, evaluating the second operand when the first is null (Nothing) does make sense.

  Imports System
  
  Module Module1
    Sub Main()
        Dim nullBool As Boolean? = Nothing
        Console.WriteLine(nullBool AndAlso Fail())
    End Sub
  
    Function Fail() As Boolean
        Console.WriteLine("Fail")
        Return False
    End Function
  End Module

• Case 2: Equality comparison between Nullable and bool:

  The compiler currently evaluates comparison between null and True as null, while it should be false.

  Imports System
  
  Module Module1
    Sub Main()
        Dim nullBool As Boolean? = Nothing
        Console.WriteLine(nullBool = True) ' This should print False. But it currently doesn't print anything because the compiler evaluates the expression to null (`Nothing`).
    End Sub
  End Module

[Youssef1313]

@RikkiGibson The behavior stated in "Case 2" violates the language spec:

https://github.com/dotnet/vblang/blob/master/spec/expressions.md#relational-operators

The spec says:

All of the relational operators result in a Boolean value.

However, the example results in a Nullable boolean with value of null.

Fixing this will result in a breaking change. Is that a kind of breaking change you accept?

[CyrusNajmabadi]

VB has supported AndAlso on nullable booleans since we introduced them: https://docs.microsoft.com/en-us/dotnet/visual-basic/programming-guide/language-features/data-types/nullable-value-types

Vb is not C#. Things not allowed in C# may be allowed in VB :)

[Youssef1313]

@CyrusNajmabadi Great. The documentation clearly states:

AndAlso and OrElse, which use short-circuit evaluation, must evaluate their second operands when the first evaluates to Nothing.

But the problem now is how the Equals operator works.

[CyrusNajmabadi]

Fixing this will result in a breaking change. Is that a kind of breaking change you accept?

No. This behavior was intentional, and is likely very widespread. We would not break that.

In terms of the spec, there is likely some room for clarifying language. I believe there are two sections at play here. One is the types of a relational operator (which is boolean for relational operators), one is about lifting operators onto nullable receivers. These combine to allow you to get a nullable-boolean back when comparing nullable-booleans.

[CyrusNajmabadi]

But the problem now is how the Equals operator works.

Will need investigation on the VB side about this. At first glance it seems wrong.

[BlythMeister]

Any updates on this? Seems to still be broken and is making it impossible for us to upgrade our farm of build agents to support .net5 because upgrading would break legacy framework branches/repos.

[jaredpar]

Sorry missed this issue. The change which caused this difference is #38802.

The core issue here is that the result of AndAlso in VB when dealing with nullable values is not just True and False but can also contain the result Nothing. Specifically it means that CType(Nothing, Nullable(Of Boolean) AndAlso False produces the value False. That means that in the case of nullable operations VB is not short circuiting when the left hand side returns Nothing. It must evaluate the right hand side to understand the return.

This has been the design of VB since nullable values were introduced. Unfortunately an optimization bug slipped into the compiler which caused this logic to not be consistently applied to user code. As a result it was hard for customers to predict how their programs would work and hard for us to maintain the broken logic here. After several attempts to maintain the logic we decided it was better to fix the optimization bug.

This does mean that AndAlso is not necessarily short circuiting when nullable values are involved (that has been true though since nullable was added). The code in this case needs to guard against map being Nothing in all of the branches of AndAlso.

Sharp lab demo of the behavior

[BlythMeister]

Hi @jaredpar

Whilst i do appreciate the old behaviour was a bug. The exercise of retrospectively reviewing thousands of lines of VB code to ensure that no code was dependent on the bug (across many historical versions) would require serious person power.

Short of reviewing and updating our codebase, is there anyway to reinstate the previous behaviour so we can upgrade the version of msbuild on our build farm?

This was discovered by a failing unit test, but (as with most codebase) true 100% unit test coverage is a myth, and upgrading msbuild on a build agent could lead to a change in behaviour on deployed applications which is far from ideal.

[BlythMeister]

Even if this is to throw a compiler warning or error if your using the previously correct logic.

Essentially, an "Ambiguous nullable used in AndAlso/OrElse operation" warning

[paul1956]

@BlythMeister you might be better off writing a quick analyzer to find places where this code pattern exists, it would also be quicker.

[BlythMeister]

I wouldn't know where to start doing that.

And it would still require me to modify X number of historical releases since Nullable was introduced to ensure bugs don't creep in on any patches.

[BlythMeister]

As already mentioned in this issue (I've not tested) C# gives a compiler error if you use a nullable bool in a andalso or OrElse expression.

If VB were to do the same (with this change) then I think this makes it clear that there is something for people to look at.

As it is, there could be large numbers of people who are now effected by this change and potentially blissfully unaware!

[paul1956]

@BlythMeister that is what an analyzer would do, it would flag places that had the potential issue. There are samples included in Visual Studio in C# and VB that include all the code framework and test stubs. If you don't want to write it maybe someone else does. The analyzer could throw a suggestion, warning or error. I had a similar issue changing from List to an immutable List on a 100K line code base and just human checks missed many places where the assignment was missing, VB compiler didn't care and tests were failing, the analyzer is less then 100 lines of code and a code fix would not be much larger. This should probably should have shipped with VS when they made the change.

[BlythMeister]

So something baked into msbuild?

Because I have loads of releases in git, adding the analyser to every branch would take a lot of time.

[CyrusNajmabadi]

Because I have loads of releases in git, adding the analyser to every branch would take a lot of time.

Any solution provided by MS would have the same issue for you. You'd still have to apply it to all your branches and whatnot.

I agree with the above. An analyzer/codefix is the most expedient way to go here.

[BlythMeister]

Yeah true.

But i still believe that whilst making the breaking change is sometimes unavoidable.

A change like this being only apparent at runtime, rather than at compile time seems a little harsh.

[jaredpar]

In the general case I strongly agree: having a change in runtime behavior is the least desirable outcome. It would be a much better experience to give guidance at compile time here to make it easier to spot the issue in upgrades.

Unfortunately that just wasn't possible here. We cannot issue a blanket warning whenever customers combine AndAlso with Nullable(Of T) as this is a feature many customers rely on. It's been in place for over a decade now. While I agree the fact that AndAlso doesn't short circuit in all cases is likely surprising to a lot of developers it is also a case that VB.NET specifically designed to have in the language. There is lots of code out there which takes advantage of it and a blanket warning on it would cause a very large number of VB.NET customers friction on upgrade.

I agree with others that an analyzer is the most expedient way to approach this problem. While it's not reasonable for the compiler to warn on every use of AndAlso in combination with Nullable(Of T) (it's a supported language feature that we designed for), It would be reasonable for an analyzer to do so. Essentially a VB.NET analyzer which said "Enforce AndAlso always short circuits" that customers could choose to adopt is very much the type of issue that analyzers were designed to solve.

[BlythMeister]

Having spent most of the day today reviewing all of our VB code if statements with AndAlso or OrElse i think we only had 2 instances which were affected by this change. For me, writing a code analyser for this would be overkill, but i do appreciate this is what they are for.