Add null check for Cleanup annotation #227

Closed
lombokissues opened this Issue Jul 14, 2015 · 9 comments

Projects

None yet

2 participants

@lombokissues
Collaborator

Migrated from Google Code (issue 154)

@lombokissues
Collaborator

๐Ÿ‘ค 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
Collaborator

๐Ÿ‘ค 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
Collaborator

๐Ÿ‘ค 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
Collaborator

๐Ÿ‘ค 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
Collaborator

๐Ÿ‘ค 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
Collaborator

๐Ÿ‘ค r.spilker ย  ๐Ÿ•— Nov 01, 2010 at 11:54 UTC

Great suggestion.

@lombokissues
Collaborator

๐Ÿ‘ค r.spilker ย  ๐Ÿ•— Nov 06, 2010 at 23:19 UTC

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

@lombokissues
Collaborator

๐Ÿ‘ค r.spilker ย  ๐Ÿ•— Nov 06, 2010 at 23:21 UTC

@lombokissues lombokissues added this to the 0.9.4 milestone Jul 14, 2015
@lombokissues
Collaborator

End of migration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment