Include source ranges for statements, declarations, and parameter lists in the AST dump #16473
Conversation
|
@jrose-apple Want to take a look? |
There was a problem hiding this comment.
I almost want to say "no" on principle—please please don't parse AST dumps! They are not expected to be stable or complete or machine-readable. But this is a good change regardless, so thank you for doing it. I have one review question and two style nitpicks.
include/swift/AST/Stmt.h
Outdated
| @@ -128,7 +128,7 @@ class alignas(8) Stmt { | |||
| LLVM_ATTRIBUTE_DEPRECATED( | |||
| void dump() const LLVM_ATTRIBUTE_USED, | |||
| "only for use within the debugger"); | |||
| void print(raw_ostream &OS, unsigned Indent = 0) const; | |||
| void print(raw_ostream &OS, const ASTContext * = nullptr, unsigned Indent = 0) const; | |||
There was a problem hiding this comment.
Nitpick: please provide a parameter name here, even if it's just "Ctx".
| OS.indent(Indent); | ||
| PrintWithColorRAII(OS, ParenthesisColor) << "("; | ||
| PrintWithColorRAII(OS, ASTNodeColor) << Name; | ||
| void printASTNodes(const ArrayRef<ASTNode> &Elements) { |
There was a problem hiding this comment.
This change was to allow visitBraceStmt to delegate to printCommon, so it can reuse the logic for printing the source range.
lib/AST/ASTDumper.cpp
Outdated
| @@ -424,7 +424,7 @@ namespace { | |||
|
|
|||
| void printRec(Decl *D) { D->dump(OS, Indent + 2); } | |||
| void printRec(Expr *E) { E->print(OS, Indent + 2); } | |||
| void printRec(Stmt *S) { S->print(OS, Indent + 2); } | |||
| void printRec(Stmt *S, const ASTContext& Ctx) { S->print(OS, &Ctx, Indent + 2); } | |||
There was a problem hiding this comment.
Style nitpick: please attach the & to the declaration name, not the type.
| OS.indent(Indent); | ||
| PrintWithColorRAII(OS, ParenthesisColor) << "("; | ||
| PrintWithColorRAII(OS, ASTNodeColor) << Name; | ||
| void printASTNodes(const ArrayRef<ASTNode> &Elements) { |
There was a problem hiding this comment.
This change was to allow visitBraceStmt to delegate to printCommon, so it can reuse the logic for printing the source range.
|
I've addressed the style issues. |
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
|
Merged. Thanks, Mark! |
|
@jrose-apple @bdash I reverted this because it appears to have broken a test in CI: https://ci.swift.org/job/oss-swift-incremental-RA-linux-ubuntu-16_04/4126/ Please take a look |
|
How did this pass the regular testing? Weird. Thanks, Ben. |
|
#16622 was merged after this was tested, and introduced a new |
|
#16682 is this PR rebased on master, with the necessary fixes made to tests. |
Source ranges are currently missing from statements, declarations, and parameter lists in the AST dump. This PR addresses that.
For some background: we have a Swift-based domain-specific language that works by analyzing AST dumps and transforming them into new Swift code. In cases where the input doesn't match the constraints of our DSL, we emit errors that point to the problems in the original Swift source. The lack of source range information on these node types makes this difficult for certain types of errors.