On 9/12/12 4:10 PM, Gilles Sadowski wrote: > Hello. > > [For those who don't wish to read the whole post, please at least go towards > the end and indicate your preferred option. Thanks.] > >>> [...] >>>> All are bad arguments violating API contract, >>>> all detected at method activation time -> IAE. >>> Agreed... if the policy is to _always_ check for "null"! >> I am only referring to APIs that explicitly disallow nulls. > I don't know what you mean by "only": Almost _all_ the API disallows "null". > What has "implicitly" or "explicitly" have to do with that (other than > "implicitly" being equivalent to lacking documentation vs "explicitly" being > defined by a default policy)? > >> I have >> agreed that we will not always do this. > Again, this is one point where we differ as regards to "rules". I contend > that there must be an "ideal", defined by the policy. Is the ideal to always > check or to never check. I think the latter, for the reasons explained. > > The policy cannot be "sometimes yes, sometimes no" unless you can define in > which case to do it and in which not, which is clearly more complicated. > >> The specific case being >> discussed here is an API that is going to include as an explicit >> precondition that nulls are not allowed. When this is done, what >> should be thrown is an IllegalArgumentException, which for us means >> some subclass of MathIllegalArgumentException. > If we were to decide that the ideal is to always check (an obviously > different policy), I wouldn't have a problem with this. > What I point at is the potential burden on the _user_ if he wants to > intercept exceptions: for the _same_ problem (null reference), they'd have > to catch two types of exceptions: NPE vs NAE (as sublcass of MIAE). > > If nobody cares about that, I won't either. I just wanted to stress that we > might have some user complaints about this inconsistency which will appear > at the contact point between user code and CM code: Because it is simply not > nice to report the same problem in different ways. > > And because it is inconsistent, some future contributor will raise this > issue again: "Why do we check here and throw MIAE and why don't we check > there and let the JVM throw NPE?". [This is exactly what you reported about > checking the components of a FieldVector but not the vector itself.] > >>> If we sometimes check and sometimes not, the thrown exception will be from a >>> different hierarchy (NPE vs MathRuntimeException). This is annoying (and >>> inconsistent). >> That is an unfortunate consequence of not always checking. I >> understand that it may be impractical to always check. Therefore, >> some NPEs are going to be propagated to clients with no warning. > What do you mean by "impractical"? > In practice, we will not check everything tomorrow, but we can decide to > work toward that goal. > But we have to answer the question not of whether it is practical, but > whether it is desirable. > This is a quite simple issue; and I don't believe in considerations like > it might be good in some part of CM APIs but not in others. [In _principle_ > I mean; I don't say that in _practice_ some parts of CM won't be up-to-date > sooner than other parts.] > >>> The oft-repeated issue is "Is it useful to check for null or not?"; at least >>> 4 people answered "No", because this bug will never go unnoticed: sooner or >>> later, using "null" _will_ raise exception. The sole and unique difference >>> with CM checking and JVM checking is that the stack trace will (sometimes) >>> be a little shorter in the former case. >> And inferior first failure data capture and potential additional >> computation, side effects or other unknown consequences (unless we >> always think through the consequences of nulls propagated through >> our code and make sure there are never any untoward side effects. >> All of these are reasons for the "when you must fail, fail fast and >> loudly" maxim. I understand that it may not be possible to adhere >> to this fully in [math]. > You repeat the mantra "fail fast, etc." but do not give a counter-argument > to mine about the NPE being as fast as it needs to be for this kind of > failure. > I'd like to see one example of "untoward side effects". > >>> Most people have come to agree that >>> it's not worth adopting a policy of thorough checking. [Rationale 1: users >>> who encounter a NPE will need to inspect the stack trace and go to _their_ >>> code in order to see where _they_ put a "null". Rationale 2: there is >>> probably a performance penalty to all these checks (especially in a well >>> tested code where "null" reference bugs have been ironed out).] >> The latter is a good point and why I am OK not checking everywhere. > The argument is equally valid _everywhere_ (unless you can provide the > above example), in both directions: either it is good to check, then it is > always good, or in some cases it is not necessary, then it is never > necessary. > >>>> Consider, e.g. an empty array or array indices that will lead to >>>> index out of bounds errors in APIs that take arrays. What is so >>>> different about null? >>> There is no difference... if there is a policy that decrees so. >>> >>> It is _not_ the policy of the JDK: "NullPointerException" is not the same >>> error as "IndexOutOfBoundsException"; their common parent class is the >>> (unspecific) "RuntimeException" and their sibling is >>> "IllegalArgumentException". >>> There is no "is-a" relationship between NPE, IOBE and IAE. >> Here you are missing the point above - we are talking about specific >> APIs that explicitly disallow nulls. > I do not think I miss any point. Rather it is you who fail to point to > to cases where such an "explicit" mention is needed. > As Luc pointed out, only stating where null _is_ allowed is actually > useful. And as I pointed out about your example of using null in > constructors, those occurrences should be very rare, for the very reason > that such code is much more obscure (and documentation should thus be > extremely precise to avoid confusion). > >> Just as we trap or check >> arguments to avoid array index errors, we can in some cases do the >> same for nulls. In these cases, it is appropriate to throw IAE for >> the nulls just as we do for the (avoided) AIOBE. >>> Recalling that some 1.5 years ago you were against the adoption of the >>> single exception hierarchy, rooted at "MathRuntimeException", in order to >>> stay faithful to the JDK exception types, I wonder why you are on the >>> opposite stand as NPE is concerned. >> I am not. I have agreed that we can go back to a common root. But >> that then forces us to create surrogates for the top level >> exceptions such as IAE that the "math" versions used to subclass. I >> am OK with that. But we should maintain the semantics correctly, >> throwing a MIAE when API contracts are violated. > You don't answer to my argument. I did not say that you were against a > common root _now_, but that you preferred to retain the standard exceptions > _before_. For the one exception where I think that it is better to indeed > keep the standard NPE exception, you hold the opposite view (common root). > > In summary, you agree that it might not be good (or "practical", whatever > that means) to always check for null. Then, where do you draw the line in > order to take "Rationale 2" into account? IOW, you say that "Rationale 2" > is a good point but, imagine that someone spends a few days adding all the > null checks, then this good point will not exist anymore. That's a > contradiction. > > Again, having NAE inherit from NPE removes all contradictions in one fell > swoop. I can't fathom why you insist on the naming issue, why this > precondition should be reported by "MathIllegalArgumentException". I repeat > that in the JDK "ArrayIndexOutOfBoundsException" is _not_ a subclass of > "IllegalArgumentException". That, in CM, "OutOfRangeException" _is_ a > subclass of "MathIllegalArgumentException" is a different choice that _we_ > made. We can make a different choice for "NullArgumentException" if we see > that there is more benefit: it allows a flexible, yet consistent policy, of > checking or not checking while throwing the same exception for the same > problem. > > Back to square one, with 3 fully consistent alternatives: > 1. CM to always check for null? Then "NullArgumentException" inheriting from > "MathIllegalArgumentException" is fine because we promise to the user that > no NPE will ever propagate through the CM layer. [Breaking that promise > is a CM bug.] > 2. CM to never check for null? Then we delete class "NullArgumentException". > Users are warned by the general policy: "Do not pass null unless it is > explicitly documented to be allowed." A bug will lead to the JVM raising > a NPE. > 3. CM to sometimes check for null? Then "NullArgumentException" should > inherit from "NullPointerException" because the user will sometimes see > "NullArgumentException" (when CM checks) and sometimes NPE (when CM does > not check) and both should thus belong to the same hierarchy (from the > user's point-of-view, there is no reason to handle them in different > ways since it's the exact same bug). > For the user, the consequence will be similar to alternative 2, with > sometimes more information about the failure and sometimes (marginally) > earlier detection.
As stated above, my preference is 4. CM APIs *may* disallow nulls explicitly. When that is done, the implementations *should* check parameters (as they *should* check all other stated preconditions) and when preconditions are violated, a MathIllegalArgumentException is thrown. I don't care whether or not we keep NAE. If we drop it, we should make sure whatever exception messages we used to use to indicate illegal null arguments are still there. Phil > > Your alternative (sometimes check, sometimes not) is not fully consistent: > * for the user: "In case of null reference, will I get a MathRuntimeException > or a NPE?" > * for the CM developer: "In which cases do I need to check for null?" > > Of course, I would reconsider if you could provide an actual example that > would fail with all three alternatives which I suggested. If not, then > it seems obvious that your alternative presents two defects that don't exist > in any of those three. > > > Gilles > >>>> [...] what is different about null arguments at the point of >>>> method activation in an API that explicitly disallows them. >>> The difference is that there is no need to tell the user what the problem >>> is because the raised exception says it all: "You tried to use a null >>> reference." [As said above, the only issue is _when_ the exception is >>> triggered.] >>> >>> The policy mandates what is globally valid, e.g.: >>> "If not specified otherwise, "null" is not allowed as an argument." >>> Then, a user cannot complain about a NPE being propagated when he passed an >>> invalid (null) argument. >>> >>> Finally, the case for "null" is also slightly peculiar (given the above) >>> that it should not simply be bundled with the rationale "Fail early": Indeed >>> "null" references always fail early (i.e. as soon as they are used). >>> Deferring the check until it is done by the JVM will never entails the code >>> to produce a wrong result _because_ of that null reference (it will just >>> fail in the predictable way: NPE).[1] >>> >>> >>> Gilles >>> >>> [1] Unlike in C, where an unitialized pointer would not necessarily crash >>> the program, but _could_ make it behave erratically (including produce >>> wrong results in a stealth way). >>> > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
