[clangd] Disable expand-auto action on constrained auto.

[sammccall]

I'm missing some context on this patch. My intuition is that constrained auto is unlikely to be used in deducible contexts, but maybe some people will like Iterator<int> auto I = foo.begin() or so...

clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp

99

Why? Is there a mechanical limitation (bad typeloc or so) or is this a policy decision?

If it's technical, we should link to a bug

clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp

88

This shouldn't be unavailable because it's constrained, it should be unavailable because it's not deduced.

Does this case pass or fail today?

If the issue with constrained auto is syntactic, can you add a deductible test case?

[hokein]

clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp

88

it should be unavailable because it's not deduced.

This is ideal, but not the behavior today -- the tweak is available but being failed to apply as we don't check the auto is deduced during prepare stage (it is expensive, requiring an AST traversal).

For deducible cases, we replace the C auto with the actual type, e.g.

template <typename T> concept C = true;
C auto var = 123;  // => int var = 123;

I don't think this is an expected behavior. Given the deductible & nondeductible cases, it seems like an improvement to disable the tweak for constrained auto.

[sammccall]

clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp

88

it should be unavailable because it's not deduced.

This is ideal, but not the behavior today -- the tweak is available but being failed to apply as we don't check the auto is deduced during prepare stage (it is expensive, requiring an AST traversal).

Sure, but this doesn't have anything to do with it being constrained.
We offer the tweak on auto x(); too, and equally shouldn't.

Meanwhile we offer the tweak on auto x() { return 42; } and it works correctly, and we should also offer it on Integer auto x() { return 42; }.

For deducible cases, we replace the C auto with the actual type, e.g.

template <typename T> concept C = true;
C auto var = 123;  // => int var = 123;

I don't think this is an expected behavior.

That's exactly what I'd expect this action to do. What would you expect it to do?

[nridge]

In D117463#3247915, @sammccall wrote:

My intuition is that constrained auto is unlikely to be used in deducible contexts, but maybe some people will like Iterator<int> auto I = foo.begin() or so...

I expect this to be fairly common. For example, if you look at a recent standards proposal like std::execution which assumes C++20 as a baseline, and look at an end-user code example like this one, it uses things like scheduler auto x = ... and sender auto y = ... pretty liberally.

I don't have a strong opinion on the policy question of whether replacing a constrained auto type with a concrete type is likely to be a useful refactoring. The use case of "I want the type name to be more descriptive" is less compelling since the concept name often provides an appropriate amount of description. But there may still be cases where the concrete type is important and preferable to use.

clang-tools-extra/clangd/refactor/tweaks/ExpandAutoType.cpp

bool isTemplateParam(const SelectionTree::Node *Node) {
   return false;
 }
 
 bool ExpandAutoType::prepare(const Selection& Inputs) {
   CachedLocation = llvm::None;
   if (auto *Node = Inputs.ASTSelection.commonAncestor()) {
     if (auto *TypeNode = Node->ASTNode.get<TypeLoc>()) {
       if (const AutoTypeLoc Result = TypeNode->getAs<AutoTypeLoc>()) {
-        if (!isStructuredBindingType(Node) &&
+        if (!isStructuredBindingType(Node) && !Result.isConstrained() &&
             !isDeducedAsLambda(Node, Result.getBeginLoc()) &&
             !isTemplateParam(Node))
           CachedLocation = Result;
       }
     }
   }
 
   return (bool) CachedLocation;

clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp

TEST_F(ExpandAutoTypeTest, Test) {
   EXPECT_UNAVAILABLE("const ^auto &[x,y] = (int[]){1,2};");
 
   // unknown types in a template should not be replaced
   EXPECT_THAT(apply("template <typename T> void x() { ^auto y = T::z(); }"),
               StartsWith("fail: Could not deduce type for 'auto' type"));
 
   ExtraArgs.push_back("-std=c++17");
   EXPECT_UNAVAILABLE("template <au^to X> class Y;");
+  ExtraArgs.back() = "-std=c++20";
+  EXPECT_UNAVAILABLE(
+      R"cpp(template <typename T> concept C = true;
+^C a^uto abc();
+    )cpp");
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang