[clang-query] Add non-exclusive output API

[steveire]

Summary

Add granular options for AST dumping, text printing and diagnostics.

This makes it possible to

• Have both diag and dump active at once
• Extend the output with other queryable content in the future.

[steveire]

Format

[steveire]

Commit message

[steveire]

clang-query/Query.cpp

48 ↗

(On Diff #168218)

Is the 'print' output useful at all? I'm thinking the updated API shouldn't support it at all. Thoughts?

[steveire]

Can I convince someone to be interested enough in this to approve it?

I have a talk coming up and I'd like to be able to say that you don't have to switch between dump mode and diag mode all the time.

I also want to enable other relevant outputting features in clang-query that I have in the pipeline.

[aaron.ballman]

I'm not all that keen on the idea of deprecating set output like this -- that command has been around since forever, and scripts outside of our control are likely to rely on it. However, it seems like you should be able to make set output accept a comma-delimited list of options. e.g., set output dump, diag This would give you the same level of granularity while still being backwards compatible. Do you think that would work for your needs?

[steveire]

• The scripts will continue to work at least until set output is removed, which is not going to happen soon.
• A comma-delimited list of options means that if I have foo, bar, bat enabled and want to add bang, I need to set output foo, bar, bat, bang. Or alternatively if I want to remove bat, I need to write out all the others. I don't think that's suitable.

[aaron.ballman]

In D52857#1258893, @steveire wrote:

• The scripts will continue to work at least until set output is removed, which is not going to happen soon.

Certainly, but given that deprecation implies eventual removal, people are still being asked to update their scripts. What's more, given that clang-output has no real documentation to speak of, how will users even *know* to update their scripts? Rather than invent ways to address these concerns, I'm much more in favor of making the feature work using a backwards compatible syntax. Then no one has to update their scripts to get identical functionality.

• A comma-delimited list of options means that if I have foo, bar, bat enabled and want to add bang, I need to set output foo, bar, bat, bang. Or alternatively if I want to remove bat, I need to write out all the others. I don't think that's suitable.

Correct -- if you want different output because new options are available, you need to opt into it. I'm not certain what the issue is with that; can you expound on your concerns? From what I'm seeing in the patch, it looks like you want user to write:

set dump-output
set diag-output
match foo(bar(baz()))

and I'm asking for it to instead be:

set output dump, diag
match foo(bar(baz()))

Functionally, I believe these are equivalent. However, I think the latter is more clear for users as it implies a set of output options rather than leaving you to wonder whether set blah-output options are mutually exclusive or not.

[steveire]

What's more, given that clang-output has no real documentation to speak of, how will users even *know* to update their scripts?

The release notes will tell them.

But your comment also raises a different point: If there is so little clang-query documentation, what justification is there for relying on current behavior?

Also, what scripts are you referring to that rely on clang-query? Do we know of any that are expected to work long-term without maintenance (unlikely, as the AST itself is not stable), or is this concern imagined?

can you expound on your concerns?

After this patch, the user writes

set dump-output true

or false.

and after the follow-up (https://reviews.llvm.org/D52859) they write

set matcher-output true

After more features I have in the pipeline they will write other similar commands.

With my command-design, if a user wants to enable dump output in addition to what they have, they just

set dump-output true

In your design, they have to recall/find out what options are currently enabled, and add them in a comma separated list. That is not user-friendly.

wonder whether set blah-output options are mutually exclusive or not.

These options are obviously and clearly toggles. There is definitely no reason to think that they are mutually exclusive any more than there is reason to think set output dump is mutually exclusive with set bind-root false.

And even if someone did wonder that, they would just try it and learn that they are toggles.

[aaron.ballman]

In D52857#1259522, @steveire wrote:

What's more, given that clang-output has no real documentation to speak of, how will users even *know* to update their scripts?

The release notes will tell them.

That's not user friendly.

But your comment also raises a different point: If there is so little clang-query documentation, what justification is there for relying on current behavior?

That's pretty much the only reason I think it's reasonable to consider deprecating the command in the first place. The justification for relying on it is: this has been the behavior since Day 1 of clang-query and we do not often remove user-facing options without there being a Very Good Reason to do so. This is not a situation where it's an undocumented feature that's in-flux or there's no indication it's being used.

Also, what scripts are you referring to that rely on clang-query? Do we know of any that are expected to work long-term without maintenance (unlikely, as the AST itself is not stable), or is this concern imagined?

I'm referring to third-party scripts kept by people out of tree. I have several, I know a few other folks internally at my company who do as well.

I think it's reasonable to expect differences in AST output when updating clang-query, but this is not that. It's reasonable to expect command stability.

can you expound on your concerns?

After this patch, the user writes

set dump-output true

or false.

and after the follow-up (https://reviews.llvm.org/D52859) they write

set matcher-output true

After more features I have in the pipeline they will write other similar commands.

With my command-design, if a user wants to enable dump output in addition to what they have, they just

set dump-output true

In your design, they have to recall/find out what options are currently enabled, and add them in a comma separated list. That is not user-friendly.

Yes -- you are required to make a decision that you want to opt in to new functionality. The exact same thing is true with your proposal -- you have to find the right place to stick the set dump-output true in order to enable it.

wonder whether set blah-output options are mutually exclusive or not.

These options are obviously and clearly toggles.

Your notions of obvious and clear do not match mine.

There is definitely no reason to think that they are mutually exclusive any more than there is reason to think set output dump is mutually exclusive with set bind-root false.

Well, except for the fact that each of these commands says "output" in the name and they are replacing a command argument named "output" that was mutually exclusive and there's zero documentation explaining the commands.

And even if someone did wonder that, they would just try it and learn that they are toggles.

I don't see this functionality being so critical that we need to deprecate the existing spelling when there are backwards compatible options available, which is why I'm opposed to this patch going in with the proposed syntax.

[steveire]

you have to find the right place to stick the set dump-output true in order to enable it.

What do you mean "the right place"?

[aaron.ballman]

In D52857#1260455, @steveire wrote:

you have to find the right place to stick the set dump-output true in order to enable it.

What do you mean "the right place"?

The point from which they want to enable dump outputting.

[steveire]

I don't see this functionality being so critical that we need to deprecate the existing spelling when there are backwards compatible options available, which is why I'm opposed to this patch going in with the proposed syntax.

I don't think we're going to go anywhere except around in circles :). I don't see your comma separated syntax as viable. Maybe there is a third way that both of us don't see.

I tried to catch you on IRC without success. Thanks for the reviews! I appreciate your time. I'll see if I can find a different reviewer for this in a while.

Thanks,

[aaron.ballman]

@steveire and I chatted over IRC and I wanted to capture another alternate proposal idea as part of the review (we did not reach agreement on this proposal, however). I've provided it side-by-side with the other proposals for comparison purposes.

# Original proposal
set dump-output true
match foo(bar(baz())) # Uses one output format
match bar(foo()) # Uses one output format
set diag-output true
set matcher-output true
match foo(bar(baz())) # Uses all three output formats
match bar(foo()) # Uses all three output formats
set diag-output false
set dump-output false
match foo(bar(baz())) # Uses only matcher output
 
# First Alternative
set output dump
match foo(bar(baz())) # Uses one output format
match bar(foo()) # Uses one output format
set output dump, diag, matcher
match foo(bar(baz())) # Uses all three output formats
match bar(foo()) # Uses all three output formats
set output matcher
match foo(bar(baz())) # Uses only matcher output
 
# Second Alternative
set output dump
match foo(bar(baz())) # Uses one output format
match bar(foo()) # Uses one output format
set output diag toggle on
set output matcher toggle on
match foo(bar(baz())) # Uses all three output formats
match bar(foo()) # Uses all three output formats
set output matcher
match foo(bar(baz())) # Uses only matcher output

[steveire]

Rename dump-output to ast-output.

[steveire]

Fix tests

[steveire]

Fix test

[steveire]

Update test

[steveire]

Perhaps the best solution is to introduce this new API, but not deprecate the existing 'exclusive' API.

What do you think?

Then the only thing I would have to do with this patch is restore the documentation of 'set output foo'

[steveire]

Don't deprecate existing API

[steveire]

New design

[steveire]

Update docs

[pcc]

I'm not really involved in clang-query development these days, so this is more of a drive-by comment. It wasn't really obvious to me what the wording of the help text for set enable-output meant:

Set whether to enable <feature> content non-exclusively.

I figured out from the name of the option and from reading the source code that it just means "Enable output in format <feature>." This seems confusing to me because I would expect set foo bar to set the value of a variable foo to bar, which doesn't match what this command does. What do you think about spelling it as something like enable output <feature>?

[steveire]

This seems confusing to me because I would expect set foo bar to set the value of a variable foo to bar,

This is already not the case with set output diag etc.

That said, I don't mind spelling this <enable|disable> output foo. It's not my decision. It's up to the reviewers.

[steveire]

This is already not the case with set output diag etc.

Oh, I'm wrong here. I misread your post, sorry.

[steveire]

I prefer the API from Peter. I think it's a good additional step from where Aaron and I reached in IRC discussion (this patch currently).

I can change the patch to use that later if you agree Aaron?

[aaron.ballman]

In D52857#1272276, @steveire wrote:

I prefer the API from Peter. I think it's a good additional step from where Aaron and I reached in IRC discussion (this patch currently).

I can change the patch to use that later if you agree Aaron?

To be extra clear, we're talking about changing "set enable-output detailed-ast" to be "enable output detailed-ast" instead? If so, I'm okay with that -- it seems like an improvement.

[steveire]

Yep, that's the suggestion. That will result in commands such as:

• enable output detailed-ast
• disable output detailed-ast
• set output detailed-ast
• enable output diag
• disable output diag
• set output diag

etc, which I think addresses all concerns.

[aaron.ballman]

In D52857#1272420, @steveire wrote:

Yep, that's the suggestion. That will result in commands such as:

• enable output detailed-ast
• disable output detailed-ast
• set output detailed-ast
• enable output diag
• disable output diag
• set output diag

etc, which I think addresses all concerns.

Agreed -- I think this is the right path forward.

[steveire]

New command design

[aaron.ballman]

LGTM with some minor fixes.

At some point (not necessarily as part of this patch), you should also update docs\ReleaseNotes.rst to broadly list the new features you've been adding to clang-query.

clang-query/QueryParser.cpp

292

This seems incorrect to me; we expect the command identifier output here, don't we?

298–304

Elide braces here.

[steveire]

clang-query/QueryParser.cpp

292

Actually this is the case where the user writes enable with no more output.

I match set command behavior and output here.

I added a test for this, similar to the existing equivalent set test.

clang-query/Query.h

bool run(llvm::raw_ostream &OS, QuerySession &QS) const override {
     return true;
   }
 
   static bool classof(const Query *Q) { return Q->Kind == QK_SetOutputKind; }
 
   bool QuerySession::*Var;
 };
 
+// Implements the non-exclusive 'set output dump|diag|print' options
+struct SetNonExclusiveOutputQuery : Query {
+  SetNonExclusiveOutputQuery(bool QuerySession::*Var, bool Value) : Query(QK_SetOutputKind), Var(Var), Value(Value) {}
+  bool run(llvm::raw_ostream &OS, QuerySession &QS) const override {
+    QS.*Var = Value;
+    return true;
+  }
+
+  static bool classof(const Query *Q) { return Q->Kind == QK_SetOutputKind; }
+
+  bool QuerySession::*Var;
+  bool Value;
+};
+
+struct EnableOutputQuery : SetNonExclusiveOutputQuery
+{
+  EnableOutputQuery(bool QuerySession::*Var) : SetNonExclusiveOutputQuery(Var, true) { }
+};
+
+struct DisableOutputQuery : SetNonExclusiveOutputQuery
+{
+  DisableOutputQuery(bool QuerySession::*Var) : SetNonExclusiveOutputQuery(Var, false) { }
+};
+
 } // namespace query
 } // namespace clang
 
 #endif

