This is an archive of the discontinued LLVM Phabricator instance.

[clang-apply-replacements] Correctly handle relative paths
ClosedPublic

Authored by avogelsgesang on Oct 27 2021, 11:50 AM.

Details

Summary

The fixes from the YAML file can refer to relative paths.
Those relative paths are meant to be resolved relative to the
corresponding build directory.
However, clang-apply-replacements currently interprets all
paths relative to its own working directory. This causes issues,
e.g., when clang-apply-replacements is run from outside of
the original build directory.

This commit adjusts clang-apply-replacements to take the build
directory into account when resolving relative file paths.

Diff Detail

Event Timeline

avogelsgesang created this revision.Oct 27 2021, 11:50 AM
avogelsgesang requested review of this revision.Oct 27 2021, 11:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2021, 11:50 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Remove unrelated changes

Adding reviewers based on git log

I'm not really familiar w/ this code. But, if njames93 doesn't respond, then I'll give it a go.

Overall, this looks ok, but I don't feel like I'm qualified to approve this patch, since I'm not really familiar w/ how this tool is used in practice. Some comments nonetheless:

Those relative paths are meant to be resolved relative to the corresponding build directory.

Is this behavior documented somewhere?

clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
156

let's use an optional -- when populated, we have to copy it anyhow, so there's nothing gained by passing by pointer, and it comes at the cost of a nullable pointer.

159

Why are you capturing this as a reference? This is a subtle and IMO error prone pattern, since it's not obvious in the code below that you're mutating a deeply nested element of the filesystem. Can you instead use a local variable and just set this right before you need it?

162

assuming this is now an optional.

Those relative paths are meant to be resolved relative to the corresponding build directory.

Is this behavior documented somewhere?

I couldn't find this documented anywhere. My assumption is based on behavior which I observed from clang-tidy. Without this patch, clang-apply-fixes failed to apply the changes exported by clang-tidy --export-fixes due to its inability to find some source files referred to through relative paths

It seems the clang::tooling::Diagnostic::BuildDirectory was introduced in https://reviews.llvm.org/D26137. Maybe @alexfh who reviewed that patch can provide some more context here?

clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
159

Why are you capturing this as a reference?

Mostly to keep the code shorter. Otherwise, I would have to write

auto PrevWorkingDir = SM.getFileManager().getFileSystemOpts().WorkingDir;
if (buildDir)
  SM.getFileManager().getFileSystemOpts().WorkingDir = *buildDir;
// [...]
WorkingDir = SM.getFileManager().getFileSystemOpts().WorkingDir;

which isn't really DRY.

Can you instead use a local variable and just set this right before you need it?

I think I don't understand the alternative you are proposing here. Can you provide an example?

avogelsgesang marked 2 inline comments as done.

Use llvm::Optional instead of pointer as suggested by @ymandel

@alexfh, @ymandel: Do you know who could review this change?

Overall, looks good to me. But, I'd like to get a more experienced reviewer to confirm they're comfortable with the new behavior.

clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
159

What you wrote is what I had in mind, though the last line, i think, would be:
SM.getFileManager().getFileSystemOpts().WorkingDir = PrevWorkingDir;
?

That said, I see your point. The problem is not in your code so much as in the underlying (lack of) API for setting and restoring the working directory. Honestly, the need to manually restore bothers me more than the reference, now that I consider it in more detail. Perhaps another reviewer will have a better suggestion.

hokein added a subscriber: hokein.Nov 11 2021, 1:23 AM

this change looks good to me, I will leave the final stamp for @ymandel.

clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
159

rather than keeping the code short, I would be more explicit on setting and restoring the working directory, it is easier for readers to understand the intention (the FileManager's getter API which returns a mutable reference seems a bit weird to me).

ymandel added inline comments.Nov 11 2021, 5:26 AM
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
159

Ok, let's do that then. It's "wordy" but it will be very clear what's going on, and seems the best we can do given the odd API.

Don't update WorkingDir through local reference; as requested @ymandel and @hokein

ymandel accepted this revision.Nov 11 2021, 9:02 AM
This revision is now accepted and ready to land.Nov 11 2021, 9:02 AM

Thanks for the reviews, @ymandel and @hokein!

What are the next steps to get this landed?
(I am a first time contributor and have no commit access)

I can land it for you. should be sometime this afternoon, but ping me tomorrow if you don't see any activity.

This revision was automatically updated to reflect the committed changes.
dexonsmith added inline comments.
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
179

This looks incorrect.

The FileManager is not designed to have the working directory change and it's not sound to change/restore it. This is because it caches the meaning of every relative-path lookup.

Consider:

/dir1/file1.h
/dir2/file1.h
/dir2/file2.h

if you do:

FM.WorkingDir = "/dir1";
FM.getFile("file1.h"); // returns /dir1/file1.h
FM.WorkingDir = "/dir2";
FM.getFile("file1.h"); // returns /dir1/file1.h !!!
FM.getFile("file2.h"); // returns /dir2/file2.h

See also the comment at https://reviews.llvm.org/D122549#3412965.

I don't know anything about clang-apply-replacements, but I have a couple of ideas for handling this correctly:

  • Change the FileManager so that it's constructed with the working directory set to the build directory (don't save+restore, just make it correct from the start).
  • If there are multiple concepts of a "build directory", use a different FileManager for each one.
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 6:57 PM