Please vote for http://qc.embarcadero.com/wc/qcmain.aspx?d=90482
Please vote for http://qc.embarcadero.com/wc/qcmain.aspx?d=90482
You need to use the Windows QC Client to do voting and remember that you can put 10 votes into an issue.
You need to use the Windows QC Client to do voting and remember that you can put 10 votes into an issue.
You can vote and add 10 votes through web client, too.
ReplyDeleteI'm not sure I understand the bug. Any way you mix objects and interface references (outside of ARC) can potentially result in a bug, which is why you should mix them with care and only after you understand what you are doing. Even if the compiler would add this specific scenario (and I'm not really sure exactly how), it won't be able to handle many other similar situations.
ReplyDeleteMarco Cantù The bug is that this works:
ReplyDeleteprocedure Test(const intf: IInterface);
//...
var
i: IInterface
begin
i := TInterfacedObject.Create;
Test(i);
end;
and this does not (i.e it either leaks or might run into exceptions depending on how subsequent calls are made):
begin
Test(TInterfacedObject.Create);
end;
See the answer of Barry Kelly which is linked in the QC issue as he confirms that it is a compiler bug.
Dalija Prasnikar Hah, didn't know that because it does not log me in there automatically although it logs me in on the EDN automatically.
Definitely an annoying bug! I now got used to declaring an interface and make the assignment, like in Stefan Glienke example, but now and then someone in my company forgets (or is unaware of) the problem creating various errors expecially at shutdown of the application.
ReplyDeleteGiacomo Degli Esposti We just had that topic at work today because I am used to const all string, record, interface and object (since the introduction of ARC) arguments. And we definitely have some code around that directly creates something into the method call. Occasionally I solve that by writing TInterfacedObject.Create as IInterface.
ReplyDeleteStefan Glienke I know that is a big issue. But this is how the language was designed. When you call TInterfacedObject.Create how can the compiler magically add a reference only if required? The "supposed bug" is that the compiler creates a temporary instance variable, rather than a temporary reference variable, right? But doing the opposite in general would be wrong... I think there are enough ways a developer can work around the issue, but maybe only because I cannot see an actual solution... outside of ARC!
ReplyDeleteHmm, I just noticed that assigning the result of the constructor to an object variable and then passing that causes the same issue. I thought it would work then. But it makes sense if I think about it... But since I have the reference in a variable I can still call Free on it while I cannot in the other case.
ReplyDeleteYes, that's the issue. I'm not saying it is not hurting developers, it is. So we could certainly try looking for a solution... updating the language specification.
ReplyDeleteMarco Cantù How about raising a compiler warning because it's potentially unsafe code?
ReplyDeleteI think in the age of tools that analyze code and create metrics about how maintainable your code is (I am thinking about things like NDepend maintainability index) a compiler should at least tell the developer about possible issues.
that's why i prefer to not use const with interfaces
ReplyDeleteMarco Cantù solution is quite simple. It merely requires adding hidden interface reference and assigning newly created object to that reference at place of call.
ReplyDeleteCompiler has enough information to know what should be done there.
What needs to happen in the background would be:
Test(i: IInterface := TInterfacedObject.Create);
Stefan Glienke It is compiler bug and it should be solved with working solution and not warnings. I don't think that implementing warnings would be any less work than fixing the actual bug.
ReplyDeleteMarco Cantù Updating the language specification seems to me to be a cop-out, in that we know there is a problem but we can't be bothered dealing with it. Surely the compiler has to insert some code to type-cast the newly created object to an interface, so why can't it just add some more magic code to assign it to a hidden variable? Yes it is a special case but surely there are others within the compiler.
ReplyDeleteAlexander B. Which introduces a performance penalty (yeah, PO and all that) but it also introduces, IMO, inconsistency in to the code base. I prefer that all my parameters to be 'const' if I know that reference is not going to change within that method.
Dalija Prasnikar I partially agree but still I am not sure because it would create a special case for passing the result of a constructor call (or functions that returns object references at all).
ReplyDeleteIf you pass an object reference to a const interface parameter it also leaks it. The thing there is you still have the reference and can free it.
However I would welcome a fix for this problem but even if they don't do it, I want to see a warning so I can detect dangerous code.
To argue against a change:
1)
begin
Test(TInterfacedObject.Create);
end;
is the same as:
2)
var
o: TInterfacedObject;
begin
o := TInterfacedObject.Create;
Test(o);
end;
Why should it generate different code for 1 than for 2?
Stefan Glienke Because there is
ReplyDelete3)
var
o: IInterface;
begin
o := TInterfacedObject.Create;
Test(o);
end;
And 1) doesn't have to translate to broken 2), but it can translate to working 3)
And 2) is clear case of mixing object and interface references that is not something you should do in non-ARC Delphi code or at least you have to know exactly what you are doing and why.
Dalija Prasnikar I just wanted to point out that it potentially creates an inconsistency - if that solves a problem and does not create one I am all for it :)
ReplyDeleteStefan Glienke You are looking at it from wrong POV. I think that it should be looked from POV of procedure that clearly expects interface reference, and constructing object should fulfill that expectation. In that case there is no inconsistency.
ReplyDeleteDalija Prasnikar To me is the compiler tries to infer too much from what the developer is trying to do (guessing from intentions by looking to a called function to chance the callee compiled code) it will get into a dangerous road. When you say it shouldn't convert the code to broker 2 but working 3, it is an option. A developer might want the compiler to generate version 2, as this is legitimate and (if coded properly) will work.
ReplyDeleteNicholas Ring What I mean is that the current language specification (and no, there isn't a formal reference) indicates the current behavior is expected. If we want to get a different behavior (and I'm not 100% sure, quite the opposite) we need to change how the language works. Needless to say this will affect existing applications, changing the current behavior (which is generally not ideal). So maybe we need to add a compiler directive, which would increase complexity further...
Marco Cantù There is no guessing needed. It is plain simple, newly created object during function call can be passed either as object reference or interface reference, former results in non working code, latter works and matches declared function parameter.
ReplyDeleteAlso, I can safely say that any developer that might write such code is trying to have properly working code instead of failing one.
"Newly created object" is not such a precise concept. You can call a constructor, a function returning a new object, use a singleton, and pretty much many other cases.
ReplyDelete"Either as object reference or interface reference, former results in non working code, latter works": this is very simplistic. Either one of the other is correct, depending on how you write the code.
Also, changing the behavior (after 10 years) implies that thousands of existing applications, then recompiled, will do something different. Even if better, this will still break many existing applications. Which is something to be done with extreme care.
I personally wrote about this is my Mastering Delphi books, so it has been known for quite some time.
Why don't we rather focus on finding a workaround or some ways to circumvent the bug, or try to look for some way to introduce what you want without chancing the behavior of existing code.
"Also, changing the behavior (after 10 years) implies that thousands of existing applications, then recompiled, will do something different. Even if better, this will still break many existing applications. Which is something to be done with extreme care."
ReplyDeleteYou write that as if this was something very common. If it were then thousands of applications would have memory leaks that would be fixed with solving this issue. And if you don't have a memory leak then you manually manipulated the refcount which is very uncommon I think. You rather write correct code.
"Why don't we rather focus on finding a workaround or some ways to circumvent the bug, or try to look for some way to introduce what you want without chancing the behavior of existing code."
I already suggested one. Put a warning so I can find such places in my code and fix them by adding an as IInterface. Problem solved.
I stopped using the non-web QC clients a while ago because of security reasons, and the QC web-client because of usability reasons.
ReplyDeleteMarco Cantù I understand that each implementation of the "Pascal" specification is an interpretation of if but it seems that the current interpretation is faulty.
ReplyDeleteQuote: "Needless to say this will affect existing applications, changing the current behavior"
Didn't the introduction of Unicode do this? But it was all for the better, right? and there has been others changes that broke things, right?
I do have a question for you (Marco Cantù ). How does the compiler know to covert an object instance into an interface reference? There must be some compiler magic which handles that situation and since the compiler is multi-pass (from what I last read, even it people treat it as single-pass), couldn't it look at the previous steps to see if it just created the object and if it did, do what Stefan Glienke suggested and throw a warning out. Yes, it is easy to suggest this type of thing than it is to implement it but we also a paying customer...
Marco Cantù I am not talking about factories, functions, singletons or anything that explicitly returns object reference. I am not talking about fixing the whole mess of mixing object and interface references in non-ARC Delphi.
ReplyDeleteI am talking about special case of using constructor inside function call that expect interface as parameter. Since there is no explicit intent that new object should be passed as object reference, and function expects interface reference, logical and only working solution is introduction of hidden interface reference that would keep object alive during function call.
Stefan Glienke I appreciate your input on this issue. It is true I might be exaggerating the numbers, but the problem of backwards compatibility is something to take seriously. It is not a dogma, but a subtle low level change is at times worse than a significant new feature with side effects.
ReplyDeleteNicholas Ring The compiler is single-pass, AFAIK. When you pass (or directly assign) an object to an interface, it automatically extracts the given interface, like when you do "myobject as Imyinterface". But without generating an extra temporary.
Dalija Prasnikar Two issues with "special case of using constructor inside function call that expect interface as parameter". Why that special case and not another one? Adding special cases to a language is generally not a great idea (even if there are some in the current implementation). Second, What if you do "TButton.Create" and pass that as a parameter to a function expecting an interface? All TComponent classes implement IInterface but "disable" the reference counting, so you might not want to do the same.
In general, if there is a way for a developer to ask the compiler for a specific behavior (like there is in this case, it seems) I think it is better to avoid the compiler to try to infer too much form the user intentions... Now having said this, I'm not a compiler developer so I certainly don't have the final word in a matter like this.
Marco Cantù Backward compatibility, seriously.....
ReplyDeleteIn fact the above bug got referred to me by a friend that indicated such bugs are the main reason he is not using interfaces and other modern language features and has recommended all the companies he has done consulting for to do the same.
ReplyDeleteHis reasoning is as follows: I won't use Delphi language features until they are bug free because I need to be able to trust the compiler so I can focus on the bugs in my own code, not in the compiler's bugs.
Jeroen Wiert Pluimers I understand not using a compiler until is is bug free. But to me this is not the case. One could argue, you should understand how the compiler behaves. I understand someone saying "the compiler doesn't work like I think it should, so I'm not using that feature until the compiler changes".
ReplyDeleteMarco Cantù this is about word twisting. Please understand that various groups of people do not use certain language features by intent because somehow it doesn't work for them. Either because of bugs, specs being vague, not understanding, etc. It is important to find out about these and resolve the issues. In this case - especially with the comments of some very knowledgeable people - it is something that should be fixed as being a compiler bug. How it is fixed (hint/warning/error to educate about effects, change the run-time result, or otherwise) is a different matter.
ReplyDeleteJeroen Wiert Pluimers I don't think it is word twisting, but I understand your point of view. We should certainly try to improve language features in terms of the features themselves and their documentation. But I often see a group claiming of bugs for something they want to work differently... and another group having the opposite view. If we remove the word "bug" and discuss the "how this is fixed/improved", it is often easier. For some of the language features (not this specific scenario) education and documentation are critical.
ReplyDeleteMarco Cantù this is not the case where compiler is not behaving the way we think it should, but this is clear, plain bug. Period.
ReplyDeleteThere is compiler magic going on when you
have
var
i: IInterface;
..
i := TInterfacedObject.Create;
and the same magic should apply when you have
procedure Text(const i: IInterface);
...
Test(TInterfacedObject.Create);
Now, compiler passes that object as object reference and that results in either memory leaks or crashes depending on code inside Test. There is no third option and code that would mysteriously work properly when using such construct.
I agree with Dalija Prasnikar. In this case we don't need to be backwards compatible because that compatibility is about something broken. I agree that sometimes a certain behavior cannot be changed because it would change lots of applications out there. But in this particular case it's just wrong code that might not blow up that's all. Yes, some people might have worked around this but honestly I would not care about that. Just like you did not when people used string to store binary data before Delphi 2009. It was just wrong, period.
ReplyDeleteSometimes you have to make a cut and get rid of baggage from the last millenium.
Marco Cantù I have been dancing around this bug since forever, and I can dance around it for some more. If you cannot fix it right now, because there are other more important issues right now, fine. I may not like it, but I can understand it.
ReplyDeleteBut there is nothing you can say that will convince me that this is not a bug, that compiler would have to play guessing games and that fixing it would cause troubles to some imaginary code.
So I'll say nothing. I think I've used interfaces in Delphi more than you did, starting in Delphi 6. To me either the compiler always creates a temporary when there is an interface cast to a parameter, or never does it. Just my opinion.
ReplyDeleteMarco Cantù Sure, you can do that but to what end?
ReplyDeleteIf you know interfaces as well as you claim, then you must know that doing so will not change a thing for object instances that have reference counting disabled (only achievement there would be having completely superfluous calls to _AddRef and _Release). And for reference counted objects the whole hell would break loose. But, I am sure that you know that. That is basic must know rule for anyone using interfaces: you never, ever store reference counted object instance to object reference.
But anyone that knows how to use reference counted objects would never have object reference to such object, so it could not possibly pass such reference to any procedure that expects interface as parameter. Of course, I bet that somewhere in the wild you could find people doing that and getting away with it, because by pure luck they managed to avoid triggering reference counting on their object instances. In that case assigning object reference to hidden interface would result in crashes to otherwise working code, but that code was working by accident and is inherently broken, so maybe crashing would be a chance for those developers to learn how to use them properly.
To summarize, only valid parameters you can pass when interface is expected are interface references and object references with disabled reference counting. And of course, the third option that is covered by QC in question, is calling object constructor directly during procedure call, and the only place where it is logical to add hidden interface reference.
So if you think that the only "proper" way to fix this issue is assigning all object references to hidden interface reference, before making procedure call, you can do that. But I am sure that some day, someone reading that piece of compiler code will be saying WTF a lot, while code fixing only constructor would not get such treatment.
Marco Cantù The compiler is smart enough to generate temporary implicit variables when you use as on the return value of a constructor. So the compiler can take context into account. In that scenario it knows that nothing else has taken a reference, and something needs to do so. So it cannot be the case that it is hard to fix this as everyone suggests.
ReplyDeleteI'm sure that that the behaviour is as designed. It's just that the design is bogus. Time to redesign.
My code has a liberal sprinkling of sections like this:
Intf := GetNewIntf;
DoSomething(Intf);
Naturally I'd sooner write:
DoSomething(GetNewIntf);
but that leaks.
I really care deeply about things like this. Having to write this code in two lines and introduce (and name) a local variable makes my code harder to read.
I cannot understand why Emba won't deal with this. But it's been present for getting on 15 years, so I don't hold out any hope.
David Heffernan Apparently, the problem is not in making fix, but some really bad code that might get broken by that fix.
ReplyDeleteDavid Heffernan "The compiler is smart enough to generate temporary implicit variables when you use as on the return value of a constructor", yes this is possible. However, you write:
ReplyDeleteIntf := GetNewIntf;
DoSomething(Intf);
and I guess this is not using a constructor, if not indirectly. So while we could add special code to add a reference when passing to a function the result of a constructor, how can the compiler know it has to treat a generic function call like GetNewIntf? Actually if that return an interface, you probably do get a temporary and that might already work. The issue i when you are returning an object (with no ref count) and pass it to a function expecting an interface...
There are many different scenario, some of which the compiler could handle more easily than others. But this could create a lot of inconsistency. The current behavior, even if somehow ugly and annoying, is quite predictable...
Marco Cantù What happens with ARC? Surely the exact same issue arises there? I cannot believe that it has not been dealt with under ARC.
ReplyDeleteAnd you are right that GetNewIntf as a function results in an implicit local. So no problem there. The problem is indeed just when a constructor is passed as an argument.
DoSomething(TFoo.Create)
I cannot believe that it is beyond Embarcadero to fix this. It's been at least 15 years now. I think it's a poor show that you've not dealt with this.
Who says it hasn't been considered? There is an attribute, [unsafe] to help in such scenarios.
ReplyDelete+Marco We don't want unsafe. We want something to take a reference. We need more reference counting here, not less.
ReplyDeleteDoes the analogous code leak under ARC?
ReplyDeleteDavid Heffernan It does not work under ARC compiler either. Just differently. It leaks when you create object in call where parameter is declared as const.
ReplyDeleteBroken all the way.
Wow, that blows my mind. A defect known about for all this time, and they replicate it in ARC. Insane. And trivial to fix.
ReplyDeleteDavid Heffernan Probably nobody bothered to test on ARC before... and this was originally non-ARC compiler issue.
ReplyDeleteNew report including ARC compilers https://quality.embarcadero.com/browse/RSP-17071
ReplyDelete