clang-query/QueryParser.h

private:
   QueryParser(StringRef Line, const QuerySession &QS)
       : Begin(Line.begin()), End(Line.end()), CompletionPos(nullptr), QS(QS) {}
 
   StringRef lexWord();
 
   template <typename T> struct LexOrCompleteWord;
 
   QueryRef parseSetBool(bool QuerySession::*Var);
+  template<typename QueryType>
   QueryRef parseSetOutputKind();
   QueryRef completeMatcherExpression();
 
   QueryRef endQuery(QueryRef Q);
 
   /// \brief Parse [\p Begin,\p End).
   ///
   /// \return A reference to the parsed query object, which may be an

clang-query/QueryParser.cpp

unsigned Value = LexOrCompleteWord<unsigned>(this, ValStr)
                        .Case("true", 1)
                        .Default(~0u);
   if (Value == ~0u) {
     return new InvalidQuery("expected 'true' or 'false', got '" + ValStr + "'");
   }
   return new SetQuery<bool>(Var, Value);
 }
 
+template<typename QueryType>
 QueryRef QueryParser::parseSetOutputKind() {
   StringRef ValStr;
   unsigned OutKind = LexOrCompleteWord<unsigned>(this, ValStr)
                          .Case("diag", OK_Diag)
                          .Case("print", OK_Print)
                          .Case("detailed-ast", OK_DetailedAST)
                          .Case("dump", OK_DetailedAST)
                          .Default(~0u);
   if (OutKind == ~0u) {
     return new InvalidQuery(
         "expected 'diag', 'print', 'detailed-ast' or 'dump', got '" + ValStr +
         "'");
   }
 
   switch (OutKind) {
   case OK_DetailedAST:
-    return new SetExclusiveOutputQuery(&QuerySession::DetailedASTOutput);
+    return new QueryType(&QuerySession::DetailedASTOutput);
   case OK_Diag:
-    return new SetExclusiveOutputQuery(&QuerySession::DiagOutput);
+    return new QueryType(&QuerySession::DiagOutput);
   case OK_Print:
-    return new SetExclusiveOutputQuery(&QuerySession::PrintOutput);
+    return new QueryType(&QuerySession::PrintOutput);
   }
 
   llvm_unreachable("Invalid output kind");
 }
 
 QueryRef QueryParser::endQuery(QueryRef Q) {
   const char *Extra = Begin;
   if (!lexWord().empty())
enum ParsedQueryKind {
   PQK_Set,
   PQK_Unlet,
   PQK_Quit
 };
 
 enum ParsedQueryVariable {
   PQV_Invalid,
   PQV_Output,
+  PQV_EnableOutput,
+  PQV_DisableOutput,
   PQV_BindRoot,
   PQV_PrintMatcher
 };
 
 QueryRef makeInvalidQueryFromDiagnostics(const Diagnostics &Diag) {
   std::string ErrStr;
   llvm::raw_string_ostream OS(ErrStr);
   Diag.printToStreamFull(OS);
case PQK_Match: {
     return new MatchQuery(MatcherSource, *Matcher);
   }
 
   case PQK_Set: {
     StringRef VarStr;
     ParsedQueryVariable Var =
         LexOrCompleteWord<ParsedQueryVariable>(this, VarStr)
             .Case("output", PQV_Output)
+            .Case("enable-output", PQV_EnableOutput)
+            .Case("disable-output", PQV_DisableOutput)
             .Case("bind-root", PQV_BindRoot)
             .Case("print-matcher", PQV_PrintMatcher)
             .Default(PQV_Invalid);
     if (VarStr.empty())
       return new InvalidQuery("expected variable name");
     if (Var == PQV_Invalid)
       return new InvalidQuery("unknown variable: '" + VarStr + "'");
 
     QueryRef Q;
     switch (Var) {
     case PQV_Output:
-      Q = parseSetOutputKind();
+      Q = parseSetOutputKind<SetExclusiveOutputQuery>();
+      break;
+    case PQV_EnableOutput:
+      Q = parseSetOutputKind<EnableOutputQuery>();
+      break;
+    case PQV_DisableOutput:
+      Q = parseSetOutputKind<DisableOutputQuery>();
       break;
     case PQV_BindRoot:
       Q = parseSetBool(&QuerySession::BindRoot);
       break;
     case PQV_PrintMatcher:
       Q = parseSetBool(&QuerySession::PrintMatcher);
       break;
     case PQV_Invalid:
       llvm_unreachable("Invalid query kind");
     }
 
     return endQuery(Q);
   }
 
   case PQK_Unlet: {
     StringRef Name = lexWord();
 
     if (Name.empty())
       return new InvalidQuery("expected variable name");
 
     return endQuery(new LetQuery(Name, VariantValue()));
   }
 
   case PQK_Invalid:
     return new InvalidQuery("unknown command: " + CommandStr);
   }
 
   llvm_unreachable("Invalid query kind");
 }
 
 QueryRef QueryParser::parse(StringRef Line, const QuerySession &QS) {
   return QueryParser(Line, QS).doParse();
 }
 
 std::vector<LineEditor::Completion>
 QueryParser::complete(StringRef Line, size_t Pos, const QuerySession &QS) {
   QueryParser P(Line, QS);
   P.CompletionPos = Line.data() + Pos;
 
   P.doParse();
   return P.Completions;
 }
 
 } // namespace query
 } // namespace clang

