twinbasic / lang-design

Language Design for twinBASIC
MIT License
11 stars 1 forks source link

Support `Out` keyword #3

Open bclothier opened 3 years ago

bclothier commented 3 years ago

Describe the solution you'd like A useful syntactical sugar to have would be to mark a parameter as Out. twinBASIC allows a parameter to be passed ByVal or ByRef. However, I've often found myself coding this pattern in VBA:

Dim MyThing As Something
If TryGetSomething(OutSomething:=MyThing) Then
  'It's safe to access the MyThing variable here.
Else
  'I can't get to MyThing so I cannot use it. Do something without it.
End If

The advantage of coding in this matter is that the TryGet* neatly abstracts out the mechanics of getting the thing which could potentially fail (may not exist, is not available, whatever). However, there is no compile-time validation that the variable is correctly captured and corresponds to the intention of the code. This would be a misuse:

Dim MyThing As Something
MyThing As New Something
If TryGetSomething(MyThing) Then
  'Did MyThing mutate?
Else
  'I still have a valid MyThing instance?
End If

The Out parameter would disallow above and make it a compile-time error.

We would then declare it as:

Public Function TryGetSomething(Out TheThing As Something) As Boolean

and we can then use it in 3 ways:

Use unassigned local variable

Dim MyThing As Something
If TryGetSomething(Out MyThing)...

Or declare it inline:

If TryGetSomething(Out Dim MyThing As Something)...

Or if we don't care about the out variable:

If TryGetSomething(Out _)...

This would help the API become easier to understand and discoverable, even at the call sites so it's obvious when reading the code, we can see that it's expected to get something out as a parameter.

mansellan commented 3 years ago

Along similar lines, C# (from 7.2) allows in params (passed by reference, but not allowed to mutate in the called function).

A9G-Data-Droid commented 3 years ago

It sounds like you are just wanting to avoid a null check. Wouldn't a null coalescing operator be a better solution?

Dim MyThing As Something
Set MyThing = GetSomething ?? SomethingElseEntirely

OR

Dim MyThing As Something
Set MyThing = GetSomething 
Dim DependantThing As SomethingElse
Set DependantThing = MyThing?.Property
bclothier commented 3 years ago

While this is a great feature and should be its own feature request, no, the Out wouldn't be used for that. Not all retrieval are safe and may cause an error.

Take DAO.Property for example. We can use custom properties but they may or may not exist and that requires error handling to find out.

Ugly way:

Dim prp As DAO.Property
On Error Resume Next
Set prp = CurrentDb.Properties("SomeCustomProperty")
If Err.Number Then
  Err.Clear
  Set prp = CurrentDb.Properties.Append CurrentDb.CreateProperty("SomeCustomProperty", dbText, vbNullString)
End If
On Error GoTo 0
prp.Value = "Hi, there"

Better way:

Dim prp As DAO.Property
Dim prpName As String
Dim prpValue As String

prpName = "SomeCustomProperty"
prpValue = "hi, there"

If TryGetProperty(CurrentDb.Properties, prpName, prp) Then
  prp.Value = prpValue
Else
  CurrentDb.Properties.Append prpName, dbText, prpValue
End If

Ideal:

If TryGetProperty(CurrentDb.Properties, prpName, Out prp) Then
A9G-Data-Droid commented 3 years ago

So couldn't you do:

Dim prp As DAO.Property
Dim prpName As String
Dim prpValue As String

prpName = "SomeCustomProperty"
prpValue = "hi, there"

Set prp = GetProperty(CurrentDb.Properties, prpName) ?? NewProperty(CurrentDb.Properties, prpName, dbText)
prp.Value = prpValue

Or even:

With GetProperty(CurrentDb.Properties, prpName) ?? NewProperty(CurrentDb.Properties, prpName, dbText)
    .Value = prpValue
End With
bclothier commented 3 years ago

No because a runtime error is thrown if a property doesn't exist. In C#, that would be an exception thrown. The .? and ?? operator works only if the return is a null, not an exception.

I'm totally for introducing the .? and ?? on Nothings but that is a different problem than encapsulating potential runtime error and handling it accordingly. I am not sure I want to extend it to cover the runtime errors as well as Nothing result as that would cause more problems than it would solve, I think.

EduardoVB commented 3 years ago

Hello. As for the DAO example, IMO the problem is that CurrentDb.Properties does not have an Exists method (many MS collections lack of it). But that's would be the right way to handle that situation IMO.

About the "TryGetSomething(Out...". Can't that be handled with Is Nothing?

