Redo tempfile.mktemp codemod

[clavedeluna]

Overview

The original impl of this codemod incorrectly interpreted the docs and assumed mktemp could be directly replaced with mkstemp but that is not the case

Description

• mktemp calls cannot be directly replaced with mkstemp because the former returns a filename while the latter returns a filename and a pointer to a new file with that name. mktemp is insecure because it doesn't handle a potential race condition, but we cannot directly replace it with mkstemp
• We're going to then make more bold assumptions as to the code intent when someone calls mktemp, which is that they don't only want a filename for no reason, but that they actually want to use the file for something. For that, a more appropriate replacement is to call with tempfile.NamedTemporaryFile(delete=False) as tf
• But because we cannot directly know what the code may want the filename for, we're going to keep the file open and let the code author decide when to close it
• I've handled for most reasonable cases while leaving some good unit tests xfailed for more complex cases.

Closes #560

[andrecsilva]

So far so good save for some small issues.

[andrecsilva]

This means the file will be closed as soon as it is created. It needs to account for any references after the assignment and include them in the block.

I remember tackling this problem in the file-resource-leak codemod, so you can refer to it for a partial solution.
An ideal solution would just include as far as the last reference goes, but a simple one is just to move every statement after the assignment to the with block. You can get those by just grabbing all the statements in the parent cst.Module | cst.IndentedBlock with index greater than the assignment.

[clavedeluna]

I'm trying to understand your comment in the context of the current _make_args. If you look at all unit tests, every one uses delete=False. I purposefully didn't account for more complex cases in this iteration. Does that address your concern or did I miss something?

[andrecsilva]

Sorry for not being clear, let me try with an example.

Consider the following:

tmp_file = open(tempfile.mktemp(), "w+")
tmp_file.write("text")
print(tmp_file.name)

If I read the code correctly, the codemod will produce:

with tempfile.NamedTemporaryFile("w+") as tf:
    tmp_file = tf.name
tmp_file.write("text")
print(tmp_file.name)

Which means the file will be closed as soon as the assignment is done (due to the with statement). This ends with an error at the temp_file.write("text") (due to the file being closed).

My point is that the resulting code is quite useless as it only creates the file. Nothing else can be done with it since the file is closed almost as soon as it is created.

[clavedeluna]

The codemod isn't implemented to change your test example. It short circuits early because it doesn't match the pattern I've asked it for which is much more specific than what you have. In fact, there are similar tests all under the xfailed class, but I've added your test example as a "no_change" test to demonstrate this.

[drdavella]

Eventually I think we can/should make this codemod smarter and allow it to function much the same way as the resource leak codemod. But for now my understanding is that it will simply replace usages of tempfile.mktemp() in order to get a temporary filename.

Can we make sure to add an issue for the more sophisticated behavior if it doesn't already exist?

[clavedeluna]

Exactly! I communicated this at our previous meeting, this is iterative and this PR addresses only the first easiest cases as a way to demonstrate that we're aware the original codemod was just wrong. Will ticket the rest.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[drdavella]

But because we cannot directly know what the code may want the filename for, we're going to keep the file open and let the code author decide when to close it

I don't think this is true. The context manager ensures that the file is closed but not deleted. So there's no resource leak here and we assume that the process and/or OS will clean up the filesystem when it's no longer needed.

[drdavella]

This looks good to me as a fix although I think we all understand that eventually we can make this codemod more sophisticated and handle some of the currently failing edge cases.

@clavedeluna for the Sonar codemod is it possible to add an unfixed finding to CodeTF for the cases we can't handle? If not for this PR could you create a new issue?

[andrecsilva]

Looks fine, just needs a test that make changes inside a function.

[clavedeluna]

But because we cannot directly know what the code may want the filename for, we're going to keep the file open and let the code author decide when to close it

I don't think this is true. The context manager ensures that the file is closed but not deleted. So there's no resource leak here and we assume that the process and/or OS will clean up the filesystem when it's no longer needed.

this is right! I misspoke. Closed != deleted