unittests/clang-query/QueryEngineTest.cpp

TEST_F(QueryEngineTest, Basic) {
   EXPECT_TRUE(
       SetExclusiveOutputQuery(&QuerySession::DetailedASTOutput).run(OS, S));
   EXPECT_TRUE(MatchQuery(FooMatcherString, FooMatcher).run(OS, S));
 
   EXPECT_TRUE(OS.str().find("FunctionDecl") != std::string::npos);
 
   Str.clear();
 
+  EXPECT_TRUE(
+      EnableOutputQuery(&QuerySession::DiagOutput).run(OS, S));
+  EXPECT_TRUE(
+      EnableOutputQuery(&QuerySession::DetailedASTOutput).run(OS, S));
+  EXPECT_TRUE(MatchQuery(FooMatcherString, FooMatcher).run(OS, S));
+
+  {
+    auto Output = OS.str();
+    EXPECT_TRUE(Output.find("FunctionDecl") != std::string::npos);
+    EXPECT_TRUE(Output.find("foo.cc:1:1: note: \"root\" binds here") !=
+                std::string::npos);
+  }
+
+  Str.clear();
+
   EXPECT_TRUE(SetQuery<bool>(&QuerySession::BindRoot, false).run(OS, S));
   EXPECT_TRUE(MatchQuery(FooMatcherString, FooMatcher).run(OS, S));
 
   EXPECT_TRUE(OS.str().find("No bindings.") != std::string::npos);
 
   Str.clear();
 
   EXPECT_FALSE(MatchQuery("isMain()", isMain()).run(OS, S));

unittests/clang-query/QueryParserTest.cpp

TEST_F(QueryParserTest, Set) {
   Q = parse("set output dump");
   ASSERT_TRUE(isa<SetExclusiveOutputQuery >(Q));
   EXPECT_EQ(&QuerySession::DetailedASTOutput, cast<SetExclusiveOutputQuery>(Q)->Var);
 
   Q = parse("set output detailed-ast");
   ASSERT_TRUE(isa<SetExclusiveOutputQuery>(Q));
   EXPECT_EQ(&QuerySession::DetailedASTOutput, cast<SetExclusiveOutputQuery>(Q)->Var);
 
+  Q = parse("set enable-output detailed-ast");
+  ASSERT_TRUE(isa<EnableOutputQuery>(Q));
+  EXPECT_EQ(&QuerySession::DetailedASTOutput, cast<EnableOutputQuery>(Q)->Var);
+
+  Q = parse("set disable-output detailed-ast");
+  ASSERT_TRUE(isa<DisableOutputQuery>(Q));
+  EXPECT_EQ(&QuerySession::DetailedASTOutput, cast<DisableOutputQuery>(Q)->Var);
+
   Q = parse("set bind-root foo");
   ASSERT_TRUE(isa<InvalidQuery>(Q));
   EXPECT_EQ("expected 'true' or 'false', got 'foo'",
             cast<InvalidQuery>(Q)->ErrStr);
 
   Q = parse("set bind-root true");
   ASSERT_TRUE(isa<SetQuery<bool> >(Q));
   EXPECT_EQ(&QuerySession::BindRoot, cast<SetQuery<bool> >(Q)->Var);