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

[Alex Lorenz via cfe-dev]

> On Jun 19, 2017, at 5:02 AM, Сергей Прейс <spreis at yandex-team.ru> wrote:
> 
> Hello Alex,
>  
> This is great effort! I will monitor it closely for my employer in-house projects and will consider particitpation if find my help applicable. Do you plan any protection from incorrect source positions reported by clang in AST for some entities?

In the context of “Global Rename”, the engine shouldn't rename an identifier whose name is not the same as the name of the renamed symbol. This does not catch all cases though, as the incorrect source position might point to the same identifier. The number of incorrect source positions should be zero though, so we should just fix these issues.

My stress-test tool that I mentioned in the RFC can actually help find such bugs, as it can detect cases when our AST misses a location for some symbol (by trying to initiate a rename at every non-keyword identifier token). Unfortunately it has a lot of false positives, so I haven’t had time to look into it that much.

> I recently implemented c++ indexer and found at least one case where SourceLocation information doesn't match (https://bugs.llvm.org/show_bug.cgi?id=26195) <https://bugs.llvm.org/show_bug.cgi?id=26195)>. I tried to find name reported in AST at SourceLoc attached to the entity. In some cases names are articificial and don't naturally match, but the case in question is real issue. There may be other cases (I don't index absolutely everyting in AST), so, probably, some protection logic is needed agains similar issue. I have fix for this particular issue with test exhibiting the offending behavior using c-index-test on trunk clang (https://reviews.llvm.org/D32439) <https://reviews.llvm.org/D32439)>, so you may look and see how it is relevant to your development.
>  

Thanks! I’ll take a look this week.

> Thak you,
> -- 
> Serge Preis
>  
>  
>  
>  
> 17.06.2017, 02:00, "Alex Lorenz via cfe-dev" <cfe-dev at lists.llvm.org>:
>>  
>>  
>>> 
>>> On Jun 16, 2017, at 11:38 AM, Ilya Biryukov <ibiryukov at google.com <mailto: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.
>>  
>>  
>>  
>>  
>> ,
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170619/1abba27f/attachment.html>