dotnet / vblang

The home for design of the Visual Basic .NET programming language and runtime library.
290 stars 66 forks source link

Throw Expression #370

Open jrmoreno1 opened 5 years ago

jrmoreno1 commented 5 years ago

C# introduced throw expressions in 7.0 after being requested in https://github.com/dotnet/roslyn/issues/5143, I would like to see the same thing in VB.Net.

Nukepayload2 commented 5 years ago

+1 . When I translate C# samples to VB with tools, I usually see compilation errors like this:

_currentValue = If(newValue, Throw New ArgumentNullException(NameOf(newValue))) ' BC30201: Expression expected
pricerc commented 5 years ago

So that's a shortcut for:

If newValue Is Nothing Then 
    Throw New ArgumentNullException(NameOf(newValue))
End If

_currentValue = newValue

I'd say that what you're describing is a limitation of the translation tool you're using that doesn't know that C#'s traditional ternary statement has been overloaded with a new use-case, which is a hybrid operator-and-control-flow-statement.

I kind-of see how it would work in C#, hi-jacking the ternary format, but I'm not a fan of mixing flow-control and assignment into one statement, and it looks to me like unnecessary obfuscation for not much return. Never mind that it would require changing VB's If operator into something different (as if there aren't already enough variants of If in VB).

This would be very low on a list of enhancements to VB that I'd be interested in.

Berrysoft commented 5 years ago

How about write a function:

Function IfNothingThrow(Of T As Class)(value As T, name As String) As T
    If value Is Nothing Then
        Throw New ArgumentNullException(name)
    End If
    Return value
End Function

And you can call it like

_currentValue = IfNothingThrow(newValue, NameOf(newValue))
paul1956 commented 5 years ago

@Nukepayload2 could you post the original C# code that produced

_currentValue = If(newValue, Throw New ArgumentNullException(NameOf(newValue))) ' BC30201: Expression expected

I want to test my translator to make sure it produces working code.

Nukepayload2 commented 5 years ago

@paul1956 Please try the following c# code:

namespace CreditCardFraudDetection.Predictor
{
    public class Predictor
    {
        private readonly string _modelfile;
        private readonly string _dasetFile;

        public Predictor(string modelfile, string dasetFile) {
            _modelfile = modelfile ?? throw new ArgumentNullException(nameof(modelfile));
            _dasetFile = dasetFile ?? throw new ArgumentNullException(nameof(dasetFile));
        }
    }
}

Copied from https://github.com/Nukepayload2/machinelearning-samples/blob/7b7fe181a3da709d7945fdaa42c3541b9d6ffed2/samples/csharp/getting-started/BinaryClassification_CreditCardFraudDetection/CreditCardFraudDetection.Predictor/Predictor.cs#L20

rrvenki commented 5 years ago

@Nukepayload2, the translation by two different tools gave two different code interestingly. First I used http://www.carlosag.net/tools/codetranslator/ which looks incorrect as below.

Namespace CreditCardFraudDetection.Predictor
    Public Class Predictor
        Private _modelfile As String
        Private _dasetFile As String
        Public Sub New(ByVal modelfile As String, ByVal dasetFile As String)
            MyBase.New
            Throw New ArgumentNullException(nameof(modelfile))
            Throw New ArgumentNullException(nameof(dasetFile))
        End Sub
    End Class
End Namespace

While http://converter.telerik.com/ gave an interesting additional support class code for C#'s throw implementation as below.

Namespace CreditCardFraudDetection.Predictor
    Public Class Predictor
        Private ReadOnly _modelfile As String
        Private ReadOnly _dasetFile As String

        Public Sub New(ByVal modelfile As String, ByVal dasetFile As String)
            _modelfile = If(modelfile, CSharpImpl.__Throw(Of System.String)(New ArgumentNullException(NameOf(modelfile))))
            _dasetFile = If(dasetFile, CSharpImpl.__Throw(Of System.String)(New ArgumentNullException(NameOf(dasetFile))))
        End Sub

        Private Class CSharpImpl
            <Obsolete("Please refactor calling code to use normal throw statements")>
            Shared Function __Throw(Of T)(ByVal e As Exception) As T
                Throw e
            End Function
        End Class
    End Class
End Namespace

@paul1956, please share your tool's output. Could help us all. Also try to make your code translator as VSIX addon to VS.

jrmoreno1 commented 5 years ago

@pricerc : I don’t think they changed operators in c#, they changed throw.

And yes, it does mix assignment with flow. But unlike C’s if (y=z){}, it’s adding control flow to assignment, not assignment to control flow. It’s not going to cause any any confusion or mistakes. And it makes it clear that if you can’t make the assignment that the current scope can’t complete. Your equivalent code doesn’t make that clear because the assignment happens 4 or 5 lines later.

paul1956 commented 5 years ago

