Bad return signature in pre-constructor leads to null instance
Got a bug for the team. Don't ask me how long it took me to track it down. Consider this code and the effect of running it: ============== public class Foo { bad_pre_constructor(); fun float bad_pre_constructor() { <<< "bad_pre_constructor, this =", this >>>; } } Foo bad_foo; <<< bad_foo =", bad_foo>>> ============== % chuck foo.ck bad_pre_constructor, this = 0x634720 bad_foo = 0x0 ============== Note that the instance of bad_foo is null. Now consider this code: ============== public class Foo { good_pre_constructor(); fun void good_pre_constructor() { <<< "good_pre_constructor, this =", this >>>; } } Foo good_foo; <<< good_foo =", good_foo>>> ============== % chuck foo.ck good_pre_constructor, this = 0x634720 good_foo = 0x634720 ============== Note that the instance of good_foo is a reasonable non-null value. Did you spot the difference? "bad_pre_constructor" is declared (improperly) to return a float, but it doesn't, and an instantiation of Foo silently returns a null. "good_pre_constructor" is properly declared with a void return, and an instantiation of Foo returns a good value. So yes, this was operator error -- it's an error not to return a value when you claim you're going to. But ChucK fails silently, at least until you get hit with a NullPointerException some time later in the execution. This is not friendly behavior. - Rob P.S.: Uh, is this the right forum for reporting bugs like this?
Robert;
Did you spot the difference? "bad_pre_constructor" is declared (improperly) to return a float, but it doesn't, and an instantiation of Foo silently returns a null. "good_pre_constructor" is properly declared with a void return, and an instantiation of Foo returns a good value.
Ah, yes. I think this has been under debate before; functions that claim they will return something must return this and ChucK doesn't verify that it does. I think it was debated in some depth on the forum. Part of the problem is that it's not clear ChucK could know a function will always return when we do know this. Consider this; fun int invert (int input) { if (input == 0) return true; else if (input == 1) return false; } We may know that this function will only get fed a 1 or a 0 and so it may be "safe" but if it would get a 3 then there will be a issue. While this may be ok in certain contexts I'd always re-write it like this; fun int invert (int input) { if (input == 0) return true; else return false; } What we could do is require a function of a non-void type to have a "return" statement somewhere in it, maybe even require that it should be on the last line and outside of any conditions and issue a complaint otherwise, but that will outlaw some types of functions that may be perfectly fine. I'm a bit scared that rules that will prevent improper functions from being written at all will also stifle creativity; in many cases us programmers are a lot more clever than parsers are. That said; I think it's good form to always end functions of a non-void type with a return statement, even if it's just "else return false", often I will make it print a warning if it fails in such a way due to unexpected input. Some people insist on only having a single return statement on the last line. While that's very safe I feel it can lead to overly complicated constructs that I don't find as pleasant to read or write and that proper indentation and syntax highlighting can make it clear other strategies are safe as well; the second example I gave above does that and I consider that one quite natural and readable as well as clearly safe. The only reason I can think of -right now- to not have a return statement on the last line and outside of any conditions is when we would want to kill the whole shred on that line. While strictly speaking this is valid and might be useful that's far from clean and will likely lead to readability issues but is that really a reason to ban such tricks? I mean; maybe our foot is rebelling against us, we may have a valid reason for shooting it ;¬).
This is not friendly behavior.
I agree, but it's not clear what proper behaviour should be either. For now I'm inclined to have compilation halt if the "return" keyword isn't used in a function that is of a non-void type and leave the programmer to make sure it will return in each and every case. That would at least catch simple typos in the definition of the function type and give a fair warning.
P.S.: Uh, is this the right forum for reporting bugs like this?
Absolutely. It's interesting that you found that this issue (which in itself was known) will prevent object instantiation as well. I agree with you that the current behaviour can lead to lengthy searches due to only making a single typo. Yours, Kas.
I agree, but it's not clear what proper behaviour should be either.
FWIW, Java complains, however, C++ silently returns 0.0--or at least
prints a 0.0. I think that it matters less which solution ChucK uses
than the idea that it's clear about it. i.e. ChucK should complain,
or avoid the error, but definitely not return a null object reference.
my $0.02
.mc
On Thu, Mar 12, 2009 at 9:54 AM, Kassen
Robert;
Did you spot the difference? "bad_pre_constructor" is declared (improperly) to return a float, but it doesn't, and an instantiation of Foo silently returns a null. "good_pre_constructor" is properly declared with a void return, and an instantiation of Foo returns a good value.
Ah, yes. I think this has been under debate before; functions that claim they will return something must return this and ChucK doesn't verify that it does. I think it was debated in some depth on the forum. Part of the problem is that it's not clear ChucK could know a function will always return when we do know this. Consider this;
fun int invert (int input) { if (input == 0) return true; else if (input == 1) return false; }
We may know that this function will only get fed a 1 or a 0 and so it may be "safe" but if it would get a 3 then there will be a issue. While this may be ok in certain contexts I'd always re-write it like this;
fun int invert (int input) { if (input == 0) return true; else return false; }
What we could do is require a function of a non-void type to have a "return" statement somewhere in it, maybe even require that it should be on the last line and outside of any conditions and issue a complaint otherwise, but that will outlaw some types of functions that may be perfectly fine. I'm a bit scared that rules that will prevent improper functions from being written at all will also stifle creativity; in many cases us programmers are a lot more clever than parsers are. That said; I think it's good form to always end functions of a non-void type with a return statement, even if it's just "else return false", often I will make it print a warning if it fails in such a way due to unexpected input.
Some people insist on only having a single return statement on the last line. While that's very safe I feel it can lead to overly complicated constructs that I don't find as pleasant to read or write and that proper indentation and syntax highlighting can make it clear other strategies are safe as well; the second example I gave above does that and I consider that one quite natural and readable as well as clearly safe.
The only reason I can think of -right now- to not have a return statement on the last line and outside of any conditions is when we would want to kill the whole shred on that line. While strictly speaking this is valid and might be useful that's far from clean and will likely lead to readability issues but is that really a reason to ban such tricks? I mean; maybe our foot is rebelling against us, we may have a valid reason for shooting it ;¬).
This is not friendly behavior.
I agree, but it's not clear what proper behaviour should be either. For now I'm inclined to have compilation halt if the "return" keyword isn't used in a function that is of a non-void type and leave the programmer to make sure it will return in each and every case. That would at least catch simple typos in the definition of the function type and give a fair warning.
P.S.: Uh, is this the right forum for reporting bugs like this?
Absolutely. It's interesting that you found that this issue (which in itself was known) will prevent object instantiation as well. I agree with you that the current behaviour can lead to lengthy searches due to only making a single typo.
Yours, Kas.
_______________________________________________ chuck-users mailing list chuck-users@lists.cs.princeton.edu https://lists.cs.princeton.edu/mailman/listinfo/chuck-users
Mike;
FWIW, Java complains, however, C++ silently returns 0.0--or at least prints a 0.0. I think that it matters less which solution ChucK uses than the idea that it's clear about it. i.e. ChucK should complain, or avoid the error, but definitely not return a null object reference.
I agree; I'm leaning towards a runtime notice with the name of the function listed if it goes wrong (in addition to either dropping the shred or returning a new instance or zero value of whatever the type of the function) and a compile time complaint if such a function doesn't contain the word "return" at all. That would allow nearly everything that currently runs to keep running, no matter how dangerous the construction, yet provide notices if we made a error. I'm not in favour of ChucK baby-sitting me when I may be doing something that ChucK thinks is dangerous even when I know it's fine in this context; the parser may make less real errors than I do but I'm still convinced I'm more clever than the parser is. Kas.
On 12 Mar 2009, at 06:54, Kassen wrote:
...Part of the problem is that it's not clear ChucK could know a function will always return [a value] [discussion on the difficulty of data flow analysis, etc...]
I agree that "it is an error" to declare that a function return a value and then not execute a return statement in the course of execution. I even think it's reasonable to be C++ like and return a null (for a reference) or a 0 (for a number). But that misses the point: it is NOT reasonable behavior for a null return value for a pre-constructor to cause the constructor itself to return null (unless that's documented behavior?!?). Consider this: ============ public class FanOfKas { pre_constructor(); public int pre_constructor() { return 42; } } FanOfKas we_like_kas; <<< "we_like_kas =", we_like_kas >>>; ============== Would anyone would claim that we_like_kas should be set to the integer 42 rather than an instance of WeLikeKas? - Rob
Rob; But that misses the point: it is NOT reasonable behavior for a null return
value for a pre-constructor to cause the constructor itself to return null (unless that's documented behavior?!?). Consider this:
I completely agree. I don't think anybody would argue the current behaviour is right; it's quite wrong and as you demonstrated it can have follow up effects that lead to hard to trace issues. the question I was attempting to address is "what should happen instead?". In the past some people have suggested that this situation (non-returning functions) should be made impossible (at the parser stage) and I don't think we can do that without demanding a very strict style of programmers. I admit it was unclear that I was partially addressing that suggestion instead of the bug you reported and that this was unclear. As far as I can see there is nothing here on which we disagree. I would, BTW, like to suggest that "fans of kassen" should have plenty of member functions that return stuff, cups of coffee and dark beers would be especially good :¬). Cheers, Kas.
participants (3)
-
Kassen -
mike clemow -
Robert Poor