Dim MyThing As Something
If Not MyThing Is Nothing Then
  'It's safe to access the MyThing variable here.
Else
  'I can't get to MyThing so I cannot use it. Do something without it.
End If

How a parameter declared as "Out" would differ from a one ByRef?

bclothier commented 3 years ago

I agree that it'd be the right thing but alas, we can't get them to add it and we may not have the option of adding it unless tB implements something akin to extension methods (which is another scenario entirely).

In C#, the out enforces that the variable must be left uninitialized and also that the function assigns to it. There are no such checks with ref (a la ByRef).

Therefore this code will not compile:

var foo = 123;
GetARandomNumber(out foo);

Nor will this compile:

public bool GetANumber(out int someNumber)
{
     return true;
}

It's those compile-time checks that I want as that helps with guaranteeing the program's correctness.

EduardoVB commented 3 years ago

I get the point. It is more informative (seeing the Out keyword in the code) and error checking (raising an error if it was assigned beforehand or not assigned by the function) than allowing something new.

wqweto commented 3 years ago

In VB6 ByVal parameters produce [in] attribute in the typelib while ByRef produce [in, out].

IMO an additional Out keyword would be useful to be able to produce [out] only parameters too.

The point being that currently VB6 can consume methods with [out] parameters ok and the compiler knows how to call these -- it just makes sure that the actual argument is cleared before passing it i.e. currently in VB6 using this typelib

    [odl, uuid(db6f6ddb-ac77-4e88-8253-819df9bbf140)]
    interface ID3D11Device : IUnknown {
        ...
        void GetImmediateContext(
            [out] ID3D11DeviceContext **ppImmediateContext);
        ...
    };

. . . in this VB6 code

    Dim pDevice As ID3D11Device
    Dim pContext As ID3D11DeviceContext

    Set pContext = GetNonNullContext()
    pDevice.GetImmediateContext pContext

. . . before calling GetImmediateContext the compiler emits extra pContext set to Nothing at callsite to explicitly clear the argument for the [out] only parameter so even if GetImmediateContext does not set/touch its [out] only parameter the value is never "undefined" after the actual method call.

This is much simpler (and sane) philosophy than .Net stance on the subject, keeping in mind that VB6 automatically clears local variables too so we are not used to having "undefined" state in variables/parameters at all and this has worked pretty well to simplify the language for newcommers.

Edit: What will be placed in the output parameter if the function fails? article by Raymond Chen.

WaynePhillipsEA commented 3 years ago

@wqweto Just FYI, that is exactly how we handle [out] args in twinBASIC too, and I agree that is the most sane way of doing it.

I also agree that having an Out keyword in tB would be a useful tool.

Kr00l commented 3 years ago

I suggest the keyword 'ByOut'. This sounds just more "VB-Ish" and to honor the 'ByVal' and 'ByRef' keywords.

EduardoVB commented 3 years ago

The naming of things in VB has a meaning and a sense in English. It should be Out or ForOut IMO.

Clarity is more important than uniformity.

Or check how it is named in other languages, or in VB.NET.

Something I've read some time ago: https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/general-naming-conventions

I (still) didn't read this one, but Wikipedia has an article on the subject: https://en.wikipedia.org/wiki/Naming_convention_(programming)

retailcoder commented 3 years ago

VB.NET doesn't have an equivalent for the C# out keyword, as per this answer on Stack Overflow. This part is particularly interesting:

If you examine code in the framework (for example Double.TryParse), you may see the <OutAttribute> added to parameters, but that only makes a difference when the call is marshalled for COM interop or platform invoke.

I'd much rather have an Out modifier (doesn't need any more characters than that) than to introduce a whole attribute mechanism just for that.

I wish it just looked like this:

Public Function TryParse(ByVal Value As String, Out Result As Double) As Boolean
End Function

And then twinBASIC would simply provide compile-time assurance that Result is assigned a value in every execution path in the implementation of this TryParse method.

retailcoder commented 3 years ago

Actually there is an existing attribute syntax/mechanism... I'm just not sure it works for parameters, but... extending it could be another possibility:

Attribute TryParse.Result.TB_OutParam

I find an Out keyword/modifier more user-friendly, though.

EduardoVB commented 3 years ago

Another consideration is not to add many keywords that cannot later be used in code to name variables because they become reserved words.

And BTW, the addition of these new keywords can break existent code and backward compatibility.

EduardoVB commented 3 years ago

Some alternatives to consider: RefOut ByRefOut OutRef

