Page MenuHomePhabricator

[refactor] Initial support for refactoring action rules
ClosedPublic

Authored by arphaman on Jul 31 2017, 1:15 AM.

Details

Summary

This patch implements a couple of functions that were described in my RFC from last week that’s available at http://lists.llvm.org/pipermail/cfe-dev/2017-July/054831.html. This patch adds the following pieces: apply function, requiredSelection function, and the selection::SourceSelectionRange constraint.

This code will be used as a base to start moving the functionality and tests for clang-rename over to clang-refactor.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Jul 31 2017, 1:15 AM
arphaman updated this revision to Diff 110563.Aug 10 2017, 5:46 AM
arphaman edited the summary of this revision. (Show Details)
  • Simplify error/diagnostic handling. Use DiagnosticOr instead of DiagOr<Expected>.
  • Simplify the code for the selection requirements by removing lambda deducers and instead using special classes for requirements instead of lambdas/functions.
  • Rename selectionRequirement to requiredSelection

+ Introduce refactoring diagnostics.

ioeric edited edge metadata.Aug 18 2017, 9:41 AM

This is great work and definitely a lot to digest! ;) Some high-level comments for the first round.

In general, I really appreciate the high-level interfaces; I am just a bit concerned about the implementation which is a bit hard to follow at this point, especially with all the template magics. I left some suggestions in the comments; hopefully they would help.

I would also appreciate more detailed high-level comments on the major classes and their relationships - I found myself constantly going back to the RFC to understand their intentions.

include/clang/Basic/DiagnosticOr.h
1 ↗(On Diff #110563)

Could you please split changes related to DiagnosticOr into a separate patch and have owners of clang/Basic/ directory review it? ;)

include/clang/Tooling/Refactoring/RefactoringActionRules.h
2

Code in this file is a bit hard to follow, especially with so many classes and template magics :P

Is it possible to split them into smaller modules, and ideally with more detailed documentation for each?

It seems to me that the following logics could live in their own headers:

  • RefactoringActionRule interface.
  • Base classes for requirements.
  • SourceSlection-related requirements.
  • Derived/specialized rules.
228

Why is this called apply? I feel something like createAction or generateAction would be more intuitive.

