This is an archive of the discontinued LLVM Phabricator instance.

[LibTooling] Relax Transformer to allow rewriting macro expansions
ClosedPublic

Authored by ymandel on Jul 10 2019, 10:48 AM.

Details

Summary

Currently, Transformer rejects any changes to source locations inside macro
expansions. This change relaxes that constraint to allow rewrites when the
entirety of the expansion is replaced, since that can be mapped to replacing the
entirety of the expansion range in the file source. This change makes
Transformer consistent with the handling of edit ranges in clang::edit::Commit
(which is used, for example, for applying FixItHints from diagnostics).

Diff Detail

Repository
rL LLVM

Event Timeline

ymandel created this revision.Jul 10 2019, 10:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2019, 10:48 AM

This clearly increases the utility of the library, but also seems to add corner cases that the library won't handle (see the comment about unittests for an example).
WDYT about those? Are they important, should we support producing warnings in those cases to let the users know things might get broken?

clang/lib/Tooling/Refactoring/Transformer.cpp
76 ↗(On Diff #209020)

Could we add unit tests for this particular function?

Interesting cases ([[ and ]] mark the start and end of a range):

#define FOO(a) a+a;
#define BAR 10+

// change part of a macro argument
int a = FOO([[10]] + 10);

// change the whole macro expansion
int b = [[FOO(10+10)]];

// Try to change 10 inside 'BAR', but not '+'.
// Should this fail? Should we give a warning?
int c = BAR 3; 

// Try changing the lhs (10) of a binary expr, but not rhs.
// Is that allowed? Should we give a warning?
int d = FOO(10);
99 ↗(On Diff #209020)

naming NIT: use BeginInfo

clang/unittests/Tooling/TransformerTest.cpp
625 ↗(On Diff #209020)

could we change to something other than 0 to make sure it's not the macro being expanded?

ymandel marked 2 inline comments as done.Jul 16 2019, 12:26 PM

This clearly increases the utility of the library, but also seems to add corner cases that the library won't handle (see the comment about unittests for an example).
WDYT about those? Are they important, should we support producing warnings in those cases to let the users know things might get broken?

That's a really good question. The code explicitly chooses to treat these failures like "this didn't match" rather than "this matched and now there's an error". That reflects the split that some users will want to know while others will want the system to always skip such matches, just like it skips non-matching expressions.

This seems like a good candidate for configuration -- the user could then choose which mode to run in. But, I'm also open to just reporting these conditions as errors. It's already in a context that returns Expected, so its no trouble; it's just a matter of choosing what we think is "correct".

clang/lib/Tooling/Refactoring/Transformer.cpp
76 ↗(On Diff #209020)

Sure. What do you think of exposing this function in clang/include/clang/Tooling/Refactoring/SourceCode.h and testing it from there?

ymandel updated this revision to Diff 210148.Jul 16 2019, 12:28 PM

tweaks in response to comments.

ymandel marked an inline comment as done.Jul 16 2019, 12:28 PM

This seems like a good candidate for configuration -- the user could then choose which mode to run in. But, I'm also open to just reporting these conditions as errors. It's already in a context that returns Expected, so its no trouble; it's just a matter of choosing what we think is "correct".

WRT to returning Expected vs Optional. Either seems fine and in the spirit of the library, depending on whether we want to produce more detailed errors. However, if we choose Optional let's stick to it, as practice shows switching from Optional to Expected correctly is almost impossible, as that requires a lot of attention to make sure all clients consume the errors (and given that it's an error case, tests often don't catch unconsumed errors).
I would personally go with Optional here (meaning the client code would have to say something generic like could not map from macro expansion to source code). But up to you, not a strong preference.

WRT to which cases we choose to handle, I'd start with a minimal number of supported examples (covering full macro expansion, or inside a single argument) and gradually add other cases as we find use-cases. What are your thoughts on that?

clang/lib/Tooling/Refactoring/Transformer.cpp
76 ↗(On Diff #209020)

Sounds reasonable. Was thinking of a better name, maybe something like getRangeForEdit()?
Would also suggest to accept SourceManager and LangOptions instead of MatchResult to narrow down the requirements on the clients.

This seems like a good candidate for configuration -- the user could then choose which mode to run in. But, I'm also open to just reporting these conditions as errors. It's already in a context that returns Expected, so its no trouble; it's just a matter of choosing what we think is "correct".

WRT to returning Expected vs Optional. Either seems fine and in the spirit of the library, depending on whether we want to produce more detailed errors. However, if we choose Optional let's stick to it, as practice shows switching from Optional to Expected correctly is almost impossible, as that requires a lot of attention to make sure all clients consume the errors (and given that it's an error case, tests often don't catch unconsumed errors).
I would personally go with Optional here (meaning the client code would have to say something generic like could not map from macro expansion to source code). But up to you, not a strong preference.

I think we might be talking about different things here. I meant that the *calling* function, translateEdits, returns Expected, so it would be easy to return an error when makeValidRange returns None. I agree that makeValidRange (or whatever we choose to call it) should stick with Optional for simplicity (with the generic interpretation of None being "could not map from macro expansion to source code").

WRT to which cases we choose to handle, I'd start with a minimal number of supported examples (covering full macro expansion, or inside a single argument) and gradually add other cases as we find use-cases. What are your thoughts on that?

I assume you mean which cases makeValidRange should handle (successfully)? If so, that sounds good. But, what do you think about how to handle failures of makeValidRange -- ignore them silently (which is what we're doing now) or treat them as errors?

This seems like a good candidate for configuration -- the user could then choose which mode to run in. But, I'm also open to just reporting these conditions as errors. It's already in a context that returns Expected, so its no trouble; it's just a matter of choosing what we think is "correct".

WRT to returning Expected vs Optional. Either seems fine and in the spirit of the library, depending on whether we want to produce more detailed errors. However, if we choose Optional let's stick to it, as practice shows switching from Optional to Expected correctly is almost impossible, as that requires a lot of attention to make sure all clients consume the errors (and given that it's an error case, tests often don't catch unconsumed errors).
I would personally go with Optional here (meaning the client code would have to say something generic like could not map from macro expansion to source code). But up to you, not a strong preference.

I think we might be talking about different things here. I meant that the *calling* function, translateEdits, returns Expected, so it would be easy to return an error when makeValidRange returns None. I agree that makeValidRange (or whatever we choose to call it) should stick with Optional for simplicity (with the generic interpretation of None being "could not map from macro expansion to source code").

Ah, great, we're on the same page then. LGTM!

WRT to which cases we choose to handle, I'd start with a minimal number of supported examples (covering full macro expansion, or inside a single argument) and gradually add other cases as we find use-cases. What are your thoughts on that?

I assume you mean which cases makeValidRange should handle (successfully)? If so, that sounds good.

Yes, exactly.

But, what do you think about how to handle failures of makeValidRange -- ignore them silently (which is what we're doing now) or treat them as errors?

I think it depends on the use-case, e.g. if we try to produce a clang-tidy fix for some warning and can't produce a fix because makeValidRange failed, then not showing the fix (i.e. failing silently) seems fine.
OTOH, if we're building a refactoring tool that should find an replace all occurrences of a matcher and apply the transformation, failing silently is probably not a good option, we should possibly list the locations where the transformation failed (so that users can do manual changes to complete the refactoring).

ymandel updated this revision to Diff 210575.Jul 18 2019, 8:11 AM

Moved makeValidRange to its own revision and rebased onto that.

ymandel updated this revision to Diff 210576.Jul 18 2019, 8:13 AM

remove unneeded include

ymandel marked 3 inline comments as done.Jul 18 2019, 8:15 AM
ymandel added inline comments.
clang/lib/Tooling/Refactoring/Transformer.cpp
76 ↗(On Diff #209020)

Went with passing the ASTContext rather than the MatchResult (in the new revision D64924)

Harbormaster completed remote builds in B35264: Diff 210576.
This revision is now accepted and ready to land.Jul 18 2019, 9:19 AM
This revision was automatically updated to reflect the committed changes.
ymandel marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2019, 10:44 AM