Rework ASTDumper #68438

beccadax · 2023-09-11T19:00:19Z

This PR refactors the ASTDumper to make it more structured, less mistake-prone, and more amenable to future changes. For example:

  // Before:
  void visitUnresolvedDotExpr(UnresolvedDotExpr *E) {
    printCommon(E, "unresolved_dot_expr")
      << " field '" << E->getName() << "'";
    PrintWithColorRAII(OS, ExprModifierColor)
      << " function_ref=" << getFunctionRefKindStr(E->getFunctionRefKind());
    if (E->getBase()) {
      OS << '\n';
      printRec(E->getBase());
    }
    PrintWithColorRAII(OS, ParenthesisColor) << ')';
  }

  // After:
  void visitUnresolvedDotExpr(UnresolvedDotExpr *E, StringRef label) {
    printCommon(E, "unresolved_dot_expr", label);

    printFieldQuoted(E->getName(), "field");
    printField(E->getFunctionRefKind(), "function_ref", ExprModifierColor);

    if (E->getBase()) {
      printRec(E->getBase());
    }

    printFoot();
  }

Values are printed through calls to base class methods, rather than direct access to the underlying raw_ostream.
- These methods tend to reduce the chances of bugs like missing/extra spaces or newlines, too much/too little indentation, etc.
- More values are quoted, and unprintable/non-ASCII characters in quoted values are escaped before printing.
Infrastructure to label child nodes now exists.
- Some weird breaks from the normal "style", like PatternBindingDecl's original and processed initializers, have been brought into line.
Some types that previously used ad-hoc dumping functions, like conformances and substitution maps, are now structured similarly to the dumper classes.
I've fixed the odd dumping bug along the way. For example, distributed actors were only marked actor, not distributed actor.

This PR doesn't change the overall style of AST dumps; they're still pseudo-S-expressions. But the logic that implements this style is now isolated into a relatively small base class, making it feasible to introduce e.g. JSON dumping in the future.

beccadax · 2023-09-11T19:02:14Z

@swift-ci please smoke test

test/expr/capture/generic_params.swift

beccadax · 2023-09-11T19:41:30Z

Changes:

Corrected some commit messages
Fixed one comment change where an apostrophe was incorrectly changed to a double-quote

ASTDumper now uses new printHead() and printFoot() helpers for this.

This does an incredible amount of work to clean up the dumper.

Also changes the signature to be more compatibile with other printRec() overloads.

New helper for printing a name-value pair.

Formalizes the printing of unlabeled flags in a node. Also changes the spelling of a few flags, and corrects a bug where distributed actors were always just flagged as `actor`.

Attempts to remove direct access to PrintBase::OS from its subclasses, so that all printing is mediated by the print*() methods.

Only PrintBase can access the raw_ostream and indentation directly, so all direct I/O is now isolated to that class. I think this will help us to eventually support multiple output formats (e.g. JSON).

printField and several related helpers put the key before the value, while printRec and the visit methods always put the value before the key. Standardize on the latter since we can’t change the visit methods.

Allows us to more easily audit these later for e.g. JSON support.

Except for simple keyword fields and a few other things like capture lists, we now put values in double quotes. We are also correctly escaping these values using functionality built into llvm::raw_ostream. As part of this change, printField()’s value must now be something that can be handled by a getDumpString() overload. Arbitrary strings should go through printFieldQuoted() or, in a pinch, printFieldRaw().

Make dumps for Requirement, ProtocolConformance, SubstitionMap, etc. use PrintBase and its helpers.

beccadax · 2023-09-11T19:48:52Z

More commit message corrections

beccadax · 2023-09-11T19:51:52Z

The test jobs from before I started force-pushing are:

Updates SILOptimizer/sil_combine_concrete_existential.sil to match updated conformance dumping.

Fixes Concurrency/where_clause_main_resolution.swift.

beccadax · 2023-09-11T21:36:01Z

@swift-ci please test

ktoso

AFAICS nice improvements 👍

