Page MenuHomePhabricator

[refactor] add clang-refactor tool with initial testing support and local-rename action
ClosedPublic

Authored by arphaman on Aug 10 2017, 5:47 AM.

Details

Summary

This patch depends on https://reviews.llvm.org/D36075 and https://reviews.llvm.org/D36156.

It introduces the clang-refactor tool alongside the local-rename action which is uses the existing renaming engine used by clang-rename. The tool doesn't actually perform the source transformations yet, it just provides testing support. I've moved one test from clang-rename over right now, but will move the others if the general direction of this patch is accepted.

The following options are supported by clang-refactor:

  • -v: use verbose output
  • -dump: dump results instead of applying them
  • -no-dbs: avoid searching/loading databases like compilation database and/or indexer stores/databases
  • -selection: The source range that corresponds to the portion of the source that's selected (currently only special command test:<file> is supported).

The testing support provided by clang-refactor is described below:
When -selection=test:<file> is given, clang-refactor will parse the selection commands from that file. The selection commands are grouped and the specified refactoring action invoked by the tool. Each command in a group is expected to produce an identical result. The precise syntax for the selection commands is described in a comment for findTestSelectionRangesIn in TestSupport.h.

Thanks for taking a look!

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
arphaman updated this revision to Diff 113533.Sep 1 2017, 4:20 AM

Rebase on top of trunk and https://reviews.llvm.org/D37210.

Ping.

klimek edited edge metadata.Sep 1 2017, 7:38 AM

One of my main concerns is still that I don't see the need for all the template magic yet :) Why doesn't everybody use the RefactoringResult we define here?

