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

Pass by Value becomes, Pass by Reference when using Variant or RecordRef as Parameter and make assignments to other Records #7312

Closed
SarkeSrb opened this issue Jan 23, 2023 · 15 comments

Comments

@SarkeSrb
Copy link

SarkeSrb commented Jan 23, 2023

When passing RecordRef as Variant by Value and change assignments to a local RecordRef to a different table it acts like a Pass by Reference and changes the value of the passed parameter like it is passed by Reference even if it is not.

*NOTE: This is isolated example problem originated using Data Type Management Codeunit

Steps to reproduce the behavior:

procedure ProcessRecRef()
var
    RecordRefSalesLine: RecordRef;
begin
    RecordRefSalesLine.Open(Database::"Sales Line");
    RecordRefSalesLine.FindFirst();
    Message('RecordRefSalesLine is ' + Format(RecordRefSalesLine.RecordId, 0, 1) + ' Before Pass to function PassRecRefToDoSomethingWithoutDataTypeManagementParameterAsVariant');
    // not passed as var ParameterRecordRefSalesLine
    PassRecRefToDoSomethingParameterAsVariant(RecordRefSalesLine);
    Message('RecordRefSalesLine is ' + Format(RecordRefSalesLine.RecordId, 0, 1) + ' After Pass to function PassRecRefToDoSomethingWithoutDataTypeManagementParameterAsVariant');
    //RecordRefSalesLine is Customer even it is not a var ParameterRecordRefSalesLine
    RecordRefSalesLine.Close();
end;

local procedure PassRecRefToDoSomethingParameterAsVariant(ParameterRecordRefSalesLine: Variant)
var
    CustomerLocal: Record Customer;
    SourceFieldRef: FieldRef;
    SourceRecordRef: RecordRef;
begin
    Message('RecordRefSalesLine is ' + Format(ParameterRecordRefSalesLine, 0, 1) + ' In Function PassRecRefToDoSomething before use DataTypeManagement.GetRecordRefAndFieldRef with ParameterRecordRefSalesLine');
    SourceRecordRef := ParameterRecordRefSalesLine;
    SourceFieldRef := SourceRecordRef.Field(2);
    Message('RecordRefSalesLine is ' + Format(ParameterRecordRefSalesLine, 0, 1) + ' In Function PassRecRefToDoSomething after use DataTypeManagement.GetRecordRefAndFieldRef with ParameterRecordRefSalesLine');
    //SourceRecordRef.Close; will solve the problem in any case this should not happen since it is possible to change the assignment without "close"
    //Do some processing with SourceFieldRef.Value
    //and now I want to use the same variable SourceFieldRef to do some other processing for some other record
    CustomerLocal.FindFirst();
    SourceRecordRef.Get(CustomerLocal.RecordId);
    SourceFieldRef := SourceRecordRef.Field(2);
    Message('RecordRefSalesLine becomes ' + Format(ParameterRecordRefSalesLine, 0, 1) + ' In Function PassRecRefToDoSomething after use DataTypeManagement.GetRecordRefAndFieldRef with Customer');
    //Variant ParameterRecordRefSalesLine becomes Customer
end;

And in documentation it is stated that “If one RecordRef variable is assigned to another RecordRef variable, then they both refer to the same table instance.” But why it is changing pass as value to pass as reference?

  1. Go to https://github.com/SarkeSrb/RecRefBug

3. Expected behavior

procedure Process()
begin
    //How it should be
    Message('How it should be i is 1 before function and after since it is not passed as var');
    i := 1;
    PassVarToDosomething(i);
    Message(Format(i));
    //How it should be i is 1
    //How it should be
    Message('How it should be i is 1 before function and after 5 since it is passed as var');
    i := 1;
    PassVarAsVarToDosomething(i);
    Message(Format(i));
     //How it should be i is 5
end;

local procedure PassVarToDosomething(i: Integer)
var
    j: Integer;
begin
    j := 5;
    i := j;
end;

local procedure PassVarAsVarToDosomething(var i: Integer)
var
    j: Integer;
begin
    j := 5;
    i := j;
end;

4. Versions:

  • AL Language: Latest
  • Visual Studio Code: Latest
  • Business Central: Detected in 20.7 and rechecked in latest 21.3
@dzzzb
Copy link

dzzzb commented Jan 24, 2023

