[PATCH] D76432: [clangd] Add a tweak for adding "using" statement.

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:34
+    // True iff "using" already exists and we should not add it.
+    bool IdenticalUsingFound = false;
+    // Location to insert the "using" statement.
----------------
sammccall wrote:
> is this redundant with Loc.isValid()?
> 
> If so, I'd either just use Loc (with a comment), or use optional<SourceLocation> with a comment, to emphasize the relationship.
Yes, it's redundant. I went back and forth on this one, original version used Loc.isValid(). I decided I like being explicit better than having slightly more complicated semantics of the Loc field, but I wasn't too confident about this. Having second opinion helps :-)

I switched to isValid()




================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:37
+    SourceLocation Loc;
+    // Extra suffix to place after the "using" statement. Depending on what the
+    // insertion point is anchored to, we may need one or more \n to ensure
----------------
sammccall wrote:
> can we get away with always inserting \n and relying on clang-format to clean it up?
Nope, that doesn't work. Auto-formatting doesn't seem to remove blank lines between using statements, for example. It's likely to allow you grouping them in some way.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:44
+  // SourceLocation if the "using" statement already exists.
+  llvm::Expected<InsertionPointData>
+  findInsertionPoint(const Selection &Inputs);
----------------
sammccall wrote:
> this doesn't use any state - make it static or a free function?
It uses Name and NNSL . Would you prefer this as free function, with both passed as arguments. My initial thought was that, since prepare() and apply() already share these via class members this was fine, but I'm certainly not against making this a free function.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:56
+  return std::string(
+      llvm::formatv("Add using statement and remove full qualifier."));
+}
----------------
sammccall wrote:
> nit: not actually a statement :-(
> 
> If "using-declaration" is overly laywerly (and easily confused with using-directive), then maybe we should just spell it out: `Add "using foo::Bar" ...`
I think spelling it out may be too long sometimes, namespaces are often long.

Changed to using-declaration


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:71
+      return true;
+    }
+    if (D->getDeclContext()->Encloses(SelectionDeclContext)) {
----------------
sammccall wrote:
> nit: we conventionally leave out the {} on one-line if bodies etc.
Uhh...is that a hard rule? I personally hate that, it's just asking for Apple SSL-style bugs
https://www.imperialviolet.org/2014/02/22/applebug.html


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:89
+
+  // If we're looking at a type or NestedNameSpecifier, walk up the tree until
+  // we find the "main" node we care about, which would be ElaboratedTypeLoc or
----------------
sammccall wrote:
> I like the idea here, but I'm not sure it quite works. e.g. any typeloc has a directly enclosing typeloc, the inner one can't be targeted. So what about `x::S*`? (pointertypeloc > elaboratedtypeloc > recordtypeloc)?
> 
> - looping up from NNS until you get to something else makes sense
> - unwrapping typeloc -> elaboratedtypeloc makes sense, but I don't think you ever want to unwrap multiple levels?
> - i don't think these cases overlap at all, you want one or the other
> 
> Am I missing something?
If you have a class foo::bar::cc and a struct st inside it, and then do:
foo::bar::c^c::st *p;
you end up with:
PointerTypeLoc -> ElaboratedTypeLoc ->NestedNameSpecifierLoc -> RecordTypeLoc
in which case you need to go up from type to NNSL and then up again, from NNSL to something that's not NNSL.

You have a good point with the PointerTypeLoc, that's a bug. We should stop going up the tree as soon as we find ElaboratedTypeLoc. I added a test for that.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:132
+llvm::Expected<AddUsing::InsertionPointData>
+AddUsing::findInsertionPoint(const Selection &Inputs) {
+  auto &SM = Inputs.AST->getSourceManager();
----------------
sammccall wrote:
> One possible hiccup is insertion splitting a comment from its target. Cases I can think of:
> - `namespace { // anonymous` -> `namespace { using foo::bar; // anonymous`. This seems rare/unimportant.
> - `/* docs */ int firstTopLevelDecl` -> `/* docs */ using foo::bar; int firstTopLevelDecl`. This seems common/bad.
> - `/* for widgets */using foo::Qux;` -> `/* for widgets */ using foo::bar; using foo::Qux`. This seems rare/unimportant.
> 
> I think you could handle the decl case by calling `ASTContext::getLocalCommentForDeclUncached()` and using the getBeginLoc() of the returned comment if any.
> 
> That said, this is probably something that we should be doing throughout clangd and we should have some common function in AST.h or so to take care of it. It seems fine to leave a comment and we can think about this later. (Or file a bug where we can dump a list of places that should be accounting for the comment range)
Added a comment. I'd be happy to fix that, but I feel it's best done in a separate change.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:144
+
+  llvm::sort(Usings, [&SM](const UsingDecl *L, const UsingDecl *R) {
+    return SM.isBeforeInTranslationUnit(L->getUsingLoc(), R->getUsingLoc());
----------------
sammccall wrote:
> This is basically always going to be already sorted. AST traversal isn't strictly left-to-right but pretty close (I think the major exceptions are weird objc things). So you could leave this out.
Done. I also added a comment to mention that we assume it's sorted.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:161
+    // We could be smarter and check if the deepest block of "using" is sorted
+    // alphabetically and if so, insert at appropriate place. For now, users can
+    // just reformat the file with clang-format or similar.
----------------
sammccall wrote:
> the tweak infrastructure should do this in any case, it runs clang-format around changed areas (assuming using-decl sorting is on, which it is in the default styles).
> 
> do you not see this behaviour? (unit-tests won't exhibit it as the clang-format step is deliberately excluded from them)
Didn't know that was the default. I'll drop the comment in that case.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:206
+  tooling::Replacements R;
+  if (auto Err = R.add(tooling::Replacement(
+          SM, CharSourceRange::getTokenRange(NNSL.getSourceRange()), "",
----------------
sammccall wrote:
> (Sorry if some of this is obvious: TL;DR: we should use TokenBuffer to handle macros in this case)
> 
> Whenever we try to use SourceLocations to reason about written source code, we have to think about macros. Some libraries try to encapsulate this, but the road to confusion is paved with good intentions, and it's often best to decide on the policy and be explicit about it.
> 
> For example, when provided a macro location, the tooling::Replacement constructor uses its spelling location. 
> Given: 
> ```
> // imagine we're trying to abstract away namespaces for C or something
> #define NS(X) foo::X
> NS(Bar) boo;
> ```
> Running this action on `N` would result in changing the definition of the `NS` macro to delete the "foo::", as that's where the qualifier was spelled!
> 
> It would be reasonable (but actually not trivial!) to try to detect any macro usage and bail out. The general case of "is there some sequence of source-code tokens that correspond to this range of preprocessed tokens and nothing else" is pretty hard.
> 
> However TokenBuffer does have APIs for this exact question, as it was designed for writing refactorings that replace nodes. I think you want:
>  - `expandedTokens(NNSL.getSourceRange())`
>  - `spelledForExpanded(ExpandedTokens)`
>  - `Token::range(SM, Spelled.front(), Spelled.back())`
>  - `Replacement("fname", Range.Offset, Range.Length, "")`
> 
> (ugh, that's really awkward. Maybe we should have a helper in TokenBuffer that combines 1-3)
> 
You have a good point that this doesn't work well with macros, but I'm not entirely sure how you think this should work.

I'd argue that code actions should refer to the thing under the cursor, not it's expansion. That would be confusing to the user, as they would not be able to understand what the action offered is, nor how it would affect other places. So in your example of 
#define NS(X) foo::X
I'd argue that we should not offer the action. 
We should, however, support something like:
EXPECT_TRUE(fo^o::bar());
In this case, the qualifier is spelled under the cursor and the fact that EXPECT_TRUE is a macro and not a function should not matter to the user.

I updated the code to support that and added tests.
We can use isMacroID() and isMacroArgExpansion() to filter out macros, except for macro arguments. Note that we can't use spelledForExpanded(), since the qualifier might not be the entire expansion of the macro, thus spelledForExpanded will return None.

Let me know if any of what I did here now doesn't make sense.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:226
+
+    if (auto Err = R.add(tooling::Replacement(SM, InsertionPoint->Loc, 0,
+                                              UsingTextStream.str()))) {
----------------
sammccall wrote:
> Again macros.
> This seems a bit more pressing, there's a nontrivial chance that you're inserting inside a namespace which is introduced by invoking a `FOO_NS_BEGIN` macro.
> 
> Unfortunately I don't see a really simple way to insert after this macro - using the expansion location fails if macro doesn't end after the `}`. (e.g. `ANON_NAMESPACE(f::X y;)`)
> (Also you'd need to take care of inserting a space after the macro invocation to avoid `FOO_NS_BEGINusing`.)
> 
> I'd suggest simply skipping over any candidate locations (namespaces or using declarations) that aren't in the main file, using the expansion location of the first-top-level-decl fallback, and asserting here that the location is in the main file. (We use raw `assert()` in LLVM)
We already skip over "using" not in main file. I added check for namespace and fixed the above-first-top-level-decl to be above getExpandedLoc() of that, which works for something like a macro declaring a namespace. It's not ideal, but I think it should be good enough, at least for now. Also added a test.

Not sure what the point of assert() here would be? Wouldn't it be far better to return an error, since it's trivial at this point and it won't crash the whole server, including the background index and such? I get that assert() can be useful in places where returning error is difficult and for things that should never, ever happen, but here seems like an overkill.

Anyway, editMainFile() already returns an error when the edit is not in the main file. IMHO that's good enough, what do you think?


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:2436
+#include "test.hpp"
+namespace {using one::two::ff;
+
----------------
sammccall wrote:
> alternately: `using ::one::two::ff`.
> Correct more often, uglier/longer, probably a style preference but not worth having an option for. Pick one :-)
The current code keeps the :: if it was there, but won't add it, so basically we respect whatever the user typed. Seems OK to me, do you agree?

I changed one test to include this aspect.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76432/new/

https://reviews.llvm.org/D76432