test/Refactor/LocalRename/Field.cpp
4 ↗(On Diff #113533)

Does this just test the selection?

tools/clang-refactor/ClangRefactor.cpp
135 ↗(On Diff #113533)

Can we use composition instead of inheritance here?

One of my main concerns is still that I don't see the need for all the template magic yet :) Why doesn't everybody use the RefactoringResult we define here?

This refactoring result is only really useful for the test part of clang-refactor so you can compare two results.

The non-test part of clang-refactor and other clients don't really need an abstract interface for results as they will have different code for different results anyway.

arphaman added inline comments.Sep 1 2017, 7:45 AM
test/Refactor/LocalRename/Field.cpp
4 ↗(On Diff #113533)

No, this is the moved clang-rename/Field.cpp test that tests local-rename. I will move the other tests when this patch is accepted.

arphaman updated this revision to Diff 113736.Sep 4 2017, 3:13 AM
arphaman marked an inline comment as done.

Get rid of inheritance of RefactoringEngine by getting rid of the class itself.

One of my main concerns is still that I don't see the need for all the template magic yet :) Why doesn't everybody use the RefactoringResult we define here?

@klimek, are you talking about the template usage in this patch or the whole approach in general? I can probably think of some way to reduce the template boilerplate if you are talking about the general approach.

klimek added a comment.Sep 4 2017, 3:29 AM

One of my main concerns is still that I don't see the need for all the template magic yet :) Why doesn't everybody use the RefactoringResult we define here?

@klimek, are you talking about the template usage in this patch or the whole approach in general? I can probably think of some way to reduce the template boilerplate if you are talking about the general approach.

Talking about the approach in general :)

test/Refactor/LocalRename/Field.cpp
4 ↗(On Diff #113533)

Ok, then I find this test really hard to read - where does it check what the symbol was replaced with?

ioeric added inline comments.Sep 4 2017, 3:41 AM
include/clang/Tooling/Refactoring/RefactoringAction.h
20 ↗(On Diff #110560)

Please add documentation for the class.

I appreciate your RFC that explains all these concepts; it would be nice if you could move some key concepts into the code documentation since we couldn't expect future readers to refer to the RFC. Thanks! ;)

include/clang/Tooling/Refactoring/SourceSelectionConstraints.h
42 ↗(On Diff #113736)

when?

55 ↗(On Diff #113736)

Can we pass RefactoringRuleContext instead of ASTRefactoringContext and get rules can get AST context from the rule context? I'm a bit nervous about having different contexts.

Maybe we should have just one interface T evaluateSelection(RefactoringRuleContext, SelectionConstraint) const. I think the rule context can be useful for all rules.

lib/Tooling/Refactoring/Rename/RenamingAction.cpp
35 ↗(On Diff #113736)

nit: It's a common practice to wrap implementations in namespaces instead of using using namespace. It becomes hard to manage scopes/namespaces when there are using namespaces.

tools/clang-refactor/ClangRefactor.cpp
60 ↗(On Diff #110560)

I think you would get a conflict of positional args when you have multiple sub-command instances.

Any reason not to use clang::tooling::CommonOptionsParser which takes care of sources and compilation options for you?

312 ↗(On Diff #110560)

Do we need a FIXME here? It seems that this simply finds the first command that is available? What if there are more than one commands that satisfy the requirements?

72 ↗(On Diff #113736)

Same here. There could be potential conflict among subcommands.

Also please provide more detailed description of this option; it is not obvious to users what a selection is.

One of my main concerns is still that I don't see the need for all the template magic yet :) Why doesn't everybody use the RefactoringResult we define here?

@klimek, are you talking about the template usage in this patch or the whole approach in general? I can probably think of some way to reduce the template boilerplate if you are talking about the general approach.

Talking about the approach in general :)

I'll work on a patch for a simpler solution as well then. It'll probably be dependent on this one though. I'm pretty sure I need some template magic to figure out how to connect inputs like selection and options and entities that produce source replacements , but I could probably get rid of most of RefactoringActionRule template magic.

include/clang/Tooling/Refactoring/SourceSelectionConstraints.h
42 ↗(On Diff #113736)

Oops, this isn't supposed to be in this patch, I will remove.

test/Refactor/LocalRename/Field.cpp
4 ↗(On Diff #113533)

I thought I should check for occurrences, but you're right, it's probably better to always check the source replacements (we can introduce options that apply non-default occurrences in the future which would allow us to check replacements for occurrences in comments).

Would something like //CHECK: "newName" [[@LINE]]:24 -> [[]]:27 work when checking for replacements?

tools/clang-refactor/ClangRefactor.cpp
60 ↗(On Diff #110560)

Not from my experience, I've tried multiple actions it seems like the right arguments are parsed for the right subcommand. It looks like the cl option parser looks is smart enough to handle identical options across multiple subcommands.

312 ↗(On Diff #110560)

I don't quite follow, when would multiple subcommands be satisfied? The subcommand corresponds to the action that the user wants to do, there should be no ambiguity.

klimek added inline comments.Sep 4 2017, 5:09 AM
test/Refactor/LocalRename/Field.cpp
4 ↗(On Diff #113533)

Yep, I think that'd be better.
Or we could just apply the replacement and check on the replaced code? (I think that's what we do in fixits for clang and clang-tidy)

arphaman updated this revision to Diff 113804.Sep 5 2017, 12:53 AM
arphaman marked 8 inline comments as done.
  • Test by checking the modified source directly. This allowed me to get rid of the refactoring result wrapper as well.
  • Remove ASTRefactoringContext
  • Address the other review comments
arphaman added inline comments.Sep 5 2017, 12:54 AM
tools/clang-refactor/ClangRefactor.cpp
60 ↗(On Diff #110560)

I agree that using CommonOptionsParser would be preferable, but right now it doesn't work well with subcommands. I will create a followup patch that improves subcommand support in CommonOptionsParser and uses them in clang-refactor when this patch is accepted.

ioeric added inline comments.Sep 5 2017, 2:37 AM
include/clang/Tooling/Refactoring/RefactoringActionRules.h
45 ↗(On Diff #113804)

Can we get rid of this overload and simply make RefactoringRuleContext mandatory for all refactoring functions?

include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
99 ↗(On Diff #113804)

Do we still need this overload?

include/clang/Tooling/Refactoring/RefactoringRuleContext.h
44 ↗(On Diff #113804)

Should we also provide an interface to test if this has an ASTContext? Or maybe simply return null if there is ASTContext? WDYT?

lib/Tooling/Refactoring/Rename/RenamingAction.cpp
50 ↗(On Diff #113804)

Should this be private?

tools/clang-refactor/ClangRefactor.cpp
60 ↗(On Diff #110560)

Thanks! Could you add a FIXME for the CommonOptionsParser change?

312 ↗(On Diff #110560)

(I added this comment a while ago, and it seemed to have shifted. I was intended to comment on line 297 auto It = llvm::find_if(. Sorry about that.)

I guess I don't quite understand how a specific subcommand is picked based on the commandline options users provide. The llvm::find_if above seems to simply find the first action/subcommand that is registered? Could you add a comment about how submamands are matched?

arphaman updated this revision to Diff 113826.Sep 5 2017, 3:31 AM
arphaman marked 6 inline comments as done.
  • Always pass in RefactoringRuleContext to the refactoring rule's function.
  • Remove now redundant overloads.
  • Make LocalRename's actions members private.
  • Add CommandLienParser FIXME comment.
  • Verify that refactoring actions have unique command names.
  • Add a comment that explains how a subcommand is selected.
include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
99 ↗(On Diff #113804)

Removed.

tools/clang-refactor/ClangRefactor.cpp
60 ↗(On Diff #110560)

Done.

312 ↗(On Diff #110560)

Ah, I see.

I added a comment in the updated patch that tries to explain the process. Basically we know that one action maps to just one subcommand because the actions must have unique command names. Since we've already created the subcommands, LLVM's command-line parser enables one particular subcommand that was specified in the command-line arguments. This is what this search does, we just look for the enabled subcommand that was turned on by LLVM's command-line parser, without taking any other command-line arguments into account.

ioeric added inline comments.Sep 6 2017, 2:50 AM
lib/Tooling/Refactoring/Rename/RenamingAction.cpp
43 ↗(On Diff #113826)

Shouldn't these be public?

tools/clang-refactor/ClangRefactor.cpp
312 ↗(On Diff #110560)

Ohh! I didn't notice RefactoringActionSubcommand inherits cl::SubCommand. Thanks for the explanation!

79 ↗(On Diff #113826)

Why return a mutable reference? Also, this is not used in this patch; maybe leave it out?

103 ↗(On Diff #113826)

Is the test selection temporary before we have true selection?

I am a bit concerned about having test logic in the production code. It makes the code paths a bit complicated, and we might easily end up testing the test logic instead of the actual code logic. I'm fine with this if this is just a short term solution before we have the actual selection support.

tools/clang-refactor/TestSupport.cpp
187 ↗(On Diff #113826)

I think the ...In suffix is redundant. We could either get rid of it or use ...InFile.

arphaman added inline comments.Sep 6 2017, 3:08 AM
tools/clang-refactor/ClangRefactor.cpp
103 ↗(On Diff #113826)

No, both are meant to co-exist. I guess we could introduce a new option (-selection vs -test-selection and hide -test-selection)? Another approach that I was thinking about is to have a special hidden subcommand for each action (e.g. test-local-rename). Alternatively we could use two different tools (clang-refactor vs clang-refactor-test)? Both will have very similar code though.

Note that it's not the first time test logic will exist in the final tool. For example, llvm-cov has a convert-for-testing subcommand that exists in the production tool. I'm not saying that it's necessarily the best option though, but I don't think it's the worst one either ;)

arphaman updated this revision to Diff 113981.Sep 6 2017, 3:32 AM
arphaman marked 3 inline comments as done.

Address review comments

ioeric added inline comments.Sep 6 2017, 5:38 AM
tools/clang-refactor/ClangRefactor.cpp
103 ↗(On Diff #113826)

I guess I am more concerned about mixing production code with testing code than having special options. For example, we have different invocation paths based on ParsedTestSelection. IMO, whether selected ranges come from test selection or actual selection should be transparent to clang-refactor. Maybe refactoring the selection handling logic out as a separate interface would help?

arphaman updated this revision to Diff 114125.Sep 7 2017, 12:48 AM

Refactor the selection argument handling by using a SourceSelectionArgument interface with subclass for test selection and separate out the test code from the tool class.

arphaman marked an inline comment as done.Sep 7 2017, 12:48 AM
arphaman added inline comments.
tools/clang-refactor/ClangRefactor.cpp
79 ↗(On Diff #113826)

Removed.

103 ↗(On Diff #113826)

Makes sense, I just did a cleanup refactoring that should make it better.

ioeric added inline comments.Sep 7 2017, 1:29 AM
tools/clang-refactor/ClangRefactor.cpp
29 ↗(On Diff #114125)

Sorry for haven't noticed this earlier, but I think clang::refactor sounds like a better name space than clang::clang_refactor.

62 ↗(On Diff #114125)

I would expect the SourceSelectionArgument to be more like a container of ranges and further decoupled with the refactoring logic. Same for the TestSelectionRangesInFile.

How about an interface like this?

template <typename T>
void ForAllRanges(T callback) const;
64 ↗(On Diff #114125)

Do we want to consider other result types?

arphaman updated this revision to Diff 114309.Sep 8 2017, 12:50 AM
arphaman marked 2 inline comments as done.
  • rename namespace
  • try to further decouple SourceSelectionArgument
arphaman added inline comments.Sep 8 2017, 12:51 AM
tools/clang-refactor/ClangRefactor.cpp
62 ↗(On Diff #114125)

Not sure I fully understand your interface suggestion, but I've tried to decouple some more in the updated patch.

ioeric accepted this revision.Sep 8 2017, 1:24 AM

I think this is ready to go.

@klimek Manuel, are all your concerns addressed?

tools/clang-refactor/ClangRefactor.cpp
62 ↗(On Diff #114125)

Sorry, I should've been more specific about what I had in mind, but what you have right now is close. I think this can be further decoupled a bit by having forAllRanges take in SourceManeger instead of RefactoringRuleContext since source selection should only care about source ranges.

62 ↗(On Diff #114309)

IIUC, createCustomConsumer is used to inject testing behavior into clang-refactor. I think this is fine, but please document the intention and the behavior. Thanks!

This revision is now accepted and ready to land.Sep 8 2017, 1:24 AM
ioeric added a comment.Sep 8 2017, 1:26 AM

Please wait for Manuel's reply before landing the patch ;-)

@klimek I've added a patch that simplifies the interface and the template code - https://reviews.llvm.org/D37681. Hopefully you'll find that the code there is cleaner.

Feel free to land the patch now if comments are addressed.

klimek accepted this revision.Sep 12 2017, 12:49 AM

LG. Nice, thanks!

lib/Tooling/Refactoring/Rename/RenamingAction.cpp
42 ↗(On Diff #114309)

I have to admit, the implementation here is pretty neat! :)

hokein accepted this revision.Sep 12 2017, 2:23 AM

LGTM, a few nits.

lib/Tooling/Refactoring/Rename/RenamingAction.cpp
42 ↗(On Diff #114309)

+1

test/Refactor/LocalRename/Field.cpp
4 ↗(On Diff #114309)

I'd use a completed statement in CHECK (like int /*range=*/Bar;), which is more obvious to readers.

test/Refactor/tool-test-support.c
29 ↗(On Diff #114309)

Maybe add a comment describing the following cases are named test group -- it took me a while to understand why these "invoking action" messages aren't ordered by the original source line number.

tools/clang-refactor/ClangRefactor.cpp
89 ↗(On Diff #114309)

nit: virtual isn't needed.

tools/clang-refactor/TestSupport.cpp
28 ↗(On Diff #114309)

I'm not a big fan of using namespace. It would make it hard to find out which namespace is the following method in -- readers have to go to the corresponding header file to find out the namespace. I'm +1 on using a more regular way namespace clang { namespace refactor {...} }.

127 ↗(On Diff #114309)

nit: virtual can be removed, the same below.

tools/clang-refactor/TestSupport.h
32 ↗(On Diff #114309)

Do you plan to use refactor on other files?

104 ↗(On Diff #114309)

s/clang_refactor/refactor

This revision was automatically updated to reflect the committed changes.
arphaman reopened this revision.Sep 12 2017, 5:51 AM

Oops, I've put in the wrong diff link for r313025. This patch is still not committed.

This revision is now accepted and ready to land.Sep 12 2017, 5:51 AM
arphaman marked 8 inline comments as done.Sep 14 2017, 2:34 AM
arphaman added inline comments.
tools/clang-refactor/TestSupport.h
32 ↗(On Diff #114309)

Probably in the future, yeah.

Seems that you have attached a wrong diff, which is committed in r313025.

tools/clang-refactor/TestSupport.h
32 ↗(On Diff #114309)

SG.

Yeah, I accidentally committed a prior patch with the link to this diff. I will commit this patch today though so it should be updated.

This revision was automatically updated to reflect the committed changes.