-
-
Notifications
You must be signed in to change notification settings - Fork 639
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
Checking for null _creates_ a null reference problem #2711
Comments
Hi, I would stick to eager evaluation here because generally it is a good idea to fail early. Lazy evaluation might lead to unexpected errors. Your specific problem could be solved by just using () -> null instead of null. |
Thank you. I believe I understand your position, but I use the ability to pass As you say, I could merely pass a collaborator that intentionally does nothing or I could use a Crash Test Dummy, which would blow up spectacularly if accidentally used. That would provide a similar benefit, although perhaps not be quite as easy to spot when skimming the code. I think this comes down to personal preference. I don't think I care enough about the difference to cling to my preference. As you point out, lazy evaluation might cause more headache for others than it causes for me in this one case. Thank you. |
I thought about it a bit.. I agree that it is artificial. In the early days of Vavr (Javaslang) having explicit null-checks all over the place was questioned. Especially when called internally (where I am sure that function refs are not null) it is an overhead of performing null-checks n times. Others asked for adding (non-)nullability annotations to method parameters in order to be able to check it at compile-time (which would not help you because you want to set a non-nullable param to null). To be honest, I would be okay with it to completely remove all the null-checks in favor of performance. Personally, the concrete message does not matter to me if a NPE occurs at runtime. Removing all the checks would change the semantics a bit (eager vs lazy eval) but hopefully no one relies on such exceptions for the control flow of their program (fingers crossed 🤞). |
I agree strongly. I'm glad you reconsidered. I still think I can easily leave with either choice, but I'm glad to see the change considered. Thank you. |
Note: I marked this issue with Task: Search all files for the string (Don't forget run the generator by rebuilding the project before creating the PR.) |
Simply blindly removing all the |
It would be nice if you could weaken the assumption (ignoring messages, just check if a NPE is thrown). I would start with the generated code and the Abstract* collection tests. |
Indeed. I suppose, if nothing else, I might practise some nice |
Quick update: 242 tests passing, 196 to go. |
I think there is a strong argument in favor of leaving the null checks, at least for arguments that will be dereferenced in the body of the function. Unfortunately, Java's type-system doesn't give us tools to express the nullability of the arguments, so these null-checks, are mentally part of the argument's type information that the IDE too can access, and prevent us from passing null somewhere where we shouldn't. Also, if for some reason a @jbrains, in your example the |
I ran out of steam on this one. I don't know whether I'll get back to it. Whatever happens, happens. |
I'd love to take this issue up and continue the work. Is it possible for me to continue building on your state, @jbrains ? |
There might be some null checks, that should be kept in place to avoid confusion. For example: Try::run would return Failure with NPE inside which might confuse a user, expecting the NPE originating from within his runnable. What should the correct behavior be on |
Is there any evidence that those explicit null checks contribute to any form of performance issues? JIT elides null checks when control flow analysis determines that certain references can't be null |
Disclaimer: I'm not the one who created the issue originally. Personally, I wouldn't make the performance argument for removing the null checks. I would argue that explicit nulls simply should be deprecated and no longer be used, period. And this makes checking for nulls unnecessary as well.
But overall, as a user, I don't care if the null checks remain going forward. I just found this to be an interesting PR, that I could finish implementing. |
I'm the originator. I proposed merely yo remove unhelpful defensive programming. Not everyone agrees with my position, so I wanted to open this up for discussion. I was not even slightly concerned with any execution speed issues. Whoever would like to continue this work has my full support. |
My apologies if you've already discussed this to death; just let me know.
I ran into this today.
I was in a test where I knew
myOption
isnone()
, so I didn't bother supplying a function for the "some" case---instead I left itnull
. I reasoned that since onlyfallback
was being called in this path, thefold()
would silently work. Instead, it blew up with a failednull
check.Admittedly, I haven't thought this through for more than a few moments, but I infer that
fold()
is eagerly checking fornull
. I think I would prefer if it didn't. Isfold()
checking fornull
as a defensive habit (for the sake of consistency, I imagine) or to avoid a genuine problem?The text was updated successfully, but these errors were encountered: