31

I've just been debugging a problem with a function that returns a string that has got me worried. I've always assumed that the implicit Result variable for functions that return a string would be empty at the start of the function call, but the following (simplified) code produced an unexpected result:

function TMyObject.GenerateInfo: string;

        procedure AppendInfo(const AppendStr: string);
        begin
          if(Result > '') then
            Result := Result + #13;
          Result := Result + AppendStr;
        end;

begin
  if(ACondition) then
    AppendInfo('Some Text');
end;

Calling this function multiple times resulted in:

"Some Text"

the first time,

"Some Text"
"Some Text"

the second time,

"Some Text"
"Some Text"
"Some Text"

the third time, etc.

To fix it I had to initialise the Result:

begin
  Result := '';
  if(ACondition) then
    AppendInfo('Some Text');
end;

Is it necessary to initialise a string function result? Why (technically)? Why does the compiler not emit a warning "W1035 Return value of function 'xxx' might be undefined" for string functions? Do I need to go through all my code to make sure a value is set as it is not reliable to expect an empty string from a function if the result is not explicitly set?

I've tested this in a new test application and the result is the same.

procedure TForm1.Button1Click(Sender: TObject);
var
  i: integer;
  S: string;
begin
  for i := 1 to 5 do
    S := GenerateInfo;
  ShowMessage(S); // 5 lines!
end;
mjn
  • 36,362
  • 28
  • 176
  • 378
avenmore
  • 2,809
  • 3
  • 33
  • 34
  • I experienced something similar with a function that returns an interface. There seems to be a general problem with data types that require initialization. I haven't tried dynamic arrays but I guess that would also show this problem. – dummzeuch Jul 15 '10 at 07:08

7 Answers7

35

This is not a bug, but "feature":

For a string, dynamic array, method pointer, or variant result, the effects are the same as if the function result were declared as an additional var parameter following the declared parameters. In other words, the caller passes an additional 32-bit pointer that points to a variable in which to return the function result.

I.e. your

function TMyObject.GenerateInfo: string;

Is really this:

procedure TMyObject.GenerateInfo(var Result: string);

Note "var" prefix (not "out" as you may expect!).

This is SUCH un-intuitive, so it leads to all kind of problems in the code. Code in question - just one example of results of this feature.

See and vote for this request.

Alex
  • 5,477
  • 2
  • 36
  • 56
  • 5
    You can't in good faith call this a feature. Misfeature, perhaps: http://catb.org/jargon/html/M/misfeature.html. But a definite +1 for the link to the docs. – Joe White Jul 14 '10 at 23:51
  • 6
    I do not think that any of the consequences are unintuitive. I have always considered `result` as a local variable, and these generally (integers, reals, booleans, ...) need to be initialized manually. So I react rather strongly to the OP's code that is lacking the `result := ''` first line. – Andreas Rejbrand Jul 15 '10 at 00:08
  • *> I have always considered result as a local variable* Which is **not**. Result is output argument of function. It's not local variable. – Alex Jul 15 '10 at 00:49
  • @Alexander: I wonder why someone gave you a downvote... Your answer is definitely better than mine at least... – Andreas Rejbrand Jul 15 '10 at 01:03
  • @Andreas Rejbrand Well, may be there are people, who actually exploit this "feature" as feature, and they dpon't want it to be "fixed". – Alex Jul 15 '10 at 09:22
  • Maybe this was more intuitive in the old days when we had no "Result variable", but had to assign the result value to the function name instead (i.e. GenerateInfo := 'Some Text' instead of Result := 'Some Text'). That way you could look at a function as "an advanced global variable with an associated code block", and you had to explicitly define the local variables you needed in the function. If you try to use the code GenerateInfo := GenerateInfo + 'Some Text', it just makes no sense (it actually creates an infinite recursive loop). +1 to Andreas' point: Write clean code that makes sense. – Jørn E. Angeltveit Jul 15 '10 at 19:00
  • @Alexander: Even the best can misunderstand the concept of the "Result-variable". See Zarko Gajic at http://delphi.about.com/od/beginners/a/subroutines.htm: "Since every function implicitly has a local variable Result[...]" – Jørn E. Angeltveit Jul 15 '10 at 19:05
  • 1
    @Jørn E. Angeltveit Okay, so you (and Andreas) are saying "init variables before use", right? No problem to me, makes sense. Now, try to apply this rule to function, which returns record with string. How we usually initialize record? Why, by using FillChar, of course! Can you guess what will FillChar do in this case? Since Result is actually an *input* variable - your "code written by good rules" will trash existing string pointer. Not pretty, right? So, it's not safe to think of Result as anything except the thing that it is: in/out argument. – Alex Jul 15 '10 at 20:02
  • 1
    P.S. Note, that using FillChar on *real* local variable **will** work. That's because local variables aren't initialized. Result is not local variable and it, in fact, is **initialized** and may contain non-empty value. That is certanly unexpected. So, attempt to use FillChar fails. – Alex Jul 15 '10 at 20:08
  • 1
    P.P.S. When speaking of "initialized" for local records, it may be a bit fuzzy. Local records aren't initialized in the sense that they aren't filled with zeros. Local records are "initialized" in the sense, that they contains zeroed fields of managed types (strings, interfaces, etc). Hope, you see, what I'm trying to say. – Alex Jul 15 '10 at 20:15
  • @Alexander: Well, I'm not saying that you should initialization everything (in the sense of nil'ing every bit in the variable) before you use it in any way. I'm saying that you should write clean code that doesn't presume anything. If the function simply assigns a value to every field in the record, then that would be a good enough "initialization". So, how do I initialize a record? Create an init-function that initialized every field in the record with the preferred default values, and call this function instead of FillChar. In Delphi 2006 "record constructors" were introduced to do this. – Jørn E. Angeltveit Jul 15 '10 at 21:18
  • Couple of questions: is the exactly the same pointer always recycled to pass to the function from the calling proc's loop? If the compiler changed the param to "out" would all functions always emit a warning if the Result was uninitialised? – avenmore Jul 15 '10 at 23:35
  • First question: I think there is no guarantee for that. Second question: if Result will be "out" someday - yes, there should be a warning, like it is now for, say, Integer. – Alex Jul 18 '10 at 09:07
  • So I see that QC has been closed without beeing fixed or giving any reason why it has been closed? – Wodzu Oct 21 '14 at 08:34
  • @Wodzu QC report was written as bug report. That was before discovery of documentation about "var" vs. "out". Since it is documented, the behavior is not a bug. That is why report is closed. It actually should be "design problem" report. I.e. request to change this behavior. I added a link - hoping it would work as one, but it did not... – Alex Oct 21 '14 at 10:15
  • Yeah it is more like a design problem and we will be falling into this trap once every a while. – Wodzu Oct 21 '14 at 15:15
  • This seems to be right, at least when calling inner functions and function recursively but it can't be the whole story as there are examples that work as you would intuitively expect. See my 'answer' – Clint Good Mar 31 '17 at 02:48
