Bug 414237 - [compiler][null] Support a @LazyNonNull annotation for fields
Summary: [compiler][null] Support a @LazyNonNull annotation for fields
Status: REOPENED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 4.3   Edit
Hardware: All All
: P3 enhancement with 10 votes (vote)
Target Milestone: ---   Edit
Assignee: JDT-Core-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: helpwanted
: 247564 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-08-01 14:24 EDT by Stephan Herrmann CLA
Modified: 2020-06-09 10:42 EDT (History)
13 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Stephan Herrmann CLA 2013-08-01 14:24:09 EDT
This bug is proposing a concept initially presented by the name @EventuallyNonNull in http://wiki.eclipse.org/JDT_Core/Null_Analysis/Options and then discussed as @LazyNonNull in bug 383371 and others.

The idea is to have an annotation for fields that expresses:
- a field may remain uninitialized (null) past the end of any constructor
- any assignment to the field must provide a non-null value
Some researchers call this monotonous, because such a field can only change from null to non-null but never back to null.

This design has the benefit that fields with this annotation could safely participate in flow analysis, because none of the common dangers - aliased references, side effects nor concurrence - can spoil a non-null value once it has been observed.

The annotation is highly relevant because lazy initialization of fields is a wide spread coding style.

Implementing analysis using @LazyNonNull has some impact on the complexity in the compiler, so introducing this feature should not happen just for theoretical reasons, but users must give indication that it will help them to write better code easier.

So, please chime in if you'd like to see this feature in a future version of JDT.
Comment 1 Ed Willink CLA 2013-08-01 14:44:40 EDT
Absolutely. I currently use /*@LazyNonNull*/ and get very annoyed when I end up in one of the areas where non-irritation means I get no checks at all.
Comment 2 Timo Kinnunen CLA 2013-08-02 00:06:03 EDT
I've used /*@LazyNonNull*/ in a couple of classes and while it's useful for dealing with objects that have a complex internal state space, explicit @Nullable annotations see a lot more use, 26 uses to 7. Not to mention all the implicit @NotNull annotations that are everywhere. 

@LazyNonNull could see more usage except I've found that splitting such complex classes into a Builder-Immutable Value -pair of classes is better. The fields that would have been annotated with @LazyNonNull become @NotNull on the Immutable Value side and could get converted from fields to local variables on the Builder side.