@rrvenki it a desktop application, that can do projects, folders and snippets. Source can be found here https://github.com/paul1956/CSharpToVB and it produces garbage for the code above, something I will fix this weekend. My goal was to be able to translate all of Roslyn with comments and reasonable formatting. What I do when VB can't do something directly that C# can, is try to see if I can get something logically similar and if that fails comment out the code and flag with TODO so I can continue. I am waiting for VB16 preview 2 to improve comment translation. Today I keep most/all of the comments, regions, directives but some of the comment are not near where they should be and some directives break VB code but in a very consistent way that a human can easily fix. With the addition of

Partial statement _ ' Comment
rest of statement

I should be able to fix all the comments. If #357 gets implemented directive issues will be fixed.

paul1956 commented 5 years ago

@rrvenki Here is what it produces after the fix

Namespace CreditCardFraudDetection.Predictor

    Public Class Predictor

        Private ReadOnly _modelfile As String
        Private ReadOnly _dasetFile As String

        Public Sub New(modelfile As String, dasetFile As String)
            If modelfile Is Nothing Then Throw New ArgumentNullException(NameOf(modelfile))
            _modelfile = modelfile
            If dasetFile Is Nothing Then Throw New ArgumentNullException(NameOf(dasetFile))
            _dasetFile = dasetFile
        End Sub
    End Class
End Namespace
rskar-git commented 5 years ago

As much as this may seem like a way to invite an abusive level of code golf, I can appreciate this as a means for inline-style values checking, particularly for nulls and out-of-range conditions. The CSharpImpl.__Throw approach is a practical work-around, with a downside of adding a little clutter to the stack trace and a small bit of (Of T) code clutter.

But if @Nukepayload2's C# example could look more like

_modelfile = If(modelfile, Throw New ArgumentNullException(NameOf(modelfile)))
_dasetFile = If(dasetFile, Throw New ArgumentNullException(NameOf(dasetFile)))

instead of

_modelfile = If(modelfile, DoThrow(Of String)(New ArgumentNullException(NameOf(modelfile))))
_dasetFile = If(dasetFile, DoThrow(Of String)(New ArgumentNullException(NameOf(dasetFile))))

that looks a whole lot nicer on the eyes and brain.

====

This being VB, I wonder if we could instead go with having a Throw operator?

' Throw New ArgumentNullException()
_modelfile = If(modelfile, Throw) 

' Throw New ArgumentNullException(NameOf(modelfile))
_modelfile = If(modelfile, Throw(NameOf(modelfile))) 

' Throw New ArgumentNullException("Your own message")
_modelfile = If(modelfile, Throw("Your own message")) 

' Throw New YourCustomException("Your custom message")
_modelfile = If(modelfile, Throw(New YourCustomException("Your custom message"))) 

Hence, it could be Throw on its own, or Throw(string-expression), or Throw(exception-expression).

' Throw New Exception()
_index = If(index >= 0 AndAlso index < 10, index, Throw)
_index = If(index < 0 OrElse index >= 10, Throw, index)

' Throw New Exception($"{NameOf(index)} is out of range")
_index = If(index >= 0 AndAlso index < 10, index, Throw($"{NameOf(index)} is out of range"))
_index = If(index < 0 OrElse index >= 10, Throw($"{NameOf(index)} is out of range"), index)

' Throw New ArgumentOutOfRangeException(NameOf(index))
_index = If(index >= 0 AndAlso index < 10, index, Throw(New ArgumentOutOfRangeException(NameOf(index))))
_index = If(index < 0 OrElse index >= 10, Throw(New ArgumentOutOfRangeException(NameOf(index))), index)

' Throw New ArgumentOutOfRangeException(NameOf(index), "Index must be between 0 and 9.")
_index = If(index >= 0 AndAlso index < 10, index, Throw(New ArgumentOutOfRangeException(NameOf(index), "Index must be between 0 and 9.")))
_index = If(index < 0 OrElse index >= 10, Throw(New ArgumentOutOfRangeException(NameOf(index), "Index must be between 0 and 9.")), index)

So I guess instead of

_modelfile = If(modelfile, Throw New ArgumentNullException(NameOf(modelfile)))
_dasetFile = If(dasetFile, Throw New ArgumentNullException(NameOf(dasetFile)))

it could be

_modelfile = If(modelfile, Throw(NameOf(modelfile)))
_dasetFile = If(dasetFile, Throw(NameOf(dasetFile)))
paul1956 commented 5 years ago

The other place this come up

        public static bool Parse(ReadOnlySpan<char> value) =>
            TryParse(value, out bool result) ? result : throw new FormatException(SR.Format(SR.Format_BadBoolean, new string(value)));

becomes

        Public Shared Function Parse(value As ReadOnlySpan(Of Char)) As Boolean
            Dim result As Boolean = Nothing
            If Not TryParse(value, result) Then Throw New FormatException(SR.Format(SR.Format_BadBoolean, New String(value)))
            Return result
        End Function

@rskar-git for this to be useful it needs to support all the forms of throw, which is why I like the above approach. It is also very clear what is happening. It would be a little cleaner if VB supported creation of out variables and didn't require they be initialized (I think requiring initialization is being fixed in VB16)

