Preprocessor: Add synthetic token for trailing disabled branch trivia;

This was breaking cst json round trip when ifdefs were present.
Added another round trip test for that case.

This also puts all of the disabled content back under disabledTokens, which is generally easier to work with.

Author: AndrewNolte

include/slang/syntax/CSTSerializer.h

@@ -10,6 +10,10 @@
 #include "slang/syntax/SyntaxNode.h"
 #include "slang/text/Json.h"
 
+namespace slang {
+class SourceManager;
+}
+
 namespace slang::syntax {
 
 class SyntaxTree;
@@ -23,6 +27,16 @@ class SLANG_EXPORT CSTSerializer {
 public:
     explicit CSTSerializer(JsonWriter& writer, CSTJsonMode mode = CSTJsonMode::Full);
 
+    /// Sets a source manager used to annotate tokens that originate from a macro expansion
+    /// or an included file with a `"fromExpansion": true` property. Such tokens are
+    /// serialized in addition to the directive that produced them (the macro usage or
+    /// `include, which occupies the same textual position); this flag lets consumers (e.g.
+    /// source reconstruction) tell the two apart and avoid double-counting the text.
+    ///
+    /// When serializing a whole SyntaxTree this is set automatically from the tree; it only
+    /// needs to be called explicitly when serializing a bare SyntaxNode.
+    void setSourceManager(const SourceManager* sm) { sourceManager = sm; }
+
     /// Serialize a syntax tree to JSON
     void serialize(const SyntaxTree& tree);
 
@@ -32,6 +46,7 @@ class SLANG_EXPORT CSTSerializer {
 private:
     JsonWriter& writer;
     CSTJsonMode mode;
+    const SourceManager* sourceManager = nullptr;
 };
 
 } // namespace slang::syntax

scripts/reconstruct_from_json.py

@@ -21,17 +21,30 @@ def extract_tokens(node, tokens):
     if node is None:
         return
 
+    # Text that comes from a macro expansion or an included file is serialized in addition
+    # to the directive that produced it (which occupies the same source position), so skip
+    # it to reconstruct the original, unexpanded source. This flag is set on both tokens
+    # and their plain-text trivia. Note directive trivia carry no "text" of their own, so
+    # even when they sit inside expanded content they are still recursed into below (the
+    # producing `include / macro usage lives there).
+    expanded = isinstance(node, dict) and node.get("fromExpansion")
+
     if isinstance(node, dict):
         # Check if this is a token node
         if "kind" in node and "text" in node:
-            # This is a token - add its text and trivia
+            # Always process trivia first (it may itself contain directive syntax, e.g.
+            # `ifdef branches whose disabled tokens hang off the directive, or the
+            # `include / macro usage that produced an expansion) followed by the token's
+            # own text.
             if "trivia" in node:
                 for trivia in node["trivia"]:
-                    if "text" in trivia:
-                        tokens.append(trivia["text"])
-            tokens.append(node["text"])
+                    extract_tokens(trivia, tokens)
+            if not expanded:
+                tokens.append(node["text"])
         else:
-            # This is a syntax node - recurse through all properties
+            # This is a syntax node (or a piece of trivia) - recurse through all
+            # properties. This covers directive trivia, whose content lives under a
+            # nested "syntax" node rather than a flat "text" field.
             for key, value in node.items():
                 if key in ["kind"]:  # Skip metadata
                     continue

source/parsing/Preprocessor.cpp

@@ -9,6 +9,7 @@
 
 #include "slang/diagnostics/LexerDiags.h"
 #include "slang/diagnostics/PreprocessorDiags.h"
+#include "slang/parsing/TokenKind.h"
 #include "slang/syntax/AllSyntax.h"
 #include "slang/text/Glob.h"
 #include "slang/text/SourceManager.h"
@@ -988,25 +989,14 @@ Trivia Preprocessor::parseBranchDirective(Token directive,
             }
 
             if (done) {
-                // Put the token back so that we'll look at it next, but with
-                // its trivia rewritten to change comments and whitespace to disabled
-                // text, since this branch was not taken and we want the comments to
-                // disappear in the preprocessed output.
-                SmallVector<Trivia, 2> trivia(token.trivia());
-                for (auto& t : trivia) {
-                    switch (t.kind) {
-                        case TriviaKind::LineComment:
-                        case TriviaKind::BlockComment:
-                        case TriviaKind::Whitespace:
-                        case TriviaKind::EndOfLine:
-                            t.kind = TriviaKind::DisabledText;
-                            break;
-                        default:
-                            break;
-                    }
-                }
 
-                currentToken = token.withTrivia(alloc, trivia);
+                // Make a synthetic token for holding the trailing trivia, to keep it still
+                // classified as disabled text.
+                scratchTokenBuffer.push_back(
+                    Token::createMissing(alloc, TokenKind::Placeholder, token.location())
+                        .withTrivia(alloc, token.trivia()));
+
+                currentToken = token.withTrivia(alloc, {});
                 break;
             }
             scratchTokenBuffer.push_back(token);

source/syntax/CSTSerializer.cpp

@@ -15,6 +15,7 @@
 #include "slang/syntax/AllSyntax.h"
 #include "slang/syntax/SyntaxTree.h"
 #include "slang/syntax/SyntaxVisitor.h"
+#include "slang/text/SourceManager.h"
 #include "slang/util/Util.h"
 
 namespace slang::syntax {
@@ -23,6 +24,11 @@ CSTSerializer::CSTSerializer(JsonWriter& writer, CSTJsonMode mode) : writer(writ
 }
 
 void CSTSerializer::serialize(const SyntaxTree& tree) {
+    // A tree always knows its source manager, so use it (unless one was set explicitly)
+    // to annotate macro-expanded tokens.
+    if (!sourceManager)
+        sourceManager = &tree.sourceManager();
+
     writer.startObject();
     writer.writeProperty("kind");
     writer.writeValue("SyntaxTree"sv);
@@ -37,8 +43,10 @@ struct always_false : std::false_type {};
 struct CSTJsonVisitor {
     JsonWriter& writer;
     CSTJsonMode mode;
+    const SourceManager* sourceManager;
 
-    CSTJsonVisitor(JsonWriter& w, CSTJsonMode m) : writer(w), mode(m) {}
+    CSTJsonVisitor(JsonWriter& w, CSTJsonMode m, const SourceManager* sm) :
+        writer(w), mode(m), sourceManager(sm) {}
 
     template<std::derived_from<SyntaxNode> T>
     void visit(const T& node) {
@@ -143,10 +151,33 @@ struct CSTJsonVisitor {
         writer.endArray();
     }
 
-    void writeTrivia(parsing::Trivia trivia) {
+    // Returns true if the given source location comes from a macro expansion or an
+    // included file, i.e. text that is serialized in addition to the directive that
+    // produced it and so should not be double-counted when reconstructing source.
+    bool isExpandedLoc(SourceLocation loc) const {
+        return sourceManager &&
+               (sourceManager->isMacroLoc(loc) || sourceManager->isIncludedFileLoc(loc));
+    }
+
+    // @a parentExpanded is the expansion state of the token this trivia belongs to; plain
+    // text trivia has no location of its own and so inherits it.
+    void writeTrivia(parsing::Trivia trivia, bool parentExpanded) {
         writer.startObject();
         writer.writeProperty("kind");
         writer.writeValue(toString(trivia.kind));
+
+        // Flag trivia that is itself expanded text. Directive/skipped trivia carry their
+        // own location; plain text trivia inherits the parent token's expansion state.
+        bool expanded = parentExpanded;
+        if (sourceManager) {
+            if (auto loc = trivia.getExplicitLocation())
+                expanded = isExpandedLoc(*loc);
+        }
+        if (expanded) {
+            writer.writeProperty("fromExpansion");
+            writer.writeValue(true);
+        }
+
         switch (trivia.kind) {
             case parsing::TriviaKind::Directive:
             case parsing::TriviaKind::SkippedSyntax:
@@ -181,14 +212,26 @@ struct CSTJsonVisitor {
         writer.writeProperty("text");
         writer.writeValue(token.rawText());
 
+        // Flag tokens that come from a macro expansion or an included file. Such tokens
+        // are serialized in addition to the directive that produced them -- the macro
+        // usage, or the `include -- which occupies the same textual position, so consumers
+        // reconstructing the original source can skip them to avoid double-counting. The
+        // same flag is propagated to the token's plain-text trivia, which has no location
+        // of its own.
+        bool expanded = isExpandedLoc(token.location());
+        if (expanded) {
+            writer.writeProperty("fromExpansion");
+            writer.writeValue(true);
+        }
+
         // Handle trivia based on mode
         if (!token.trivia().empty()) {
             switch (mode) {
                 case CSTJsonMode::Full:
                     writer.writeProperty("trivia");
                     writer.startArray();
                     for (auto& t : token.trivia())
-                        writeTrivia(t);
+                        writeTrivia(t, expanded);
                     writer.endArray();
                     break;
                 case CSTJsonMode::NoWhitespace: {
@@ -211,7 +254,7 @@ struct CSTJsonVisitor {
                         writer.writeProperty("trivia");
                         writer.startArray();
                         for (auto& t : filtered)
-                            writeTrivia(t);
+                            writeTrivia(t, expanded);
                         writer.endArray();
                     }
                     break;
@@ -238,7 +281,7 @@ struct CSTJsonVisitor {
 };
 
 void CSTSerializer::serialize(const SyntaxNode& node) {
-    CSTJsonVisitor visitor(writer, mode);
+    CSTJsonVisitor visitor(writer, mode, sourceManager);
     node.visit(visitor);
 }
 

tests/regression/CMakeLists.txt

@@ -24,6 +24,48 @@ add_test(
 set_tests_properties(regression_cst_json_roundtrip
                      PROPERTIES DEPENDS regression_cst_json_gen)
 
+# CST JSON round trip test focused on preprocessor conditional directives, whose disabled
+# branches exercise the trivia handling that plain source (all.sv) doesn't cover.
+add_test(
+  NAME regression_cst_json_conditionals_gen
+  COMMAND slang::driver --cst-json ${CMAKE_CURRENT_BINARY_DIR}/conditionals_cst.json
+          ${CMAKE_CURRENT_LIST_DIR}/conditionals.sv "--std=1800-2023")
+
+add_test(
+  NAME regression_cst_json_conditionals_roundtrip
+  COMMAND
+    ${Python_EXECUTABLE} ${PROJECT_SOURCE_DIR}/scripts/reconstruct_from_json.py
+    ${CMAKE_CURRENT_BINARY_DIR}/conditionals_cst.json --compare
+    ${CMAKE_CURRENT_LIST_DIR}/conditionals.sv)
+
+set_tests_properties(regression_cst_json_conditionals_roundtrip
+                     PROPERTIES DEPENDS regression_cst_json_conditionals_gen)
+
+# Exercise the same source with different conditional branches enabled. Each variant writes
+# to a separate file so that the generation tests can run concurrently.
+function(add_cst_json_conditionals_variant name)
+  set(json_file ${CMAKE_CURRENT_BINARY_DIR}/conditionals_${name}_cst.json)
+
+  add_test(
+    NAME regression_cst_json_conditionals_${name}_gen
+    COMMAND slang::driver --cst-json ${json_file}
+            ${CMAKE_CURRENT_LIST_DIR}/conditionals.sv "--std=1800-2023" ${ARGN})
+
+  add_test(
+    NAME regression_cst_json_conditionals_${name}_roundtrip
+    COMMAND
+      ${Python_EXECUTABLE} ${PROJECT_SOURCE_DIR}/scripts/reconstruct_from_json.py
+      ${json_file} --compare ${CMAKE_CURRENT_LIST_DIR}/conditionals.sv)
+
+  set_tests_properties(
+    regression_cst_json_conditionals_${name}_roundtrip
+    PROPERTIES DEPENDS regression_cst_json_conditionals_${name}_gen)
+endfunction()
+
+add_cst_json_conditionals_variant(foo -DFOO)
+add_cst_json_conditionals_variant(bar_baz -DBAR -DBAZ)
+add_cst_json_conditionals_variant(foo_inner -DFOO -DINNER)
+
 add_subdirectory(driver)
 
 if(SLANG_INCLUDE_UVM_TEST)

tests/regression/conditionals.sv

@@ -0,0 +1,54 @@
+// Exercises preprocessor conditional directives so the CST JSON round-trip covers the
+// trivia handling of disabled branches (line breaks, indentation, and comments that end
+// a skipped branch must still round-trip faithfully).
+
+module m;
+`ifdef FOO
+    logic disabled_simple;
+`endif
+
+`ifndef FOO
+    logic taken_simple;
+`else
+    logic disabled_else;
+`endif
+
+`ifdef FOO
+    logic a;
+`elsif BAR
+    logic b;
+`else
+    logic c;
+`endif
+
+`ifndef FOO
+    // taken branch with a nested disabled block
+    `ifdef BAZ
+        logic nested_disabled;
+    `else
+        logic nested_taken;
+    `endif
+`endif
+
+`ifdef FOO
+    // disabled branch containing comments and a nested pair
+    logic x; // trailing comment
+    `ifdef INNER
+        logic y;
+    `endif
+    // dangling comment before endif
+`endif
+
+// A macro whose body itself contains conditional directives. In the FOO-defined test
+// variants its invocation produces expanded directive trivia that must not be
+// double-counted when reconstructing the original source.
+`define GUARD(name) \
+    `ifndef name \
+        `define name \
+    `endif
+
+`ifdef FOO
+    `GUARD(SOMETHING)
+`endif
+
+endmodule

tests/regression/driver/cst-json-modes.sv

@@ -16,24 +16,24 @@
 // RUN: echo CST-MODE-simple-tokens && %slang --cst-json - --cst-json-mode simple-tokens --parse-only %data/cst_serializer_modes.sv || true
 
 // "full" mode keeps every trivia kind verbatim, including whitespace, the
-// disabled `ifdef text, comments, directives and skipped tokens.
+// disabled `ifdef branch (whose tokens are carried as disabled tokens ending in
+// a Placeholder), comments, directives and skipped tokens.
 // CHECK-LABEL: CST-MODE-full
 // CHECK-DAG: "kind": "Whitespace"
-// CHECK-DAG: "kind": "DisabledText"
+// CHECK-DAG: "kind": "Placeholder"
 // CHECK-DAG: "kind": "Directive"
 // CHECK-DAG: "kind": "LineComment"
 // CHECK-DAG: "kind": "BlockComment"
 // CHECK-DAG: "kind": "SkippedTokens"
 
-// "no-whitespace" mode drops Whitespace/EndOfLine (and whitespace-only
-// DisabledText) trivia but preserves comments, directives and skipped tokens.
+// "no-whitespace" mode drops Whitespace/EndOfLine trivia but preserves comments,
+// directives and skipped tokens.
 // CHECK-LABEL: CST-MODE-no-whitespace
 // CHECK-DAG: "kind": "Directive"
 // CHECK-DAG: "kind": "LineComment"
 // CHECK-DAG: "kind": "BlockComment"
 // CHECK-DAG: "kind": "SkippedTokens"
 // CHECK-NOT: "kind": "Whitespace"
-// CHECK-NOT: "kind": "DisabledText"
 
 // "simple-trivia" mode emits the trivia of each token as a single concatenated
 // string value instead of an array of structured trivia objects.

tests/unittests/data/cst_serializer_modes.sv

@@ -3,9 +3,10 @@
 
 // Fixture for tests/regression/driver/cst-json-modes.sv. It is serialized via
 // `--cst-json` in every CSTJsonMode. It intentionally contains a macro
-// definition, an untaken `ifdef branch (which produces DisabledText trivia),
-// line and block comments, and a stray token after `endmodule` (which becomes
-// SkippedTokens trivia) so that the directive, disabled-text and skipped-token
+// definition, an untaken `ifdef branch (whose skipped tokens are carried as the
+// directive's disabled tokens, terminated by a Placeholder token), line and
+// block comments, and a stray token after `endmodule` (which becomes
+// SkippedTokens trivia) so that the directive, disabled-token and skipped-token
 // serialization paths all run.
 
 `define WIDTH 8