include/clang/Tooling/Refactoring/RefactoringOperationController.h
19 ↗(On Diff #110563)

What are all the possible states, for example?

22 ↗(On Diff #110563)

Maybe I am missing too much context, but I found it hard to understand the comment and couldn't relate the comment to the public interfaces. Could you elaborate more?

It is also unclear to me if this is specific to the source selection.

40 ↗(On Diff #110563)

It's unclear to me what the intentions of these members are. Could you add some comments for these members?

include/clang/Tooling/Refactoring/RefactoringResult.h
36

Do we expect the result changes to be modified? Why?

include/clang/Tooling/Refactoring/SourceSelectionConstraints.h
22

nit: redundant be used.

36

It might worth explaining the relationship between this and the RequirementBase.

alexander-shaposhnikov added inline comments.
include/clang/Basic/DiagnosticOr.h
40 ↗(On Diff #110563)

explicit ?

49 ↗(On Diff #110563)

but probably it would be a bit cleaner to enable SFINAE via a template parameter (http://en.cppreference.com/w/cpp/types/enable_if , option #4) rather than via extra argument.

include/clang/Tooling/Refactoring/RefactoringResult.h
28

explicit ?

Extracted DiagnosticOr to a separate patch at https://reviews.llvm.org/D36969.

I will update this patch tomorrow.

arphaman marked an inline comment as done.Aug 21 2017, 8:56 AM
arphaman updated this revision to Diff 112195.Aug 22 2017, 10:18 AM
arphaman marked 7 inline comments as done.
  • Split the header
  • Remove DiagnosticOr in favour of Expected that will use a DiagnosticError that was proposed in the other patch.
  • Address the other review comments
include/clang/Tooling/Refactoring/RefactoringOperationController.h
19 ↗(On Diff #110563)

Bad comment. I think inputs is more descriptive.

include/clang/Tooling/Refactoring/RefactoringResult.h
28

Nah, it's more convenient to be able to return a single AtomicChanges without an explicit initializer I think.

36

No, that was a workaround for a non-const member use. I'll use a const cast instead.

include/clang/Tooling/Refactoring/SourceSelectionConstraints.h
36

This class is not related to RequirementBase though. Were you talking about another class?

Thanks for the changes! The code is much clearer.

I am wondering if the current design could be extended to support tools (or rules) that use AST matchers? Or is the selection expected to be powerful enough to replace AST matchers?

We have a few tools in clang-tools-extra (e.g. clang-move and change-namespace) and many internal tools that are based on AST matchers, but we would really like to move them into clang-refactor in the future :)

include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
26

Could you help me understand this class?

This seems to be a selection-specific requirement and should live in selection. It is also used in BaseSpecializedRule which seems to be a higher level of abstraction.

include/clang/Tooling/Refactoring/RefactoringResult.h
21

I'm a bit unsure about the abstraction of the refactoring result. I would expected refactoring results to be source changes always. Do you have any refactoring tool that outputs occurrences in mind?

28

+1 to explicit which could prevent unintentional conversion. RefactoringResult(Change); isn't too bad IMO.

include/clang/Tooling/Refactoring/SourceSelectionConstraints.h
55

Does detail mean internal implementation? Maybe use internal which is more commonly used for this?

arphaman added inline comments.Aug 24 2017, 4:49 AM
include/clang/Tooling/Refactoring/RefactoringResult.h
21

In Xcode we require rename to return symbol occurrences because the IDE is responsible for figuring out:

  1. The new name. Thus we can't produce replacements when renaming because we don't know what the new name is when gathering the occurrences.
  2. Whether these occurrences should be actually applied. Thus we can't produce replacements because it's up to the user to decide if they want to rename some occurrence in a comment for example.

In general 2) can be applied to tools like clang-refactor that could allow users to select occurrences that don't guarantee a direct semantic match (comments, etc.) in an interactive manner.

I think clangd has a rather naive rename support, so these points may not apply, but we may want to extend clangd's support for rename in the future as well.

Thanks for the changes! The code is much clearer.

I am wondering if the current design could be extended to support tools (or rules) that use AST matchers? Or is the selection expected to be powerful enough to replace AST matchers?

We have a few tools in clang-tools-extra (e.g. clang-move and change-namespace) and many internal tools that are based on AST matchers, but we would really like to move them into clang-refactor in the future :)

The selection requirement is supposed to be orthogonal to AST matchers, not a replacement. It should be used when working with source selection in editors.

I did mess around with moving over clang-reorder-fields using this kind of model and didn't see any issues when using AST matchers. Essentially I used the requiredOption requirements and mapped them to run my matching code instead of using the selection requirement. Thus this requirement was satisfied only when the AST matchers were successful. It might be possible to simplify that pattern even further to make it simpler.

Thanks for the changes! The code is much clearer.

I am wondering if the current design could be extended to support tools (or rules) that use AST matchers? Or is the selection expected to be powerful enough to replace AST matchers?

We have a few tools in clang-tools-extra (e.g. clang-move and change-namespace) and many internal tools that are based on AST matchers, but we would really like to move them into clang-refactor in the future :)

arphaman added inline comments.Aug 24 2017, 5:05 AM
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
26

It's just a container class that stores all information about the requiredSelection requirement. I agree about BaseSpecializedRule, that connection should be chopped. I will move the evaluation code into the requirement itself when I update the patch.

I would tend to disagree about moving it though, as SourceSelectionRequirement is a requirement first and I think that's why it should live with other requirements. Yes, it's related to selections, but it uses them to implement the requirement. I think it's better to keep requirements together, as opposed to having option requirements close to options, selection requirements close to selection, and so on. WDYT?

arphaman updated this revision to Diff 112566.Aug 24 2017, 8:12 AM
arphaman marked 2 inline comments as done.
  • Rename detail to internal.
  • Remove the BaseSpecializedRule.
  • Simplify selection requirement and constraint evaluation.
ioeric added inline comments.Aug 24 2017, 9:20 AM
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
26

Thanks!

Makes sense. We might want to put individual requirements into their own headers so that this doesn't grow into a huge file when more requirements are supported.

include/clang/Tooling/Refactoring/RefactoringActionRuleRequirementsInternal.h
43

It might worth having a comment explaining why and how Expected<Optional> is wrapped and unwrapped during the evaluation.

include/clang/Tooling/Refactoring/RefactoringActionRules.h
34

We might want to document supported requirements somewhere else so that we don't need to update this file every time a new requirement is added.

include/clang/Tooling/Refactoring/RefactoringOperation.h
2

s/RefactoringOperationController.h/RefactoringOperation.h/ :)

30

I found the name a bit confusing - RefactoringOperation sounds a bit like RefactoringAction.

Would it make sense to call this RefactoringContext or RefactoringRuleContext, if this stores states of a refactoring rule?

include/clang/Tooling/Refactoring/RefactoringResult.h
21

I feel occurrences are more of an intermediate state of a refactoring action than a result. I'm wondering if it makes sense to introduce a separate class to represent such intermediate states? I am a bit nervous to fuse multiple classes into one; the interfaces can get pretty ugly when more result kinds are added.

lib/Tooling/Refactoring/SourceSelectionConstraints.cpp
13 ↗(On Diff #112566)

We are tempted to avoid using namespace if possible.

arphaman added inline comments.Aug 24 2017, 9:36 AM
include/clang/Tooling/Refactoring/RefactoringResult.h
21

Good point. I agree.

I think it would be better to differentiate between RefactoringActionRules then. Ordinary rules return a set of AtomicChanges instead of RefactoringResult. But then we could also have "interactive" rules that return "partial" results like symbol occurrences.

I think I'll try the following approach:

class RefactoringActionRule {
  virtual ~RefactoringActionRule() {}
};

class RefactoringActionSourceChangeRule: public RefactoringActionRule {
public:
  virtual Expected<Optional<AtomicChanges>>
  createSourceReplacements(RefactoringOperation &Operation) = 0;
};

class RefactoringActionSymbolOccurrencesRule: public RefactoringActionRule {
public:
  virtual Expected<Optional<SymbolOccurrences>>
  findSymbolOccurrences(RefactoringOperation &Operation) = 0;
};
arphaman added inline comments.Aug 24 2017, 9:40 AM
lib/Tooling/Refactoring/SourceSelectionConstraints.cpp
13 ↗(On Diff #112566)

Why? It's not in a header. using namespace clang is the common practice across all of Clang's sources.

@arphaman

The selection requirement is supposed to be orthogonal to AST matchers, not a replacement. It should be used when working with source selection in editors. 
I did mess around with moving over clang-reorder-fields using this kind of model and didn't see any issues when using AST matchers. Essentially I used the  requiredOption requirements and mapped them to run my 
matching code instead of using the selection requirement. Thus this requirement was satisfied only when the AST matchers were successful. 
It might be possible to simplify that pattern even further to make it simpler.

that's great, i'm interested in this too and would be happy to see clang-reorder-fields moving to clang-refactor (pls, let me know if i can help make this happen)

arphaman marked 5 inline comments as done.Aug 25 2017, 4:59 AM
arphaman added inline comments.
include/clang/Tooling/Refactoring/RefactoringActionRules.h
34

Do you think it should be in Clang's documentation? I can start on a new document there but I'd prefer to do it in a separate patch. WDYT?

arphaman updated this revision to Diff 112678.Aug 25 2017, 5:00 AM
  • Get rid of RefactoringResult in favour of different rule types.
  • Rename RefactoringOperation to RefactoringRuleContext.
  • Address a couple more comments.
ioeric accepted this revision.Aug 28 2017, 2:07 AM

Thanks for the changes! Lgtm with a few nits.

include/clang/Tooling/Refactoring/RefactoringActionRules.h
34

Sure, this is fine for now. It would be nice to have proper documentation in the future when pieces get into places.

include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
34

Why isn't this a interface in SpecificRefactoringRuleAdapter with return type Expected<Optional<T>>?

unittests/Tooling/RefactoringActionRulesTest.cpp
68

It would be nice to also rename the variable from Operation to Context.

This revision is now accepted and ready to land.Aug 28 2017, 2:07 AM

Thanks,

I'll start working on the documentation patch and will split follow-up clang-refactor patch into one or two parts today.

include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
34

A method declaration in SpecificRefactoringRuleAdapter won't work since it won't be available in either the template specialisation or the deriving class as the classes won't be directly related. I could use a separate parent class independent of SpecificRefactoringRuleAdapter that declares a generic interface though.

arphaman marked an inline comment as done.Aug 28 2017, 3:59 AM
This revision was automatically updated to reflect the committed changes.
klimek added inline comments.Aug 28 2017, 8:02 AM
cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRule.h
23–37 ↗(On Diff #112879)

Sorry for being late, was out on vacation.
Generally, why do we need this tag-based abstraction here instead of using the more typical OO double-dispatch where necessary?
(We do this in the AST a lot, but the AST is special, because there we want to implement a lot of different algorithms that rely on the node type, while I don't see how that applies here)

arphaman added inline comments.Aug 28 2017, 8:10 AM
cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRule.h
23–37 ↗(On Diff #112879)

Generally the clients will have to somehow distinguish between the types of results that are produced by rules to figure out what to do (e.g. AtomicChanges -> apply, SymbolOccurrences -> ask user, Continuation -> look for more ASTs). So far I've thought that the LLVM based dynamic casts will work well for this, e.g.

if (auto *Action = dyn_cast<SourceChangeRefactoringRule>()) {
  Expected<Optional<AtomicChanges>> Changes = Action->createSourceReplacements();
  applyChanges(Changes);
} else if (...) {
  ...
} else (...) {
   ...
}

But you're probably right, there might be a better way to do this rather than the tag based approach. Something like a consumer class that clients can implement that provides consumer functions that take in the specific results. I reckon a single consumer will actually work better in the long-run when we might Continuations that both return changes in the first TU and information for searches in other TUs. I'll see if I can get a patch out that removes this tag and uses the consumer approach.

klimek added inline comments.Aug 28 2017, 8:19 AM
cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRule.h
23–37 ↗(On Diff #112879)

Cool, looking forward to it :)