5

We've run into this before, I think maybe as far back as Delphi 6 or 7. Yes, even though the compiler doesn't bother to give you a warning, you do need to initialize your string Result variables, for precisely the reason you ran into. The string variable is getting initialized -- it doesn't start as a garbage reference -- but it doesn't seem to get reinitialized when you expect it to.

As for why it happens... not sure. It's a bug, so it doesn't necessarily need a reason. We only saw it happen when we called the function repeatedly in a loop; if we called it outside a loop, it worked as expected. It looked like the caller was allocating space for the Result variable (and reusing it when it called the same function repeatedly, thus causing the bug), rather than the function allocating its own string (and allocating a new one on each call).

If you were using short strings, then the caller does allocate the buffer -- that's long-standing behavior for large value types. But that doesn't make sense for AnsiString. Maybe the compiler team just forgot to change the semantics when they first implemented long strings in Delphi 2.

Joe White
  • 94,807
  • 60
  • 220
  • 330
  • 2
    The caller do not allocate space but passes a valid reference to an initialized variable, at least the general conception is that as I understand it. It's got reports: http://qc.embarcadero.com/wc/qcmain.aspx?d=894, http://qc.embarcadero.com/wc/qcmain.aspx?d=32556 probably some others as well. – Sertac Akyuz Jul 14 '10 at 22:50
  • 1
    It's a not bug - see my reply. – Alex Jul 14 '10 at 23:39
2

This is not a Bug. By definition no variable inside function is initialized, including Result.

So your Result is undefind on first call, and can hold anything. How it is implemented in compiler is irrelevant, and you can have different results in different compilers.

dmajkic
  • 3,448
  • 1
  • 18
  • 24
1

It seems like your function should be simplified like this:

function TMyObject.GenerateInfo: string;
begin
  if(ACondition) then
    Result := 'Some Text'
  else
    Result := '';
end;

You typically don't want to use Result on the right side of an assignment in a function.

Anyway, strictly for illustrative purposes, you could also do this, though not recommended:

procedure TForm1.Button1Click(Sender: TObject);
var
  i: integer;
  S: string;
begin
  for i := 1 to 5 do
  begin
    S := ''; // Clear before you call
    S := GenerateInfo;
  end;
  ShowMessage(S); // 5 lines!
end;
Marcus Adams
  • 53,009
  • 9
  • 91
  • 143
0

This looks like a bug in D2007. I just tested it in Delphi 2010 and got the expected behavior. (1 line instead of 5.)

Mason Wheeler
  • 82,511
  • 50
  • 270
  • 477
0

If you think that some automatic management of strings are made to make your life easier, you're only partly right. All such things are also done to make string logic consistent and side-effects free.

In plenty of places there are string passed by reference, passed by value, but all these lines are expecting VALID strings in whose memory-management counter is some valid, not a garbage value. So in order to keep strings valid the only thing for sure is that they should be initialized when they firstly introduced. For example, for any local variable string this is a necessity since this is the place a string is introduced. All other string usage including function(): string (that actually procedure(var Result: string) as Alexander correctly pointed out) just expects valid strings on the stack, not initialized. And validness here comes from the fact that (var Result: string) construction says that "I'm waiting for a valid variable that definetly was introduced before". UPDATE: Because of that the actual contents of Result is unexpected, but due to the same logic, if it's the only call to this function that has a local variable at the left, the emptiness of the string in this case is guaranteed.

Maksee
  • 2,311
  • 2
  • 24
  • 34
0

Alex's answer is nearly always right and it answers why I was seeing the strange behaviour that I was, but it isn't the whole story.

The following, compiled without optimisation, produces the expected result of sTemp being an empty string. If you swap the function out for the procedure call you get a different result.

There seems to be a different rule for the actual program unit.

Admittedly this is a corner case.

program Project1;

{$APPTYPE CONSOLE}

uses System.SysUtils;

  function PointlessFunction: string;
  begin
  end;

  procedure PointlessProcedure(var AString: string);
  begin
  end;

var
  sTemp: string;
begin
  sTemp := '1234';
  sTemp := PointlessFunction;
  //PointlessProcedure(sTemp);
  WriteLn('Result:' + sTemp);
  ReadLn;
end.
Clint Good
  • 820
  • 6
  • 14