[PATCH] D88299: [clang-format] Add MacroUnexpander.

Mon Oct 12 11:23:53 PDT 2020

sammccall added a comment.

This is magnificently difficult :-)
I've sunk a fair few hours into understanding it now, and need to send some comments based on the interface+tests.
Some of these will be misguided or answered by the implementation, but I'm afraid I can't fit it all into my head (on a reasonable timeframe) until I understand it better.

================
Comment at: clang/lib/Format/FormatToken.h:449

+  /// When macro expansion introduces parents, those are marked as
+  /// \c MacroParent, so formatting knows their children need to be formatted.
----------------
I can't really understand from the comment when this is supposed to be set, and there are no tests of it.

(The comment is vague: is a "parent" the inverse of FormatToken::Children here? Is this scenario when the parents in question are new, or their children are new, or both? What part of the code is "formatting", and why would it otherwise skip the children?)

================
Comment at: clang/lib/Format/Macros.h:134

+/// Matches formatted lines that were created by macro expansion to a format
+/// for the original macro call.
----------------
"matches formatted lines" probably describes the hard technical problem it has to solve, but not so much what it does for the caller:  what the transformation is between its inputs and its outputs.

Is it something like:

```
Rewrites expanded code (containing tokens expanded from macros) into spelled code (containing tokens for the macro call itself). Token types are preserved, so macro arguments in the output have semantically-correct types from their expansion. This is the point of expansion/unexpansion: to allow this information to be used in formatting.
```