This PR refactors the ASTDumper to make it more structured, less mistake-prone, and more amenable to future changes. For example: ```cpp // Before: void visitUnresolvedDotExpr(UnresolvedDotExpr *E) { printCommon(E, "unresolved_dot_expr") << " field '" << E->getName() << "'"; PrintWithColorRAII(OS, ExprModifierColor) << " function_ref=" << getFunctionRefKindStr(E->getFunctionRefKind()); if (E->getBase()) { OS << '\n'; printRec(E->getBase()); } PrintWithColorRAII(OS, ParenthesisColor) << ')'; } // After: void visitUnresolvedDotExpr(UnresolvedDotExpr *E, StringRef label) { printCommon(E, "unresolved_dot_expr", label); printFieldQuoted(E->getName(), "field"); printField(E->getFunctionRefKind(), "function_ref", ExprModifierColor); if (E->getBase()) { printRec(E->getBase()); } printFoot(); } ``` * Values are printed through calls to base class methods, rather than direct access to the underlying `raw_ostream`. * These methods tend to reduce the chances of bugs like missing/extra spaces or newlines, too much/too little indentation, etc. * More values are quoted, and unprintable/non-ASCII characters in quoted values are escaped before printing. * Infrastructure to label child nodes now exists. * Some weird breaks from the normal "style", like `PatternBindingDecl`'s original and processed initializers, have been brought into line. * Some types that previously used ad-hoc dumping functions, like conformances and substitution maps, are now structured similarly to the dumper classes. * I've fixed the odd dumping bug along the way. For example, distributed actors were only marked `actor`, not `distributed actor`. This PR doesn't change the overall style of AST dumps; they're still pseudo-S-expressions. But the logic that implements this style is now isolated into a relatively small base class, making it feasible to introduce e.g. JSON dumping in the future.

AnthonyLatsis · 2024-01-11T18:55:07Z

lib/AST/ASTDumper.cpp

+
+private:
+  virtual void write_impl(const char *Ptr, size_t Size) override {
+    base_os.write_escaped(StringRef(Ptr, Size), /*UseHexEscapes=*/true);


FYI, this made the tau in generic parameter names print as escape sequences:

(primary_archetype_type address=true name="\xCF\x84_0_0" (interface_type=generic_type_param_type depth=0 index=0))

True. Using write_escaped happened to be very convenient, but I wouldn't be averse to a patch that excepted the tau character, or some larger set of known-printable characters (as long as it'll still escape various newlines, quotes, control characters, etc.).

Context: swiftlang#68438 (comment)

beccadax added 3 commits September 7, 2023 17:21

[ASTDumper][NFC] Standardize on getDumpString()

9a5b754

[ASTDumper] More getDumpString adoption

f58e13f

[ASTDumper] Extract base class for dump visitors

6df4273

beccadax requested review from hborla, slavapestov, xedin, AnthonyLatsis, ahoppen, bnbarham, CodaFi, DougGregor, rintaro, ktoso and kavon as code owners September 11, 2023 19:00

bnbarham reviewed Sep 11, 2023

View reviewed changes

test/expr/capture/generic_params.swift Outdated Show resolved Hide resolved

beccadax force-pushed the a-trip-to-the-dump branch from 57a652d to 6efb4e3 Compare September 11, 2023 19:39

beccadax added 12 commits September 11, 2023 12:45

[ASTDumper][NFC] Standardize paren printing

0e654dc

ASTDumper now uses new printHead() and printFoot() helpers for this.

[ASTDumper][NFC] Make printRec() print newlines

11b04f7

This does an incredible amount of work to clean up the dumper.

[ASTDumper] Move Type printRec() into PrintBase

58d4b68

Also changes the signature to be more compatibile with other printRec() overloads.

[ASTDumper][NFC] Thread labels through visitors

e2f4c46

[ASTDumper] Adopt labels in dumps

4efcd6a

[ASTDumper][NFC] Hoist and add some print helpers

021d139

[ASTDumper][NFC] Refactor arg/param list handling

db47ef9

[ASTDumper][NFC] Add helper for indentation

4628ff7

[ASTDumper] Adopt printField

c1e8569

New helper for printing a name-value pair.

[ASTDumper][NFC] Add helpers for source locations

