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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG, thanks! Let me know if/when I should land this for you.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:105
+// argument.
+bool isInMacroNotArg(const SourceManager &SM, const SourceLocation Loc) {
+  return Loc.isMacroID() && !SM.isMacroArgExpansion(Loc);
----------------
This is just SourceManager::isMacroBodyExpansion(), I think.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:163
+  if (isInMacroNotArg(SM, QualifierToRemove.getBeginLoc()) ||
+      isInMacroNotArg(SM, QualifierToRemove.getEndLoc())) {
+    return false;
----------------
I think the endloc check should rather be SM.isWrittenInSameFile - it's no good if e.g. they're both macro arg expansions, but different ones.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:171
+// The insertion point might be a little awkward if the decl we're anchoring to
+// has a comment in an unfortunate place (e.g. directly above using, or
+// immediately following "namespace {". We should add some helpers for dealing
----------------
nit: you've mentioned the two cases I don't think we should bother fixing, but not the crucial one: we're anchoring to a regular decl, and we're going to insert in between the decl and its doc comment.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:245
+  auto &SM = Inputs.AST->getSourceManager();
+  auto TB = Inputs.AST->getTokens();
+  // Determine the length of the qualifier under the cursor, then remove it.
----------------
yikes, this copies the tokenbuffer, I didn't think that was even possible!
auto&


================
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);
----------------
adamcz wrote:
> 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.
Oops, right. I think since this is the complicated part and it has a fairly simple interface, making its inputs/outputs explicit is nice hygiene.

(prepare and apply do communicate through class state, which feels a bit hacky to me. We didn't come up with anything nicer but still flexible+simple when designing this. It is conventional and documented at least)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:71
+      return true;
+    }
+    if (D->getDeclContext()->Encloses(SelectionDeclContext)) {
----------------
adamcz wrote:
> 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
There's no hard rule, but we do use that style consistently and consistency has value too.

The clang-tidy check `readability-misleading-indentation` catches that bug. Can I suggest:
 - (for us) adding it to `.clang-tidy` in `llvm-project`? This will affect clangd and the phabricator linter
 - (for the world) we could start a list of on-by-default clang-tidy checks for clangd users with no `.clang-tidy` config, and include this check


================
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
----------------
adamcz wrote:
> 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.
> foo::bar::c^c::st *p;

But this isn't a case we support, right? We only support extraction when the NNS consists entirely of namespaces, and such NNSes don't have any children apart from the qualifier NNS.


================
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()), "",
----------------
adamcz wrote:
> 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.
> 
> in your example of #define NS(X) foo::X I'd argue that we should not offer the action.

Indeed, sorry I didn't spell that out - I was highlighting it because the original code silently did something (bad) in this case.

> can't use spelledForExpanded(), since the qualifier might not be the entire expansion of the macro

Ah, D75446 hasn't landed yet. @kadircet, want to land this soon? :-)




================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:226
+
+    if (auto Err = R.add(tooling::Replacement(SM, InsertionPoint->Loc, 0,
+                                              UsingTextStream.str()))) {
----------------
adamcz wrote:
> 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?
Behaviour looks good.

> Not sure what the point of assert() here would be?
This is a consistency check - we're relying on an invariant we think we've established elsewhere. If this goes wrong, it's a programmer error, and for better or worse, LLVM defines those as asserts.

Practical benefits:
 - developers are more likely to notice when these are violated (assuming an -UNDEBUG build)
 - we get a stacktrace, which is often useful (not really here). Returning an error doesn't give us that.
 - we don't pay for the check in release builds
 - it documents the distinction between invariant-breaking bugs and dynamically-possible error conditions
 - consistency (in principle we could change all our err-handling, but we can't change this in clang)


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:2436
+#include "test.hpp"
+namespace {using one::two::ff;
+
----------------
adamcz wrote:
> 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.
Actually yeah, that's nice and neatly avoids having to make and defend a policy :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76432