[Is it just tokentypes? I guess it's also Role and MustBreakBefore and some other stuff like that?]

================
Comment at: clang/lib/Format/Macros.h:142
+/// When getting the formatted lines of the expansion via the \c addLine method:
+/// -> class A {
+/// -> public:
----------------
I'm a bit confused by these arrows. It doesn't seem that they each point to an unwrappedline passed to addLine?

================
Comment at: clang/lib/Format/Macros.h:142
+/// When getting the formatted lines of the expansion via the \c addLine method:
+/// -> class A {
+/// -> public:
----------------
sammccall wrote:
> I'm a bit confused by these arrows. It doesn't seem that they each point to an unwrappedline passed to addLine?
This example didn't really help me understand the interface of this class, it seems to be a special case:
 - the input is a single block construct (rather than e.g. a whole program), but it's not clear why that's the case.
 - the output (unexpanded form) consists of exactly a macro call with no leading/trailing tokens, which isn't true in general

If the idea is to provide as input the minimal range of lines that span the macro, we should say so somewhere. But I would like to understand why we're not simply processing the whole file.

================
Comment at: clang/lib/Format/Macros.h:147
+///
+/// Creates the unwrapped lines containing the macro call tokens so that
+/// the macro call tokens fit the semantic structure of the expanded formatted
----------------
this says "creates the unwrapped lines" but getResult() returns only one.
Does the plural here refer to the tree? Maybe just use singular or "the unwrappedlinetree"?

================
Comment at: clang/lib/Format/Macros.h:154
+/// -> })
+class MacroUnexpander {
+public:
----------------
I get the symmetry between the expander/unexpander classes, but the name is making it harder for me to follow the code.
 - the extra compound+negation in the name makes it hard/slow to understand, as I'm always thinking first about expansion
 - the fact that expansion/unexpansion are not actually opposites completely breaks my intuition. It also creates two different meanings of "unexpanded" that led me down the garden path a bit (e.g. in the test).

Would you consider `MacroCollapser`? It's artificial, but expand/collapse are well-known antonyms without being the same word.

(Incidentally, I just realized this is why I find "UnwrappedLine" so slippery - the ambiguity between "this isn't wrapped" and "this was unwrapped")

================
Comment at: clang/lib/Format/Macros.h:164
+  /// For the given \p Line, match all occurences of tokens expanded from a
+  /// macro and replace them with the original macro call in \c getResult().
+  void addLine(const UnwrappedLine &Line);
----------------
I find this hard to follow.
- again, the "match" part is an implementation detail that sounds interesting but isn't :-)
- "from a macro" sounds like one in particular, but is actually every macro
- the bit about "getResult" is a separate point that feels shoehorned in

What about:
"Replaces tokens that were expanded from macros with the original macro calls. The result is stored and available in getResult()"

================
Comment at: clang/lib/Format/Macros.h:175
+  /// Generally, this line tries to have the same structure as the expanded,
+  /// formatted unwrapped lines handed in via \c addLine():
+  /// If a token in a macro argument is a child of a token in the expansion,
----------------
how can this be the case if the input can have multiple lines and the output only one?

Is the return value a synthetic parent of the translated lines?
Or is there a hidden requirement on the caller here that we don't keep feeding in lines unless we're continuing a macro call and therefore know we'll end up with one line?

This stuff could be clarified in docs but again I have to ask, can't we sidestep the whole issue by processing the whole file and returning all the lines?

(This is somewhat answered in the implementation, though that doesn't seem like the right place)

================
Comment at: clang/lib/Format/Macros.h:237
+  // lines as children.
+  // In the end, we stich the lines together so that each subsequent line
+  // is a child of the last token of the previous line. This is necessary
----------------
The explanation here seems to be proving the converse: if we *didn't* use this representation, then the code wouldn't work. However what I'm missing is an explanation of why it **is** correct/sensible.

After staring at the tests, I'm still not sure, since the tests seem to postprocess the "natural" output the same way before asserting equality.

My tentative conclusion is it would be clearest to move this "in the end" step to the *caller* of getResult(), as it seems to have more to do with formatting than unexpansion. But I haven't looked in detail at that caller, maybe I'm wrong...

================
Comment at: clang/unittests/Format/MacroUnexpanderTest.cpp:1
+#include "../../lib/Format/Macros.h"
+#include "../../lib/Format/UnwrappedLineParser.h"
----------------
All of these tests use both the expander and unexpander (so need consider behavior/bugs of both). 
Is it possible to test the unexpander in isolation?

(Context: I'm trying to use the tests to understand what the class does, but the inputs aren't visible)

================
Comment at: clang/unittests/Format/MacroUnexpanderTest.cpp:1
+#include "../../lib/Format/Macros.h"
+#include "../../lib/Format/UnwrappedLineParser.h"
----------------
sammccall wrote:
> All of these tests use both the expander and unexpander (so need consider behavior/bugs of both). 
> Is it possible to test the unexpander in isolation?
> 
> (Context: I'm trying to use the tests to understand what the class does, but the inputs aren't visible)
Having read all the tests here, they seem to follow exactly the same schema:

1. some macro definitions
2. some sequence of expand() calls, simulating expanding some macro-heavy code
3. verify the expanded token sequence, rearranging it into expected UnwrappedLine structure
4. call unexpand() and check that the result has the expected UnwrappedLine structure

You do this using various fairly-general helpers and DSLs, but don't really combine them in different ways... the tests are somewhat readable and error messages are OK, but if these are important tests, it might be worth looking at a data-driven test. e.g.

```
Case NestedChildBlocks;
NestedChildBlocks.Macros = {"ID(x)=x", "CALL(x)=f([] { x })"};
NestedChildBlocks.Original = "ID(CALL(CALL(return a * b;)))";
NestedChildBlocks.Expanded = R"cpp(
{
f([] {
  f([] {
    return a * b;
  })
})
}
)cpp"; // indentation shows structure
NestedChildBlocks.Unexpanded = R"cpp(
ID(
  {
  CALL(CALL(return a * b;))
  }
)
)cpp";
NestedChildBlocks.verify();
```

Definitely involves a bit of reinventing wheels though.

================
Comment at: clang/unittests/Format/MacroUnexpanderTest.cpp:20
+
+class Expansion {
+public:
----------------
docs for this class/major members?

================
Comment at: clang/unittests/Format/MacroUnexpanderTest.cpp:28
+    auto *ID = Lex.id(Name);
+    auto UnexpandedLine = std::make_unique<UnwrappedLine>();
+    UnexpandedLine->Tokens.push_back(ID);
----------------
this name (and all the other "Unexpanded*" in this class) is misleading, because there's a process called "unexpanding" but these aren't the output of it.

I'd suggest "spelled", though I do think renaming the unexpander would also be worthwhile.

================
Comment at: clang/unittests/Format/MacroUnexpanderTest.cpp:93
+
+struct Matcher {
+  Matcher(const TokenList &Tokens) : Tokens(Tokens), It(this->Tokens.begin()) {}
----------------
this needs docs (it's a cool technique! no need to make the reader decipher it though)

================
Comment at: clang/unittests/Format/MacroUnexpanderTest.cpp:96
+
+  Chunk consume(const TokenList &Tokens) {
+    TokenList Result;
----------------
you always consume(lex(...)) - taking a StringRef directly instead would make it clearer that the identity of the temporary tokens doesn't matter

================
Comment at: clang/unittests/Format/MacroUnexpanderTest.cpp:125
+  std::unique_ptr<MacroExpander>
+  create(const std::vector<std::string> &MacroDefinitions) {
+    return std::make_unique<MacroExpander>(MacroDefinitions,
----------------
`create()` is a strange name for the *expander* in an *unexpander* test

================
Comment at: clang/unittests/Format/MacroUnexpanderTest.cpp:141
+
+  UnwrappedLine line(llvm::ArrayRef<Chunk> Chunks) {
+    UnwrappedLine Result;
----------------
FWIW, this seems confusing to me - line() has overloads that are simple, and then this one that does the same nontrivial stitching that the production code does.
The fact that the a/b lines are not sibling sub-lines in `line({tokens("a"), tokens("b")})`but they *are* sibling tokens in  `line(lex("a b"))` is hard to keep track of in the tests.

If the stitching really is necessary, I think it's important for the expected output to also be shown in its stitched form.

================
Comment at: clang/unittests/Format/MacroUnexpanderTest.cpp:155
+
+  Chunk tokens(llvm::StringRef Text) { return Chunk(lex(Text)); }
+
----------------
why is this "tokens" and not "chunk"?

================
Comment at: clang/unittests/Format/MacroUnexpanderTest.cpp:207
+  Unexp.addLine(line(E.consume(lex("};"))));
+  EXPECT_TRUE(Unexp.finished());
+  Matcher U(Call);
----------------
you have lots of assertions that this is true, but none that it is false

================
Comment at: clang/unittests/Format/MacroUnexpanderTest.cpp:242
+  Expansion Exp1(Lex, *Macros);
+  TokenList Call1 = Exp1.expand("ID", {"a *b"});
+  // 2. expand ID({ a *b; })
----------------
the high-level point of this test seems to be that by looking the post-expansion context, we can determine that b is a declared pointer so we don't put a space before it.

And everywhere these tokens are mentioned/verified here, the spacing is correct, as if it were propagated... but of course the spacing is actually ignored everywhere and underneath it's just sequences of tokens.
This makes it hard to track how well this it testing what it really wants to test.

Is it possible to mark the asterisk with the correct tokentype at the point where formatting would occur, and then verify that the unexpanded form (i.e. `Chunk1.Tokens[1]`) still has the tokentype set?
(It's probably possible to prove this by inspection by understanding what U1 contains, how Matcher works etc, but I think it'd still be illustrative)

================
Comment at: clang/unittests/Format/MacroUnexpanderTest.cpp:245
+  TokenList Arg;
+  Arg.push_back(Lex.id("{"));
+  Arg.append(Exp1.getTokens().begin(), Exp1.getTokens().end());
----------------
what does id() mean if not identifier?

================
Comment at: clang/unittests/Format/MacroUnexpanderTest.cpp:406
+  Matcher E(Exp.getTokens());
+  Unexp.addLine(line(E.consume(lex("x;"))));
+  Unexp.addLine(line(E.consume(lex("x y"))));
----------------
because this example isn't realistic, it'd be nice to have the comment here showing what formatting you're simulating

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88299