ab25854

[ASTDumper] Introduce and adopt printFlag()

7670ba4

Formalizes the printing of unlabeled flags in a node. Also changes the spelling of a few flags, and corrects a bug where distributed actors were always just flagged as `actor`.

[ASTDumper] Improve helpers for printing names

d9cb355

beccadax added 8 commits September 11, 2023 12:47

[ASTDumper] Remove arbitrary I/O from subclasses

a96995c

Attempts to remove direct access to PrintBase::OS from its subclasses, so that all printing is mediated by the print*() methods.

[ASTDumper][NFC] Prevent direct printing

c81482b

Only PrintBase can access the raw_ostream and indentation directly, so all direct I/O is now isolated to that class. I think this will help us to eventually support multiple output formats (e.g. JSON).

[ASTDumper][NFC] Standardize on (value, key) args

4931a8f

printField and several related helpers put the key before the value, while printRec and the visit methods always put the value before the key. Standardize on the latter since we can’t change the visit methods.

[ASTDumper][NFC] Omit most blank names

918f74b

Allows us to more easily audit these later for e.g. JSON support.

[ASTDumper] Use PrintBase for conformance dumps

af3584f

[ASTDumper] Update requirement/conformance dumps

be6e07b

Make dumps for Requirement, ProtocolConformance, SubstitionMap, etc. use PrintBase and its helpers.

[ASTDumper][NFC] Formatting cleanup, doc comments

9193489

beccadax force-pushed the a-trip-to-the-dump branch from 6efb4e3 to 9193489 Compare September 11, 2023 19:48

beccadax added 2 commits September 11, 2023 14:16

[ASTDumper] Fix broken test

a6005bb

Updates SILOptimizer/sil_combine_concrete_existential.sil to match updated conformance dumping.

[ASTDumper] Escape paths consistently on Windows

d8faa01

Fixes Concurrency/where_clause_main_resolution.swift.

ktoso approved these changes Sep 11, 2023

View reviewed changes

beccadax merged commit 8770c7f into swiftlang:main Sep 12, 2023

beccadax mentioned this pull request Sep 13, 2023

[NFC-ish] Overhaul ASTDumper #38257

Closed

AnthonyLatsis reviewed Jan 11, 2024

View reviewed changes

AnthonyLatsis added a commit to AnthonyLatsis/swift that referenced this pull request Dec 7, 2024

ASTDumper: Do not escape Unicode chars in quoted fields

81a564a

Context: swiftlang#68438 (comment)

AnthonyLatsis mentioned this pull request Dec 7, 2024

ASTDumper: Do not escape Unicode chars in quoted fields #78044

Merged

AnthonyLatsis added a commit to AnthonyLatsis/swift that referenced this pull request Dec 10, 2024

ASTDumper: Do not escape Unicode chars in quoted fields

1001c46

Context: swiftlang#68438 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Rework ASTDumper #68438

Rework ASTDumper #68438

Uh oh!

beccadax commented Sep 11, 2023 •

edited

Loading

Uh oh!

beccadax commented Sep 11, 2023

Uh oh!

Uh oh!

beccadax commented Sep 11, 2023

Uh oh!

beccadax commented Sep 11, 2023

Uh oh!

beccadax commented Sep 11, 2023

Uh oh!

beccadax commented Sep 11, 2023

Uh oh!

ktoso left a comment

Uh oh!

AnthonyLatsis Jan 11, 2024

Uh oh!

beccadax Jan 11, 2024

Uh oh!

Uh oh!

Rework ASTDumper #68438

Rework ASTDumper #68438

Uh oh!

Conversation

beccadax commented Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beccadax commented Sep 11, 2023

Uh oh!

Uh oh!

beccadax commented Sep 11, 2023

Uh oh!

beccadax commented Sep 11, 2023

Uh oh!

beccadax commented Sep 11, 2023

Uh oh!

beccadax commented Sep 11, 2023

Uh oh!

ktoso left a comment

Choose a reason for hiding this comment

Uh oh!

AnthonyLatsis Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

beccadax Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

beccadax commented Sep 11, 2023 •

edited

Loading