retailcoder commented 3 years ago

And BTW, the addition of these new keywords can break existent code and backward compatibility.

Not if this parser works similar to Rubberduck's. An Out keyword would parse as some OUTMOD token in the context of some ParamDefinition parser rule, and encountered anywhere else it probably matches some SimpleNameExpression parser rule.

Introducing a new keyword should have no impact on what identifiers are legal or not, in pretty much every context I can think off the top of my head.

EduardoVB commented 3 years ago

OK, good to know that the issue of keywords can work even better than VB6.

Kr00l commented 2 years ago

I have an issue with the INamespaceWalkCB::InitializeProgressDialog in my CNamespaceWalk class.. It crashes in 64 bit mode (VBA or TB) when assigning a custom dialog title. In 32 bit mode it works..

Does it has todo something with the Out ? [out] LPWSTR *ppszTitle

Currently I have this as ByRef lpszTitle As LongPtr

But it won't crash when not "touching" lpszTitle. So when not providing a custom caption..

bclothier commented 2 years ago

[out] MIDL attribute does not really change the semantics; it's more like a flag to let us know that it should be used as a out parameter.

Looking at the header file, the definition is:

        virtual HRESULT STDMETHODCALLTYPE InitializeProgressDialog( 
            /* [string][out] */ __RPC__deref_out_opt_string LPWSTR *ppszTitle,
            /* [string][out] */ __RPC__deref_out_opt_string LPWSTR *ppszCancel) = 0;

Based on comments in this article about header annotation and this thread, it sounds like we need something more akin to:

lpszTitle As Any

which at the call site, you must provide ByVal &H0 when you do not want the title, or otherwise a pointer variable.

I may be wrong, though. I'm a kinda of winging it.

But that said, if the new Out keyword is added, it should be able to handle that case. I know even in C#, it is a pain to deal with. Wayne may remember the back-bending he went through to get C# to play with some of type library API that dislikes getting initialized variables for the out variables.

WaynePhillipsEA commented 2 years ago

[out] without [in] actually does have subtle differences in semantics. It means that the callee implementation should disregard any content already contained in the pointed to variable and not treat it as valid data. Take this C++ example:

   // Note that something is not being initialized to nullptr, so in C++, something will contain whatever crud is on the stack (not necessarily zero!)
   IUnknown* something;    
   myObject->GetSomething(&something);

   HRESULT __stdcall IFoo::GetSomething(IUnknown** outObject)
   {
       *outObject = m_myObject;
       m_myObject->AddRef();
       return S_OK;
   }

All is fine in the above example, since the GetSomething implementation doesn't try to call Release() on the passed in outObject. But a normal ByRef parameter in VBx does not behave like this... ByRef it is considered [in][out], and as such VBx will generate code something like this:

   HRESULT __stdcall IFoo::GetSomething(IUnknown** outObject)
   {
       if (*outObject) (*outObject)->Release();   // this is bad if outObject is an [out]-only parameter, since the value can contain crud data
       *outObject = m_myObject;
       m_myObject->AddRef();
       return S_OK;
   }

The same problem also applies to out-only Strings, Arrays and Variants in VBx, since these types are "managed" and so VBx/tB will try to release the pointed to values when you write to them.

At the moment, if you really must define out-only objects/strings/arrays/variants as ByRef then you need to clear the content of the outObject variable immediately before you touch it (at the very top of your routine), carefully, by using RtlZeroMemory on it in order to clear the pointed to value to zero. Even then, if you're stepping through the code, the debugger will try to read the crud value passed in before you get to set it to zero, and so a crash is likely. The alternative is to redefine the parameters as pointers, and do everything manually, so you can ensure the out-only semantics are enforced.

Kr00l commented 2 years ago

Thanks for the help. I found the solution in my case.

So, for the [out] LPWSTR *ppszTitle I just create a string with CoTaskMemAlloc and assigning this on ByRef lpszTitle As LongPtr But, previously I CoTaskMemFree'ed the string on the object's release. And that was the crash.. I re-checked in tB and it crashes there in 32 bit as well. Just wonder why not in VB6... But now it's resolved.

As the internals of the interface are like: (found on google)

LPWSTR pszTitle = NULL, pszCancel = NULL;

// Retrieve dialog text from callback.
hr = _pnswcb ? _pnswcb->InitializeProgressDialog(&pszTitle, &pszCancel) : S_OK;
if (SUCCEEDED(hr))
{
    [...]
}
CoTaskMemFree(pszTitle);
CoTaskMemFree(pszCancel);