Add null check for Cleanup annotation

[lombokissues]

Migrated from Google Code (issue 154)

[lombokissues]

👤 steve.klouvi   🕗 Oct 21, 2010 at 15:37 UTC

It might be a good feature if code generated for Cleanup annotation does not include a null check. Initialiszation expression may throw an exception , and variable would be null.

public class MissingNullCheck {
@ Test
public void cleanNullReference() throws IOException {
@ Cleanup InputStream in = null; // throws NPE

    // @ Cleanup InputStream in = new FileInputStream(...);
    // FileInputStream constructor may throw an exception, reference will be null.
}

}

BTW, it's a very impressive library ;)

[lombokissues]

👤 r.spilker   🕗 Oct 21, 2010 at 15:46 UTC

Thanks Steve,

I'm not quite sure what your suggestion is and what problem it would solve.

Are you saying that currently the statement

@ Cleanup InputStream in = null;
causes a NullPointerException?

If you can give us an example of the problem and what the delomboked code should look like to solve the problem, that would be a great help.

Roel

[lombokissues]

👤 steve.klouvi   🕗 Oct 21, 2010 at 15:58 UTC

Probably my mistake. I think i shouldn't do this in the first place.

public class MissingNullCheck {
@ Test
public void cleanNullReference() throws IOException {
@ Cleanup InputStream in = null;

    // throws an NPE, original IOException is lost in the process
    in = openStream();
}

private InputStream openStream() throws IOException {
    throw new IOException();
}

}

[lombokissues]

👤 koen.verrecken   🕗 Oct 29, 2010 at 12:31 UTC

Reading a file from the classpath is one such example. When the resource doesn't exist the classloader will return null and the @ Cleanup annotation will throw a NPE:

@ Cleanup
InputStream inputStream = SomeClass.class.getClassLoader().getResourceAsStream("unexisting.txt");

[lombokissues]

👤 reinierz   🕗 Oct 29, 2010 at 13:47 UTC

So, what you are asking for is that instead of generating:

try {
....
} finally {
varName.close();
}

We should generate:

try {
....
} finally {
if (varName != null) varName.close();
}

right? If so, well, I can't think of any reasons not to do so off the top of my head. Roel?

[lombokissues]

👤 r.spilker   🕗 Nov 01, 2010 at 11:54 UTC

Great suggestion.

[lombokissues]

👤 r.spilker   🕗 Nov 06, 2010 at 23:19 UTC

Fixed in 17bcfda. Will be in 0.9.4 "Burning Emu"

[lombokissues]

👤 r.spilker   🕗 Nov 06, 2010 at 23:21 UTC

[lombokissues]

End of migration