Refactor preprocessor API to avoid coupling (#1559)

Based on review feedback from #1556, this change updates the Slang preprocessor so that it is no longer coupled to policy details from higher levels of the software stack. In particular, the preprocessor used to:

* Deal with updating the list of file paths that a `Module` depends on.

* (As of #1556) detect NVAPI-related macro definitions and use them to construct an AST-level `Modifier` attached to the `ModuleDecl`.

This change introduces a callback interface where the `Preprocessor` calls out to a `PreprocessorHandler` at certain points during execution, allowing the handler to introduce custom logic that suits a particular high-level use case.

This change also removes the dependence of the preprocessor on the `Linkage`, because in practice only a small number of its sub-objects were needed. As a convenience, a wrapper function that takes a `Linkage` was left in place so that the existing call sites didn't have to change very much.

3 files changed, 250 insertions, 135 deletions

diff --git a/source/slang/slang-preprocessor.cpp b/source/slang/slang-preprocessor.cpp
index 5a54bfcb0..b5861d986 100644
--- a/source/slang/slang-preprocessor.cpp
+++ b/source/slang/slang-preprocessor.cpp
@@ -216,21 +216,24 @@ struct Preprocessor
     // represent end-of-input situations.
     Token                                   endOfFileToken;
 
-        /// The linkage the provides the context for preprocessing
-    Linkage*                                linkage = nullptr;
-
-        /// The module, if any, that the preprocessed result will belong to
-    Module*                                 parentModule = nullptr;
-
-        /// The AST builder that should be used when creating AST nodes for `parentModule`
-    ASTBuilder*                             astBuilder = nullptr;
+        /// Callback handlers
+    PreprocessorHandler*                    handler = nullptr;
 
     // The unique identities of any paths that have issued `#pragma once` directives to
     // stop them from being included again.
     HashSet<String>                         pragmaOnceUniqueIdentities;
 
-    NamePool* getNamePool() { return linkage->getNamePool(); }
-    SourceManager* getSourceManager() { return linkage->getSourceManager(); }
+        /// Name pool to use when creating `Name`s from strings
+    NamePool*                               namePool = nullptr;
+
+        /// File system to use when looking up files
+    ISlangFileSystemExt*                    fileSystem = nullptr;
+
+        /// Source maanger to use when loading source files
+    SourceManager*                          sourceManager = nullptr;
+
+    NamePool* getNamePool() { return namePool; }
+    SourceManager* getSourceManager() { return sourceManager; }
 };
 
 
@@ -1788,17 +1791,16 @@ static SlangResult readFile(
     // The actual file loading will be handled by the file system
     // associated with the parent linkage.
     //
-    auto linkage = context->preprocessor->linkage;
-    auto fileSystemExt = linkage->getFileSystemExt();
+    auto fileSystemExt = context->preprocessor->fileSystem;
     SLANG_RETURN_ON_FAIL(fileSystemExt->loadFile(path.getBuffer(), outBlob));
 
     // If we are running the preprocessor as part of compiling a
     // specific module, then we must keep track of the file we've
     // read as yet another file that the module will depend on.
     //
-    if(auto module = context->preprocessor->parentModule)
+    if( auto handler = context->preprocessor->handler )
     {
-        module->addFilePathDependency(path);
+        handler->handleFileDependency(path);
     }
 
     return SLANG_OK;
@@ -2451,18 +2453,18 @@ static TokenList ReadAllTokens(
 }
 
     /// Try to look up a macro with the given `macroName` and produce its value as a string
-static bool _findMacroValue(
+Result findMacroValue(
     Preprocessor*   preprocessor,
     char const*     macroName,
     String&         outValue,
     SourceLoc&      outLoc)
 {
-    auto namePool = preprocessor->linkage->getNamePool();
+    auto namePool = preprocessor->namePool;
     auto macro = LookupMacro(preprocessor, namePool->getName(macroName));
     if(!macro)
-        return false;
+        return SLANG_FAIL;
     if(macro->flavor != PreprocessorMacroFlavor::ObjectLike)
-        return false;
+        return SLANG_FAIL;
 
     MacroExpansion* expansion = new MacroExpansion();
     initializeMacroExpansion(preprocessor, expansion, macro);
@@ -2482,118 +2484,68 @@ static bool _findMacroValue(
 
     outValue = value;
     outLoc = macro->getLoc();
-    return true;
+    return SLANG_OK;
 }
 
-    /// Validate that a re-defintion of an NVAPI-related macro matches any previous definition
-static void _validateNVAPIMacroMatch(
-    Preprocessor*   preprocessor,
-    char const*     macroName,
-    String const&   existingValue,
-    String const&   newValue,
-    SourceLoc       loc)
+void PreprocessorHandler::handleEndOfFile(Preprocessor* preprocessor)
 {
-    if( existingValue != newValue )
-    {
-        preprocessor->sink->diagnose(loc, Diagnostics::nvapiMacroMismatch, macroName, existingValue, newValue);
-    }
+    SLANG_UNUSED(preprocessor);
 }
 
-    /// Collect macro definitions that are relevant to subsequent compilation steps, and store them
-static void _collectDownstreamRelevantMacros(
-    Preprocessor*   preprocessor)
+void PreprocessorHandler::handleFileDependency(String const& path)
 {
-    // For now, the only case of semantically-relevant macros we need to worrry
-    // about are the NVAPI macros used to establish the register/space to use.
-    //
-    static const char* kNVAPIRegisterMacroName = "NV_SHADER_EXTN_SLOT";
-    static const char* kNVAPISpaceMacroName = "NV_SHADER_EXTN_REGISTER_SPACE";
+    SLANG_UNUSED(path);
+}
 
-    // For NVAPI use, the `NV_SHADER_EXTN_SLOT` macro is required to be defined.
-    //
-    String nvapiRegister;
-    SourceLoc nvapiRegisterLoc;
-    if(_findMacroValue(preprocessor, kNVAPIRegisterMacroName, nvapiRegister, nvapiRegisterLoc))
-    {
-        // In contrast, NVAPI can be used without defining `NV_SHADER_EXTN_REGISTER_SPACE`,
-        // which effectively defaults to `space0`.
-        //
-        String nvapiSpace = "space0";
-        SourceLoc nvapiSpaceLoc;
-        _findMacroValue(preprocessor, kNVAPISpaceMacroName, nvapiSpace, nvapiSpaceLoc);
+TokenList preprocessSource(
+    SourceFile*                         file,
+    DiagnosticSink*                     sink,
+    IncludeSystem*                      includeSystem,
+    Dictionary<String, String> const&   defines,
+    Linkage*                            linkage,
+    PreprocessorHandler*                handler)
+{
+    PreprocessorDesc desc;
 
-        // We are going to store the values of these macros on the AST-level `ModuleDecl`
-        // so that they will be available to later processing stages.
-        //
-        // Note: An alternative design here would be to either put the data directly
-        // on the `Module`, or to define some kind of side-channel output that the
-        // preprocessor can use to communicate these macro values back (and then
-        // allow another system to create the AST nodes). In practice, all of these
-        // alternatives would actually increase the amount of code/complexity,
-        // so we stick with the simple-but-hacky option of having the
-        // preprocessor create AST nodes directly.
-        //
-        auto module = preprocessor->parentModule;
-        if(!module) return;
-        auto moduleDecl = module->getModuleDecl();
+    desc.sink           = sink;
+    desc.includeSystem  = includeSystem;
+    desc.handler        = handler;
 
-        // We need to make sure that the AST nodes we create will have the right
-        // lifetime (it should match the module we are adding to).
-        //
-        auto astBuilder = preprocessor->astBuilder;
-        if(!astBuilder) return;
+    desc.defines = &defines;
 
-        if(auto existingModifier = moduleDecl->findModifier<NVAPISlotModifier>())
-        {
-            // If there is already a modifier attached to the module (perhaps
-            // because of preprocessing a different source file, or because
-            // of settings established via command-line options), then we
-            // need to validate that the values being set in this file
-            // match those already set (or else there is likely to be
-            // some kind of error in the user's code).
-            //
-            _validateNVAPIMacroMatch(preprocessor, kNVAPIRegisterMacroName, existingModifier->registerName, nvapiRegister,  nvapiRegisterLoc);
-            _validateNVAPIMacroMatch(preprocessor, kNVAPISpaceMacroName,    existingModifier->spaceName,    nvapiSpace,     nvapiSpaceLoc);
-        }
-        else
-        {
-            // If there is no existing modifier on the module, then we
-            // take responsibility for adding one, based on the macro
-            // values we saw.
-            //
-            auto modifier = astBuilder->create<NVAPISlotModifier>();
-            modifier->loc = nvapiRegisterLoc;
-            modifier->registerName = nvapiRegister;
-            modifier->spaceName = nvapiSpace;
+    desc.fileSystem     = linkage->getFileSystemExt();
+    desc.namePool       = linkage->getNamePool();
+    desc.sourceManager  = linkage->getSourceManager();
 
-            addModifier(moduleDecl, modifier);
-        }
-    }
+    return preprocessSource(file, desc);
 }
 
 TokenList preprocessSource(
-    SourceFile*                 file,
-    DiagnosticSink*             sink,
-    IncludeSystem*              includeSystem,
-    Dictionary<String, String>  defines,
-    Linkage*                    linkage,
-    Module*                     parentModule,
-    ASTBuilder*                 astBuilder)
+    SourceFile*             file,
+    PreprocessorDesc const& desc)
 {
     Preprocessor preprocessor;
-    InitializePreprocessor(&preprocessor, sink);
-    preprocessor.linkage = linkage;
-    preprocessor.parentModule = parentModule;
-    preprocessor.astBuilder = astBuilder;
+    InitializePreprocessor(&preprocessor, desc.sink);
 
-    preprocessor.includeSystem = includeSystem;
-    for (auto p : defines)
+    preprocessor.sink = desc.sink;
+    preprocessor.includeSystem = desc.includeSystem;
+    preprocessor.fileSystem = desc.fileSystem;
+    preprocessor.namePool = desc.namePool;
+
+    auto sourceManager = desc.sourceManager;
+    preprocessor.sourceManager = sourceManager;
+
+    auto handler = desc.handler;
+    preprocessor.handler = handler;
+
+    if(desc.defines)
     {
-        DefineMacro(&preprocessor, p.Key, p.Value);
+        for (auto p : *desc.defines)
+        {
+            DefineMacro(&preprocessor, p.Key, p.Value);
+        }
     }
 
-    SourceManager* sourceManager = linkage->getSourceManager();
-
     SourceView* sourceView = sourceManager->createSourceView(file, nullptr);
 
     // create an initial input stream based on the provided buffer
@@ -2601,16 +2553,10 @@ TokenList preprocessSource(
 
     TokenList tokens = ReadAllTokens(&preprocessor);
 
-    // We look at the preprocessor state after reading the entire
-    // source file/string, in order to see if any macros have been
-    // set that should be considered semantically relevant for
-    // later stages of compilation.
-    //
-    // Note: Checking the macro environment *after* preprocessing is complete
-    // means that we can treat macros introduced via `-D` options or the API
-    // equivalently to macros introduced via `#define`s in user code.
-    //
-    _collectDownstreamRelevantMacros(&preprocessor);
+    if(handler)
+    {
+        handler->handleEndOfFile(&preprocessor);
+    }
 
     FinalizePreprocessor(&preprocessor);
 
diff --git a/source/slang/slang-preprocessor.h b/source/slang/slang-preprocessor.h
index 7c72a859c..efecb912b 100644
--- a/source/slang/slang-preprocessor.h
+++ b/source/slang/slang-preprocessor.h
@@ -10,21 +10,72 @@
 
 namespace Slang {
 
-class ASTBuilder;
 class DiagnosticSink;
 class Linkage;
-class Module;
-class ModuleDecl;
 
-// Take a string of source code and preprocess it into a list of tokens.
+struct Preprocessor;
+
+    /// A handler for callbacks invoked by the preprocessor.
+    ///
+    /// A client of the preprocessor can implement its own `PreprocessorHandler` subtype
+    /// in order to insert custom logic that implements higher-level policies
+    /// that the preprocessor shouldn't need to understand.
+    ///
+struct PreprocessorHandler
+{
+    virtual void handleEndOfFile(Preprocessor* preprocessor);
+    virtual void handleFileDependency(String const& path);
+};
+
+    /// Description of a preprocessor options/dependencies
+struct PreprocessorDesc
+{
+        /// Required: sink to use when emitting preprocessor diagnostic messages
+    DiagnosticSink* sink = nullptr;
+
+        /// Required: name pool to use when creating `Name`s from strings
+    NamePool* namePool = nullptr;
+
+        /// Required: file system to use when looking up files
+    ISlangFileSystemExt* fileSystem = nullptr;
+
+        /// Required: source manager to use when loading source files
+    SourceManager* sourceManager = nullptr;
+
+        /// Optional: include system to use when resolving `#include` directives
+    IncludeSystem*  includeSystem = nullptr;
+
+        /// Optional: preprocessor `#define`s to assume are set on input
+    Dictionary<String, String> const* defines = nullptr;
+
+        /// Optional: handler for callbacks invoked during preprocessing
+    PreprocessorHandler* handler = nullptr;
+};
+
+    /// Take a source `file` and preprocess it into a list of tokens.
+TokenList preprocessSource(
+    SourceFile*             file,
+    PreprocessorDesc const& desc);
+
+    /// Convenience wrapper for `preprocessSource` when a `Linkage` is available
 TokenList preprocessSource(
-    SourceFile*                 file,
-    DiagnosticSink*             sink,
-    IncludeSystem*              includeSystem,
-    Dictionary<String, String>  defines,
-    Linkage*                    linkage,
-    Module*                     parentModule,
-    ASTBuilder*                 astBuilder = nullptr);
+    SourceFile*                         file,
+    DiagnosticSink*                     sink,
+    IncludeSystem*                      includeSystem,
+    Dictionary<String, String> const&   defines,
+    Linkage*                            linkage,
+    PreprocessorHandler*                handler = nullptr);
+
+// The following functions are intended to be used inside of implementations
+// of the `PreprocessorHandler` interface, in order to query the current
+// state of the preprocessor.
+
+    /// Try to look up a macro with the given `macroName` and produce its value as a string
+Result findMacroValue(
+    Preprocessor*   preprocessor,
+    char const*     macroName,
+    String&         outValue,
+    SourceLoc&      outLoc);
 
 } // namespace Slang
 
diff --git a/source/slang/slang.cpp b/source/slang/slang.cpp
index 0add5acea..1086338f4 100644
--- a/source/slang/slang.cpp
+++ b/source/slang/slang.cpp
@@ -862,8 +862,7 @@ Expr* Linkage::parseTermString(String typeStr, RefPtr<Scope> scope)
         &sink,
         nullptr,
         Dictionary<String,String>(),
-        this,
-        nullptr);
+        this);
 
     return parseTermFromSourceFile(
         getASTBuilder(),
@@ -919,6 +918,119 @@ FrontEndCompileRequest::FrontEndCompileRequest(
 {
 }
 
+    /// Handlers for preprocessor callbacks to use when doing ordinary front-end compilation
+struct FrontEndPreprocessorHandler : PreprocessorHandler
+{
+public:
+    FrontEndPreprocessorHandler(
+        Module*         module,
+        ASTBuilder*     astBuilder,
+        DiagnosticSink* sink)
+        : m_module(module)
+        , m_astBuilder(astBuilder)
+        , m_sink(sink)
+    {
+    }
+
+protected:
+    Module*         m_module;
+    ASTBuilder*     m_astBuilder;
+    DiagnosticSink* m_sink;
+
+    // The first task that this handler tries to deal with is
+    // capturing all the files on which a module is dependent.
+    //
+    // That information is exposed through public APIs and used
+    // by applications to decide when they need to "hot reload"
+    // their shader code.
+    //
+    void handleFileDependency(String const& path) SLANG_OVERRIDE
+    {
+        m_module->addFilePathDependency(path);
+    }
+
+    // The second task that this handler deals with is detecting
+    // whether any macro values were set in a given source file
+    // that are semantically relevant to other stages of compilation.
+    //
+    void handleEndOfFile(Preprocessor* preprocessor) SLANG_OVERRIDE
+    {
+        // We look at the preprocessor state after reading the entire
+        // source file/string, in order to see if any macros have been
+        // set that should be considered semantically relevant for
+        // later stages of compilation.
+        //
+        // Note: Checking the macro environment *after* preprocessing is complete
+        // means that we can treat macros introduced via `-D` options or the API
+        // equivalently to macros introduced via `#define`s in user code.
+        //
+        // For now, the only case of semantically-relevant macros we need to worrry
+        // about are the NVAPI macros used to establish the register/space to use.
+        //
+        static const char* kNVAPIRegisterMacroName = "NV_SHADER_EXTN_SLOT";
+        static const char* kNVAPISpaceMacroName = "NV_SHADER_EXTN_REGISTER_SPACE";
+
+        // For NVAPI use, the `NV_SHADER_EXTN_SLOT` macro is required to be defined.
+        //
+        String nvapiRegister;
+        SourceLoc nvapiRegisterLoc;
+        if(!SLANG_FAILED(findMacroValue(preprocessor, kNVAPIRegisterMacroName, nvapiRegister, nvapiRegisterLoc)))
+        {
+            // In contrast, NVAPI can be used without defining `NV_SHADER_EXTN_REGISTER_SPACE`,
+            // which effectively defaults to `space0`.
+            //
+            String nvapiSpace = "space0";
+            SourceLoc nvapiSpaceLoc;
+            findMacroValue(preprocessor, kNVAPISpaceMacroName, nvapiSpace, nvapiSpaceLoc);
+
+            // We are going to store the values of these macros on the AST-level `ModuleDecl`
+            // so that they will be available to later processing stages.
+            //
+            auto moduleDecl = m_module->getModuleDecl();
+
+            if(auto existingModifier = moduleDecl->findModifier<NVAPISlotModifier>())
+            {
+                // If there is already a modifier attached to the module (perhaps
+                // because of preprocessing a different source file, or because
+                // of settings established via command-line options), then we
+                // need to validate that the values being set in this file
+                // match those already set (or else there is likely to be
+                // some kind of error in the user's code).
+                //
+                _validateNVAPIMacroMatch(kNVAPIRegisterMacroName, existingModifier->registerName, nvapiRegister,  nvapiRegisterLoc);
+                _validateNVAPIMacroMatch(kNVAPISpaceMacroName,    existingModifier->spaceName,    nvapiSpace,     nvapiSpaceLoc);
+            }
+            else
+            {
+                // If there is no existing modifier on the module, then we
+                // take responsibility for adding one, based on the macro
+                // values we saw.
+                //
+                auto modifier = m_astBuilder->create<NVAPISlotModifier>();
+                modifier->loc = nvapiRegisterLoc;
+                modifier->registerName = nvapiRegister;
+                modifier->spaceName = nvapiSpace;
+
+                addModifier(moduleDecl, modifier);
+            }
+        }
+    }
+
+        /// Validate that a re-defintion of an NVAPI-related macro matches any previous definition
+    void _validateNVAPIMacroMatch(
+        char const*     macroName,
+        String const&   existingValue,
+        String const&   newValue,
+        SourceLoc       loc)
+    {
+        if( existingValue != newValue )
+        {
+            m_sink->diagnose(loc, Diagnostics::nvapiMacroMismatch, macroName, existingValue, newValue);
+        }
+    }
+};
+
+
 void FrontEndCompileRequest::parseTranslationUnit(
     TranslationUnitRequest* translationUnit)
 {
@@ -983,6 +1095,13 @@ void FrontEndCompileRequest::parseTranslationUnit(
         translationUnitSyntax->modifiers.first = astBuilder->create<FromStdLibModifier>();
     }
 
+    // We use a custom handler for preprocessor callbacks, to
+    // ensure that relevant state that is only visible during
+    // preprocessoing can be communicated to later phases of
+    // compilation.
+    //
+    FrontEndPreprocessorHandler preprocessorHandler(module, astBuilder, getSink());
+
     for (auto sourceFile : translationUnit->getSourceFiles())
     {
         auto tokens = preprocessSource(
@@ -991,8 +1110,7 @@ void FrontEndCompileRequest::parseTranslationUnit(
             &includeSystem,
             combinedPreprocessorDefinitions,
             getLinkage(),
-            module,
-            astBuilder);
+            &preprocessorHandler);
 
         parseSourceFile(
             astBuilder,