rskar-git commented 5 years ago

@paul1956

for this to be useful it needs to support all the forms of throw, which is why I like the above approach.

I'm not sure what you mean - are you agreeing with @pricerc? Does #370 get a thumbs-down from you?

rskar-git commented 5 years ago

@paul1956, a side note, I think there's a missing Not:

        If Not TryParse(value, result) Then Throw New FormatException(SR.Format(SR.Format_BadBoolean, New String(value)))
pricerc commented 5 years ago

@pricerc : I don’t think they changed operators in c#, they changed throw.

you say 'Tomato'...

either way, the ways you can use the ? : construct has changed.

Your equivalent code doesn’t make that clear because the assignment happens 4 or 5 lines later.

The argument about clarity is subjective, but the 4 or 5 lines later is just because I expanded the 'Then' clause. You could just use a single-line If/Then as demonstrated elsewhere, and then the assignment is on the very next line, which is what I usually do for guard statements.

But I usually put all guard clauses together, and then the assignments afterwards. So @paul1956 's example becomes

        Public Sub New(modelfile As String, dasetFile As String)
            If modelfile Is Nothing Then Throw New ArgumentNullException(NameOf(modelfile))
            If dasetFile Is Nothing Then Throw New ArgumentNullException(NameOf(dasetFile))

            _modelfile = modelfile
            _dasetFile = dasetFile
        End Sub

I think it might be because I learnt to code at a time when clock cycles were important and optimizing was done by me, not the compiler. And so I still hate wasting clock cycles on needless assignments.

But to each their own. I still don't like the mixing of flow and assignment; it still looks to me like unnecessary obfuscation and VB trying to be 'like' C#.

For me, I value language stability and ease-of-mainteance over 'cool new tricks' or 'shortcuts'. So I would like VB's future to be about improving the compiler while keeping it an easy-to-master, easy-to-read language, for the benefit of the developers using it. I would prefer new features to be about new functionality and/or fixing anomalous or inconsistent behaviour, not new ways to do old things. For me, I don't think this proposal would achieve these.

jrmoreno1 commented 5 years ago

@pricerc : I wanted it in C# because I found it clear and useful in Perl (where it’s or die), so it’s definitely not trying to be like C# — it’s a syntax/idiom I find useful and readable. Now the fact that I use Perl may cause you to doubt that I understand what “readable” means...

paul1956 commented 5 years ago

@rskar-git If #370 were available I would use it but there is a reasonable workaround. There are many items without any workaround that I would like to see first.

Thanks I missed the Not. Right now I am the only one testing and without VS Enterprise my automated tests don't work.

pricerc commented 5 years ago

@jrmoreno1

RE :Readable. For me, readable means readable by people who are not necessarily programmers.

I have on rare occasions, over the phone, talked someone through editing a bit of VB code* (in a 'plugin' for an ERP package). That I can have someone read VB code to me over the phone, and give them corrections in the same way, without them really knowing exactly what they're doing, is, I believe, one of the great strengths of VB - it can be read by people with a reasonable grasp of English, without them needing to be a nerd. And at the same time, it can generate actual machine code that's as good as that produced by more trendy languages.

I appreciate the 'abbreviating' of common patterns where they are more likely to produce more maintainable (i.e. readable) code. E.g. auto-implemented properties. But (for me) this proposal adds unnecessary complexity - not that the construct in itself is complicated, but that it adds yet another way of doing something we can already do very simply. I don't see that:

_currentValue = If(newValue, Throw New ArgumentNullException(NameOf(newValue)))

is much of an improvement over:

If(newValue Is Nothing) Then Throw New ArgumentNullException(NameOf(newValue)))
_currentValue = newValue

The latter I can easily explain to a novice or lay person. The former would require a few more words of explanation.

And then there's the fact that the latter is more flexible.

While I won't be throwing my toys out of the cot if it's implemented; heck I might even use it. It's just that I think there are better things the compiler writers could be busy with.

* Had that code been in any kind of C-esque langage, I'd have had a much tougher time of it.

Padanian commented 5 years ago

I can't agree more. Even if available, I would still use the traditional syntax, mainly because I manage a group of senior engineers (not native programmers) who find

If newValue Is Nothing Then 
    Throw New ArgumentNullException(NameOf(newValue))
End If

_currentValue = newValue

crystal clear and self explanatory.

xieguigang commented 5 years ago

test Or Die is more VB style

Demo code: IO.vb

If stream.FileExists Then
     stream = stream.ReadAllText Or die("No content data!")
ElseIf Not stream.Match("http(s)?[:]//", RegexICSng).StringEmpty Then
     stream = stream.GET Or die("No content data!")
End If
jrmoreno1 commented 5 years ago

@xieguigang : I would be totally welcoming of a or die(string) or OrElse throw new Exception syntax. I agree that it is more VBish, but am afraid it my be considered too large of a change (modifying how Or works).

Actually I would love it if OrElse was overridden to be a null coalescence in non-Boolean context. The number one reason why I don’t use it more is the questionable syntax.