KT-19762 Add inspection for unclosed (Auto)Closeable with quickfix to wrap usage in use

[DeFuex]

https://youtrack.jetbrains.com/issue/KT-19762

Old PR -> #1970

[goodwinnk]

Thank you for this amazing contribution. I see that a lot of work has been done but unfortunately a lot of corner cases are left. This is a hard inspection to implement and probably it should not have been marked with Up For Grabs tag.

I have commented the original request at https://youtrack.jetbrains.com/issue/KT-19762 and think that we need to discuss the corner cases there before putting additional efforts to the implementation.

If you're going to continue working on this inspection I suggest to wait for discussion end and divide the task to sub tasks. For example you could start from inspection only for some particular scenario. After that's is done and approved we could gradually add quick fixes. Also it probably should be two different inspections for the code that has an explicit close() and for code that doesn't.

[goodwinnk]

This line is outdated. Please run Generate All Tests shared configuration, and re-run tests. LocalInspectionTestGenerated.java file is expected to be modified after this launch, and this change should also be commited.

[goodwinnk]

Some additional tests. Please note, that there's no need to fix them all, some fixes may be postponed if the fix is hard. I think the one with exception is most important. A test with try/finally also should be added to the test suite. I don't like the one with false positive too.

import java.io.BufferedReader
import java.io.File

fun test1(args: Array<String>, reader2: BufferedReader) {
    val reader = File("hello-world.txt").bufferedReader()
    if (args.size > 2) {
        try {
            reader.readLine()
        } finally {
            reader.close()
        }

        return
    }

    reader.readLine() // Exception on fix apply
    reader.close()
}

fun test2(reader: BufferedReader) {
    reader.readLine() // No warnings
    reader.close()
}

fun test3(reader: BufferedReader) {
    try {
        reader.readLine()
    } finally {
        // Comment that is deleted after fix
        reader.close()
    }
}

fun test4(reader: BufferedReader) {
    // Works well, but should be added as an additional test.
    try {
        reader.readLine()
    } finally {
        reader.close()
    }
}

fun test5(reader: BufferedReader): BufferedReader {
    val reader = File("hello-world.txt").bufferedReader()
    reader.toString() // False positive
    return reader
}

[goodwinnk]

I'm not sure this a valid fix in general case. This is not a refactoring and what if close() throws an exception on the second execution?

[goodwinnk]

Why is visitor needed? Can only parent be checked?

[goodwinnk]

I think this code should be rewritten. It's really difficult to understand what's going on.

[goodwinnk]

Please add Fix suffix and make those classes private. I think selecting better names for classes could make preceding comments redundant.

[goodwinnk]

Please don't use !! that much. From our practice, many such calls were fixed after getting an unhanded exception. I think pointer.element is a good candidate for extracting to local variable.

[DeFuex]

Thank you for this amazing contribution. I see that a lot of work has been done but unfortunately a lot of corner cases are left. This is a hard inspection to implement and probably it should not have been marked with Up For Grabs tag.

I have commented the original request at https://youtrack.jetbrains.com/issue/KT-19762 and think that we need to discuss the corner cases there before putting additional efforts to the implementation.

If you're going to continue working on this inspection I suggest to wait for discussion end and divide the task to sub tasks. For example you could start from inspection only for some particular scenario. After that's is done and approved we could gradually add quick fixes. Also it probably should be two different inspections for the code that has an explicit close() and for code that doesn't.

I understand that it needs to be more defined and wasn't expecting it to be in the previous Kotlin release 😄 ...i think it just would be a waste of effort if i would stop now. Just let me know when you guys think that things are ready. In the meanwhile i'll try to improve the code and add more test cases when i got time.

In any case, currently it should be testable from a perspective that it inspects every function call from an instance, assigned to variables (also non-assigned) and when it is derived from a Closeable. And of course the convertion works too...a lot of rough testing went into this... but i see what you mean with instances that shouldn't be leaked and close(), referring to your youtrack comment.

[goodwinnk]

We now have more strict set of rules for adding new inspections and intentions and trying to avoid adding new checks when they have a big chance of false positives.

Nobody is driving this inspection right now, so I'm going to close this pull-request. There is a link left in the ticket to this PR so your work could be reused when working on the (Auto)Closeable inspection is continued.

Thank you for work and time and I'm sorry that we didn't recognize potential problems with this inspection beforehand.