Rewriting recursively with SyntaxRewriter

[fischeti]

Hi,

I am currently implementing a SystemVerilog pickler, which is able to concatenate files, and additionally rename modules/interfaces packages to be able to use the pickled file without naming collisions. I’m using slang::syntax::SyntaxRewriter and ran into what looks like a commit ordering / recursion boundary issue.

For instance, I want to rewrite/prefix the following example:

module core #(
    parameter common_pkg::state_t DefaultState = common_pkg::Idle
) ();
endmodule

to:

module prefix_core #(
    parameter prefix_common_pkg::state_t DefaultState = prefix_common_pkg::Idle
) ();
endmodule

And here is the rewriter implementation (the actual one is a bit more sophisticated of course):

class Rewriter : public SyntaxRewriter<Rewriter> {
public:
    void handle(const ModuleDeclarationSyntax& node) {
        auto newNameTok = node.header->name.withRawText(alloc, "prefix_core");

        auto* newHeader = deepClone(*node.header, alloc);
        newHeader->name = newNameTok;

        // parent-level replace
        replace(*node.header, *newHeader);

        visitDefault(node);
    }

    void handle(const ScopedNameSyntax& node) {
        if (node.left->kind == SyntaxKind::IdentifierName) {
            auto& left = node.left->as<IdentifierNameSyntax>();
            if (left.identifier.valueText() == "common_pkg") {
                auto* newLeft = deepClone(left, alloc);
                newLeft->identifier = left.identifier.withRawText(alloc, "prefix_common_pkg");

                auto* newNode = deepClone(node, alloc);
                newNode->left = newLeft;

                // child-level replace
                replace(node, *newNode);
            }
        }
        visitDefault(node);
    }
};

The rewriter handlers work in most cases except when a ScopedName is a descendant from ModuleDeclaration, which is the case in the provided example. My understanding so far is that CloneVisitor::visit looks if a child has a replace commit, clone uses the replacement node directly and does not recurse into the old subtree. So a parent replacement (header) can bypass descendant replacement commits (scoped names) recorded on old nodes.

Is my understanding correct, or am I missing something?

My next approach would be to implement the rewriting in multiple passes e.g. first ScopedName and then ModuleDeclaration. But I was wondering whether there is a more general approach to implement recursive rewritting.

Thanks in advance for any guidance!

[MikePopoloski]

The replace call just queues up the replacement; it doesn't go into the tree (really the result of rewriting is a new tree) until the end when all changes are applied in one go. If you create a new subtree to replace, and want to replace things within that new subtree, you need to actually visit that subtree explicitly since visitDefault won't know about it. Alternatively you can do it in multiple passes, but doing it one is going to be more efficient.

[MikePopoloski]

Just to say, I think your understanding is correct. Basically it is assumed that if you call replace() the new node you are inserting is already in the final state you want it to be. If it's not you need to manually make a call to transform that subtree.

[fischeti]

I see, thanks a lot for the quick answer! I ended up doing two passes in the end, since I anyway first need to collect module declarations before renaming references in the second pass. This works quite well, except for scoped names inside module references. Those I currently "manually" rewrite i.e. copy the subtree, rewrite the scoped names explicitely and then call replace()with the new node/subtree.

Is this along the lines on what you suggested? Or is there a way to transform the cloned subtree recursively again with my SyntaxRewriter? I was not able to achieve this since transform() requires a SyntaxTree instead of a node.

[MikePopoloski]

It seems like conceptually it should be possible but probably not with the current API. If you want to make improvements to it I'd be happy to accept PRs.

[fischeti]

Thanks! I will consider adapting it and opening a PR. At the moment, the two-pass approach works for me, but I might still be missing some edge cases.