[cfe-dev] [RFC] Adding refactoring support to Clang

[Alex Lorenz via cfe-dev]

> On Jun 16, 2017, at 11:38 AM, Ilya Biryukov <ibiryukov at google.com> wrote:
> 
>> - Does the code for the refactoring engine use ASTUnit? Can it be used without that?
> Not really. The core engine itself doesn’t really care about the ASTUnit. As long as it gets the ASTContext from somewhere it should work :) 
> Good to know, then it shouldn't be a problem to reuse your code in clangd even if we move away from ASTUnit. 
>> - Does refactoring engine have a separate component for testing correctness of the refactoring? Does it have an API to provide conflicts?
> Do you mean the correctness of the transformed source (i.e. it should be semantically identical to the original source) or something else? No, we don’t have a component that verifies semantic correctness. Furthermore, our implementation of “extract”s doesn’t provide any semantic correctness guarantees, as there are cases when the semantics of the program will change (we could probably diagnose them, but we don’t).
> I.e. a common check for rename would be to resolve all occurrences again after rename is completed and check that they actually resolve to the renamed entity. (I.e. there might've been a parameter with the same name and some references may now resolves to a parameter, rather than some class you were renaming. In case there are errors, we may want to notify users about those (that's what I call conflicts).

Yes, this is a potential issue. We don’t have any code that performs this kind of verification though.

> 
>> - What data does refactoring engine requires from Indexing API specifically?
> 
> What do you mean by the Indexing API? There’s no direct interaction with Clang’s indexing API and the refactoring engine. For global rename, Xcode’s indexer passes in the source location and symbol kinds for the previously indexed occurrences, but that relies on new refactoring APIs.
> I was asking about the data you pass from Xcode's indexer to refactoring engine. Got it, thanks.
>> - Why is 'Generate missing definitions' considered a cross-TU action? You actually touch only one file (i.e. the .cpp file where you put the definitions) and the AST for it should be enough for generating definitions.
> 
> The action is typically initiated in the header file, so we treat it as a separate TU. Also, C++ classes can be scattered across many CPP files, so even if we are initiating in a CPP file that contains the declaration of the class, it might not be the right CPP file (we put the out-of-line functions to the CPP file that already has the majority of the out-of-line methods from that class).
> Got it.
> Have you thought about actions that would require referring to classes/functions that aren't available in the TU you're changing (i.e. require includes to added in order to become available)?
> Say, we want to implement an action/refactoring to create a missing declaration inside class A (see code below):
> // Foo.h
> struct A {
>   int foo();
> };
> 
> // Foo.cpp
> #include "Foo.h"
> #include <string>
> 
> using namespace std;
> int A::bar(string a) {  // <-- action available here to create bar() in Foo.h
>   return 10;
> }
> 
> Can the current refactoring infrastructure be used to implement that? (i.e. add '#include <string>' to Foo.h, qualify string with 'std::' when creating a declaration?)
> If not, maybe you've already thought how we could make that possible?
>  

Good question. It should be possible to implement that using the new engine, you’d just have to teach it about <includes>. In your particular example we already have the “foo.h” included into “foo.cpp”, so we can do everything in one TU. I can think that an approach like this will work:

- Gather declarations from imported/included headers an associate them with those headers (in that particular example `A` is from Foo.h and `std::string` is from string). 
- Somehow determine their declaration status at the point of insertion (`A` is available, `std::string` isn’t). If a declaration isn’t available, we should include a header.

I think that it should be possible to support this kind of workflow for cross-TU operation as well, we’ d just have to persist the pointers to declarations and their associated headers across TUs.

I think the `string` will be qualified with `std` if you print it out, so I think the problem that we have here is actually the reverse - if you already include <string> and use namespace std in the header, then the `std::` prefix can be dropped. I believe there’s already existing code in lib/Tooling/Core that deals with that sort of thing, but I haven’t looked that much into it.





-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170616/df2a879e/attachment.html>