Perhaps @LazyNonNull could be most useful as a default option to consider before going with default @Nullable annotations?
Comment 3 Stephan Herrmann CLA 2013-09-05 08:31:48 EDT
*** Bug 247564 has been marked as a duplicate of this bug. ***
Comment 4 Patrice Chalin CLA 2013-11-28 12:34:46 EST
This is a useful annotation to add support for, and the name @LazyNonNull does seem better than @EventuallyNonNull or @MonotonicNonNull as we had originally proposed in JML. Looking forward to seeing support for this annotation.
Comment 5 Konrad Windszus CLA 2015-03-13 04:28:50 EDT
Is it planned that final fields with an assignment on the same line or in the constructor to some non-null value (either literal or value where non-nullness can be inferred) have the semantics of @LazyNonNull annotated fields automatically?
Especially static final fields having a non-null value assigned should have these semantics!
Comment 6 Stephan Herrmann CLA 2015-03-13 07:09:21 EDT
(In reply to Konrad Windszus from comment #5)
> Is it planned that final fields with an assignment on the same line or in
> the constructor to some non-null value (either literal or value where
> non-nullness can be inferred) have the semantics of @LazyNonNull annotated
> fields automatically?
> Especially static final fields having a non-null value assigned should have
> these semantics!

For final fields I don't see the need to apply @LazyNonNull; final fields cannot be lazy. Just mark them as @NonNull and we'll check if everything is safe. And if you have lots of @NonNull fields, use @NonNullByDefault and only mark the exceptions as @Nullable. Am I missing anything?
Comment 7 Radek Czyz CLA 2015-10-22 22:51:45 EDT
(In reply to Stephan Herrmann from comment #6)
> Am I missing anything?

I think Konrad is looking for a workaround for the problem of *Nullable* final fields, which are currently considered just as mutable as regular Nullable fields, so null checks don't prevent a potential null access warnings.

If this bug was implemented then LazyNonNull would be a way to workaround that problem - although a separate "proper" solution would be even better.

By the way, I would identify the problem of nullable final fields as my #1 biggest problem with current null analysis.
Comment 8 Holger Klene CLA 2015-10-23 05:33:32 EDT
CDI and also mapping fields with JPA produces lots of members, not guaranteed to be initialized via a constructor. Currently I cancel the default @NonNullByDefault(false) just to not be forced to make them all @Nullable or assign silly default values.

So actually I'm thinking of treating a whole List of annotations equivalent to @LazyNonNull, if in a context of @NonNullByDefault and not explicitly @Nullable. Namely:
@Inject
@Id
@Column
@JoinColumn
@OneToOne
@OneToMany
@ManyToOne
@ManyToMany
...
Comment 9 Ed Willink CLA 2015-10-23 06:12:25 EDT
I'm still looking forward to @LazyNonNull, but after commenting it in code for a couple of years, I see one limitation.

I often require to set fields to null at the end of life to facilitate garbage collection or other cleanups. It would therefore be nice to be able to rewind an @LazyNonNull back to its initial state. This obviously subverts all the alias analyses. Suggest that an assignment of null is permitted in a 'simple' synchronized block.

@LazyNonNull MyResource mine;

....

mine = new MyResource();

....

synchronized(this) { mine = null; }
Comment 10 Stephan Herrmann CLA 2018-09-08 07:41:31 EDT
@LazyNonNull keeps popping up at a low frequency, so I'm still not sure whether or not such addition is worth the effort, but haven't given up on the idea, yet.

(In reply to Ed Willink from comment #9)
> I'm still looking forward to @LazyNonNull, but after commenting it in code
> for a couple of years, I see one limitation.
> 
> I often require to set fields to null at the end of life to facilitate
> garbage collection or other cleanups.

At first I thought: "of course" but on closer look let me ask: at EOL of the containing object, how would nulling its fields facilitate GC? Isn't it actually better to keep the reference until the field itself (via its containing object) is unreachable? 

Otherwise I was thinking you want a @Destructor annotation, but descructors won't easily fit into the model of Java.

> It would therefore be nice to be able
> to rewind an @LazyNonNull back to its initial state. This obviously subverts
> all the alias analyses. Suggest that an assignment of null is permitted in a
> 'simple' synchronized block.
> 
> @LazyNonNull MyResource mine;
> 
> ....
> 
> mine = new MyResource();
> 
> ....
> 
> synchronized(this) { mine = null; }

This would only help if all access to 'mine' is synchronized on this, which I hold to be unrealistic in the general case.
Comment 11 Sergey Olefir CLA 2018-09-08 08:01:58 EDT
Right now I'm using this construct for lazily initialized @Nonnull fields:

@Nonnull
private Service service = fakeNonNull();

This is pretty self-explanatory and doesn't require any additional compiler support.

Is it equivalent? Or is the idea behind @LazyNonNull more advanced? Such as compiler would somehow try to analyze when the field was/was not initialized? Is it even possible?
Comment 12 Ed Willink CLA 2018-09-08 08:44:02 EDT
(In reply to Sergey Olefir from comment #11)
> private Service service = fakeNonNull();

The whole idea of @LazyNonNull is that it really is null until it isn't. fakeNonNull documents the not-a-useful-non-null-object but inhibits any attempt at compiler diagnosis.(In reply to Stephan Herrmann from comment #10)
> @LazyNonNull keeps popping up at a low frequency, so I'm still not sure
> whether or not such addition is worth the effort, but haven't given up on
> the idea, yet.
 
> (In reply to Ed Willink from comment #9)
> > I often require to set fields to null at the end of life to facilitate
> > garbage collection or other cleanups.
> 
> At first I thought: "of course" but on closer look let me ask: at EOL of the
> containing object, how would nulling its fields facilitate GC? Isn't it
> actually better to keep the reference until the field itself (via its
> containing object) is unreachable? 

In a simple world, each object would be owned by a single container, and 'containments' would form a tree so that freeing a node frees up all its children. In the real world we have a cyclic graph of multiple references so that just a single stale 'bad' reference can lock everything into memory. (EMF is particularly cyclic with locking singletons in registries.)

Explicitly nulling a field, particularly a collection field breaks many cycles much improving the chances of GC not being inhibited and certainly easing GC failure diagnosis.

@Destructor: if you want/need a bye-bye @LazyNonNull indication, that is fine by me.
Comment 13 Stephan Herrmann CLA 2018-09-08 08:48:20 EDT
(In reply to Ed Willink from comment #12)

got it, thanks.
Comment 14 Sergey Olefir CLA 2018-09-08 08:58:45 EDT
(In reply to Ed Willink from comment #12)
> (In reply to Sergey Olefir from comment #11)
> > private Service service = fakeNonNull();
> 
> The whole idea of @LazyNonNull is that it really is null until it isn't.
> fakeNonNull documents the not-a-useful-non-null-object but inhibits any
> attempt at compiler diagnosis.

I guess I'm not following this. In my case, from compiler POV, object is non-null. Which is like 95% fine for lazy initialization.

How would it even be possible at compile time to determine when lazy initialization occurs (I'm thinking like Spring wiring here) so that compiler can distinguish between null and non-null states?
Comment 15 Stephan Herrmann CLA 2018-09-08 09:03:59 EDT
(In reply to Sergey Olefir from comment #14)
> (In reply to Ed Willink from comment #12)
> > (In reply to Sergey Olefir from comment #11)
> > > private Service service = fakeNonNull();
> > 
> > The whole idea of @LazyNonNull is that it really is null until it isn't.
> > fakeNonNull documents the not-a-useful-non-null-object but inhibits any
> > attempt at compiler diagnosis.
> 
> I guess I'm not following this. In my case, from compiler POV, object is
> non-null. Which is like 95% fine for lazy initialization.
> 
> How would it even be possible at compile time to determine when lazy
> initialization occurs (I'm thinking like Spring wiring here) so that
> compiler can distinguish between null and non-null states?

Did you read the initial description?

(In reply to Stephan Herrmann from comment #0)
> [...]
> The idea is to have an annotation for fields that expresses:
> - a field may remain uninitialized (null) past the end of any constructor
> - any assignment to the field must provide a non-null value
> [...]
> This design has the benefit that fields with this annotation could safely
> participate in flow analysis, because none of the common dangers - aliased 
> references, side effects nor concurrence - can spoil a non-null value once it
> has been observed.

Is anything unclear about this?
Comment 16 Sergey Olefir CLA 2018-09-08 10:00:09 EDT
(In reply to Stephan Herrmann from comment #15)
> Is anything unclear about this?

Yes, I couldn't understand from the description what exactly is being proposed at all.

I've now looked through the referenced link ( http://wiki.eclipse.org/JDT_Core/Null_Analysis/Options ) and based on what I see there, it looks like any access to @LazyNonNull is supposed to be guarded by 'if (xxx != null)' with the significant difference being that the code inside 'if' block can safely assume that the value will never become null due to concurrency (so you don't need to extract value to local variable in order to safely assume non-nullability).

Is that the case?

If so, then based on my experience (I actively use Eclipse null-analysis in all my projects and everything is non-null-by-default) I would still use the 'fakeNonNull()' approach in majority of the cases over the @LazyNonNull -- simply because in my experience this kind of code mostly crops up in IOC initialization where I can safely assume that the field *will* be non-null, it is just not set in the constructor. And I wouldn't want to write null-checks whenever I need to use that field.

So, if my understanding of this proposal is correct, and since the original descriptions solicits users feedback, I'd say that for me personally this feature doesn't seem particularly useful.
Comment 17 Stephan Herrmann CLA 2018-09-08 11:22:34 EDT
(In reply to Sergey Olefir from comment #16)
> (In reply to Stephan Herrmann from comment #15)
> > Is anything unclear about this?
> 
> Yes, I couldn't understand from the description what exactly is being
> proposed at all.
> 
> I've now looked through the referenced link (
> http://wiki.eclipse.org/JDT_Core/Null_Analysis/Options ) and based on what I
> see there, it looks like any access to @LazyNonNull is supposed to be
> guarded by 'if (xxx != null)' with the significant difference being that the
> code inside 'if' block can safely assume that the value will never become
> null due to concurrency (so you don't need to extract value to local
> variable in order to safely assume non-nullability).
> 
> Is that the case?

correct.


> If so, then based on my experience (I actively use Eclipse null-analysis in
> all my projects and everything is non-null-by-default) I would still use the
> 'fakeNonNull()' approach in majority of the cases over the @LazyNonNull --
> simply because in my experience this kind of code mostly crops up in IOC
> initialization where I can safely assume that the field *will* be non-null,
> it is just not set in the constructor. And I wouldn't want to write
> null-checks whenever I need to use that field.

Have you seen bug 400421?


> So, if my understanding of this proposal is correct, and since the original
> descriptions solicits users feedback, I'd say that for me personally this
> feature doesn't seem particularly useful.

Thanks for sharing.
Comment 18 Sergey Olefir CLA 2018-09-12 07:04:09 EDT
(In reply to Stephan Herrmann from comment #17)
> Have you seen bug 400421?

Thanks for pointing this out. For posterity -- there's support for @Inject annotation specifically intended for IOC -- a non-null-annotated field can be also annotated with @Inject so that there's no requirement to initialize it in the constructor.

However, as far as I can see, @Inject support is not documented in Eclipse docs nor is it configurable/selectable via 'Annotations for Null Specifications'.

Perhaps issues should be created for docs / configuration support?
Comment 19 Mauro Molinari CLA 2020-02-20 10:22:07 EST
I recently started to use null analysis and I really miss this feature, because I really find myself in situations in which the best option seems to be... to disable null analysis :-(

Workarounds: 
- mark fields as @Nullable and fill in the code with Checks.requireNonNull(...) where you need to access those fields, being sure the check will pass... you quickly fill your code with useless statements and you are forced to add JDT null annotations to the runtime classpath (while theoretically it could be a compile-only dependency)
- leave fields as @NonNull and initialise them to some fake non-null values: simple with primitive and simple types, can be very hard with interfaces

Use cases:
- lazy initialised fields
- injected fields with annotations not whitelisted by JDT (Spring @Value is one example, see also bug #560315)
- in Servlets, fields initialised by javax.servlet.Servlet.init(ServletConfig)
- other similar @PostConstructor-like initialisation

I really really miss this feature. Because of its lack, I think I need to give up with Eclipse null analysis, or at least limit it a lot...

I see someone is referring to /*@LazyNonNull*/, what is it? I tried to add it to my field documentation, but it doesn't seem to work.
Comment 20 Stephan Herrmann CLA 2020-02-20 10:44:03 EST
Noted.

I'm sure we need something in this direction and I didn't read any alternative proposals.

=> Putting it on the radar for the next cycle, no promise yet ...
Comment 21 Sergey Olefir CLA 2020-02-20 10:58:33 EST
(In reply to Mauro Molinari from comment #19)
> simple with primitive and simple types, can be very hard with interfaces

Is it? I'm using this, I don't think I've ever had issues.

	@SuppressWarnings("null")
	public static @Nonnull <T> T fakeNonNull()
	{
		return null;
	}

Example:

import static package.NullUtil.fakeNonNull;

public ArrayList<@Nonnull InternedArrayList<@Nonnull String>> values = fakeNonNull();
Comment 22 Mauro Molinari CLA 2020-02-20 11:07:23 EST
(In reply to Sergey Olefir from comment #21)
> Is it? I'm using this, I don't think I've ever had issues.

OK, but I meant "hard WITHOUT writing some dedicated code".

Writing:
private String foo = '';
or even
private Integer bar = 0;

knowing that those initial values are useless can still be somewhat acceptable. 

Also something like:
private Person person = new Person('');

could be bearable. But having to write a utility method and filling up my code with these fakeNonNull() calls just to satisfy the compiler with regards to fields of more complex types/interfaces is awkward honestly.

Anyway, thanks for sharing!
Comment 23 Stephan Herrmann CLA 2020-02-20 11:19:53 EST
The single implementation of fakeNonNull() shouldn't weigh much.

Pragmatically,

@NonNull Foo foo = fakeNonNull();

is quite close to

@LazyNonNull Foo foo;


But beware that the former idiom promises non-null on every access, which can be wrong. The latter only promises: once tested as non-null it will never go back to null.
Comment 24 Ed Willink CLA 2020-02-20 12:20:39 EST
@NonNull Foo foo = fakeNonNull();  ... Mm sneaky.

And it supports the reset/rewind/teardown use case too.

Oh no it doesn't.

Since this declares a field that is definitely non-null, you will never get any checks on whether you accidentally use it in a pre-non-null state. You might as well just leave off the @NonNull annotation.
Comment 25 Stephan Herrmann CLA 2020-02-20 12:30:53 EST
(In reply to Ed Willink from comment #24)
> @NonNull Foo foo = fakeNonNull();  ... Mm sneaky.
> 
> And it supports the reset/rewind/teardown use case too.
> 
> Oh no it doesn't.
> 
> Since this declares a field that is definitely non-null, you will never get
> any checks on whether you accidentally use it in a pre-non-null state. You
> might as well just leave off the @NonNull annotation.

That's what I meant when saying "can be wrong":

(In reply to Stephan Herrmann from comment #23)
> But beware that the former idiom promises non-null on every access, which
> can be wrong. The latter only promises: once tested as non-null it will
> never go back to null.
Comment 26 Sergey Olefir CLA 2020-02-20 12:39:02 EST
(In reply to Ed Willink from comment #24)
> Since this declares a field that is definitely non-null, you will never get
> any checks on whether you accidentally use it in a pre-non-null state. You
> might as well just leave off the @NonNull annotation.

Not at all.

In my typical usage -- it *is* an always nonnull field, that is merely initialized in such a way that compiler knows nothing about (such as IoC or de-serialization or something).

There's no need to ever check whether or not this field this null (it isn't barring some catastrophic failure somewhere) -- so @LazyNonNull doesn't address this requirement.

There's just the need to make compiler happy about not-initializing the field (and preferably without creating unnecessary junk objects) -- which is addressed by having the fakeNonNull() method.

I think bug 400421 might be doing exactly that via @Inject annotation, but I don't remember if I've ever tested that (I don't use Google Guice) since it doesn't appear to be configurable in any way.
Comment 27 Mauro Molinari CLA 2020-06-05 18:42:20 EDT
Hi Stephan, I see you're suddenly closing many bugs regarding null analysis with WONTFIX and removing yourself from QA, what happened?
Comment 28 Stephan Herrmann CLA 2020-06-05 18:47:47 EDT
(In reply to Mauro Molinari from comment #27)
> Hi Stephan, I see you're suddenly closing many bugs regarding null analysis
> with WONTFIX and removing yourself from QA, what happened?

I'm leaving JDT, sorry. That's why I removed myself from bugs.

As I'm not aware of any committer who's likely to continue this work in the foreseeable future, I closed those null related bugs as WONTFIX.
Comment 29 Ed Willink CLA 2020-06-06 00:51:34 EDT
That is very bad to hear. But I see no justification for WONTFIX until there is a clear indication from JDT that the @NonNull analysis is no longer supported i.e. deprecated.

As it stands, I find the analysis useful but also badly flawed through its lack of PDE integration across projects. I have often tried to consider how much I would lose from stripping the annotations against how much I would gain.

Without your support I see the annotations getting flakier and flakier to the point of being downright negative. Sigh!
Comment 30 Ed Willink CLA 2020-06-06 02:58:23 EDT
Bug 564014 discusses derisking the ongoing use of @NonNull / @Nullable annotations for the OCL (and QVTd) projects.
Comment 31 Mauro Molinari CLA 2020-06-06 08:54:37 EDT
(In reply to Stephan Herrmann from comment #28)
> I'm leaving JDT, sorry. That's why I removed myself from bugs.
> 
> As I'm not aware of any committer who's likely to continue this work in the
> foreseeable future, I closed those null related bugs as WONTFIX.

Hi Stephan,
this is really sad news... :-(

In any case, as Ed also said, I take the liberty to reopen this all the same, as a legitimate enhancement request, letting the JDT team decide whether null analysis is to be considered unsupported/deprecated or rather to reject this specific enhancement request.
Comment 32 Mauro Molinari CLA 2020-06-06 08:57:51 EDT
Oh sorry Ed, I read the contents of bug #564014 just now, if you prefer me to close again this as WONTFIX there's no problem, I don't want to offend anyone.
Comment 33 Ed Willink CLA 2020-06-06 11:19:29 EDT
Nothing to apologize for. I rather agree. The WONTFIX should be accompanied by a policy from the residual committers as to what the ongoing @NonNull etc support is.
Comment 34 Dani Megert CLA 2020-06-09 10:42:55 EDT
(In reply to Ed Willink from comment #33)
> Nothing to apologize for. I rather agree. The WONTFIX should be accompanied
> by a policy from the residual committers as to what the ongoing @NonNull etc
> support is.
I can't speak for all JDT committers/contributors but at least my team has no capacity to work on this. Any help is welcome.