And in documentation it is stated that “If one RecordRef variable is assigned to another RecordRef variable, then they both refer to the same table instance.” But why it is changing pass as value to pass as reference?

It is a record reference. So, even if passing by value, you are passing a reference by value.

If you want to update an instance within the function only, then first .Duplicate() to another RecordRef and work on the latter.

N.B. I don't particularly like this, nor how it is a 'gotcha' that types like List and Dictionary work the same way. But that's just how it is.

@SarkeSrb
Copy link
Author

SarkeSrb commented Jan 24, 2023

And in documentation it is stated that “If one RecordRef variable is assigned to another RecordRef variable, then they both refer to the same table instance.” But why it is changing pass as value to pass as reference?

It is a record reference. So, even if passing by value, you are passing a reference by value.

If you want to update an instance within the function only, then first .Duplicate() to another RecordRef and work on the latter.

N.B. I don't particularly like this, nor how it is a 'gotcha' that types like List and Dictionary work the same way. But that's just how it is.

You are missing the point here. Input parameter is Variant, and problem originated using DataType Management.
RecordRef.Close() also solves the issue but the point is if you pass by a value, you do not expect it to be changed.

@dzzzb
Copy link

dzzzb commented Jan 24, 2023

I don't think I'm missing any point, but it'd be easy to miss in the convoluted original post. As far as I can see, you are passing a Variant containing a reference to a record, which is a copy of that reference put into the variant, so just like any normal record ref copy (assignment or passing directly to a function), updates to the copy are reflected in its source.

@SarkeSrb
Copy link
Author

SarkeSrb commented Jan 24, 2023

Reference should be changed only in the same scope.
Programing language is the one that should create a copy/duplicate or whatever in order not to change the value of the input parameter, if it is passed as a value.
Point is: If you pass as a value, it should be passed as a value if you pass as reference, it should be passed as a reference. Whatever datatype is!
Issue is not about the reference data types. it is about the sole meaning of the programing term pass as a value and pass as a reference.
And it should be fixed in my opinion.

@dzzzb
Copy link

dzzzb commented Jan 24, 2023

Point is: The RecordRef-inside-Variant is working the same as a RecordRef-passed-without-var would. That is, the type RecordRef has reference semantics, even if passed/copied by value. Do I agree with the design? No, there's lots about AL that I don't agree with, but I'm just stating the fact as I see it. The Variant doesn't change anything here, as far as I can see.

You will find the same 'problem' or 'design decision' with things like List, Dictionary, TextBuilder, etc. - is that a good design? No, but little of AL is... and you will go mad if you try to square it with 'how a programming language should work'. In my experience, anyway... And in any case, stuff like that cannot be changed now, as doing so would break loads of existing code.

@navdotnetreqs
Copy link

I agree with @SarkeSrb - a function the parameters of which are not defined as reference, must not be able to change the original variable!

@dzzzb
Copy link

dzzzb commented Jan 30, 2023

That's nice, but that's not how AL works. Some types 'are' references, and thus have reference semantics even when they're not passed by var. And RecordRef is the oldest, prototypical example of this. MS simply cannot change this now, as a decade plus of code that was written with an understanding of this, but did modify the record via the ref, would immediately break.

@SarkeSrb
Copy link
Author

That's nice, but that's not how AL works. Some types 'are' references, and thus have reference semantics even when they're not passed by var. And RecordRef is the oldest, prototypical example of this. MS simply cannot change this now, as a decade plus of code that was written with an understanding of this, but did modify the record via the ref, would immediately break.

Can you please let Microsoft make a decision here thx.
And on the other hand, I think that they are unaware of it since I see in base app that there are a lot of cases when they are passing RecRef as Var if they are aware of this than Var would not be necessary right. if you based your code on this bug than we would be happy to help you refactor it.

@dzzzb
Copy link

dzzzb commented Jan 30, 2023

I'm not trying to make decisions for Microsoft, but I'm pointing out a very obvious issue they must consider before considering changing this behaviour. If you can come here and say 'just change this entire behaviour of the language!', I can say 'no, do not' because I do not have time to have mine or previous developers' existing code broken suddenly; there is enough else to fix.

I don't need any help with my code because I know what I'm doing, and use this as a documented behaviour and not a bug, but thanks for the rather patronising offer :-D

@dannoe
Copy link

dannoe commented Feb 6, 2023

RecordRef in AL behaves like a reference type in c#. If you are familiar with c# have a look at this example:

using System;

var recordRef = new RecordRef()
{
    MyInt = 1,
    MyString = "bar"
};

Test(recordRef);
Console.WriteLine(recordRef.MyInt); // 4
Console.WriteLine(recordRef.MyString); // foo

TestWithoutRef(recordRef);
Console.WriteLine(recordRef.MyInt); // 4
Console.WriteLine(recordRef.MyString); // foo

TestWithRef(ref recordRef);
Console.WriteLine(recordRef.MyInt); // 99
Console.WriteLine(recordRef.MyString); // foobar

void Test(RecordRef recordRef)
{
    recordRef.MyInt = 4;
    recordRef.MyString = "foo";
}

void TestWithoutRef(RecordRef recordRef)
{
    recordRef = new RecordRef(); // assign new RecordRef
    recordRef.MyInt = 99;
    recordRef.MyString = "foobar";
}

void TestWithRef(ref RecordRef recordRef)
{
    recordRef = new RecordRef(); // assign new RecordRef
    recordRef.MyInt = 99;
    recordRef.MyString = "foobar";
}

public class RecordRef  // class: reference type
{
    public int MyInt { get; set; }
    public string MyString { get; set; }
}

Programing language is the one that should create a copy/duplicate or whatever in order not to change the value of the input
parameter, if it is passed as a value.
Point is: If you pass as a value, it should be passed as a value if you pass as reference, it should be passed as a reference.

This is true for value types but not for reference types. see https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/reference-types

dnbp is correct on this point.

@SarkeSrb
Copy link
Author

SarkeSrb commented Feb 7, 2023

Yes, I know many programing languages and they all have, references book or guide or documentation or help. And every single one has its specific behavior documented. Here it is stated:
https://learn.microsoft.com/en-us/dynamics-nav/c-al-function-calls
"You can specify that a parameter is passed to a function by value or by reference.

  • If a parameter is passed by value, then a copy of the variable is passed to the function. Any changes that the function makes to the value of the variable are local changes that affect only the copy, not the variable itself.

  • If a parameter is passed by reference, then a reference to the variable is passed to the function. The function can change the value of the variable itself."

Yes, it is CAL and here is a difference page.
https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-differences

AL is successor of CAL since it is not differently documented it should behave the same.
Nowhere is stated that value should become reference. If they just want to fix documentation it is also fine with me actually, they can decide whatever they want I simply stated that it is not working as it is supposed to, and or how it is described.

@dzzzb
Copy link

dzzzb commented Feb 7, 2023

C/AL behaved exactly the same way with RecordRef as AL does, and it documented (however vaguely worded) the same thing:

image

image

This is long-established behaviour, has not changed, and therefore cannot be changed.

What we probably want here is someone to file a bug at https://github.com/MicrosoftDocs/dynamics365smb-devitpro-pb/issues and request that this warning be far clearer (e.g. 'RecordRef as a type has reference semantics; that is, blah...') and far more prominent e.g. at the top of the page.

@SarkeSrb
Copy link
Author

SarkeSrb commented Feb 7, 2023

And where exactly do you see here in your screenshots that it is referring to a function parameter??? And once again point here is the Function parameter passed as value and passed as reference not variable scope of reference variables.
What I'm thinking now is that with this behavior I can probably wrack calls to a database triggers, I will probably make an example if I would have time, that will wrack every single call to a database trigger that are passing RecordRef as a Variable.

@dzzzb
Copy link

dzzzb commented Feb 7, 2023

Non-var function parameters are passed as-if by assignment/copy.

Anyway, I give up.

@thloke
Copy link
Contributor

thloke commented Mar 20, 2023

A few things to consider here:

  1. The real answer to why this behaves the way it does today is because it's inherited from C/AL. That doesn't mean we can't change it, but it's the reason why it is.
  2. Even though we can change it, we probably don't want to because changing this would be a behavioural breaking change and would impact a lot of live AL code today in unpredictable ways. We need a very good reason to do this. Behavioural breaking changes are very difficult to deal with as it wouldn't result in a straightforward compiler error that can be fixed since it will only manifest at runtime.

The best solution here is to update documentation to call out this behaviour so we have it in writing that it's expected to behave like this. This can be done at: https://github.com/MicrosoftDocs/dynamics365smb-devitpro-pb/issues

@thloke thloke closed this as not planned Won't fix, can't repro, duplicate, stale Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants