From 0f31f2fcf0887a2e9bfb69d8de153260b0c2dd53 Mon Sep 17 00:00:00 2001 From: Marc Auberer Date: Sat, 25 May 2024 17:38:27 +0200 Subject: [PATCH] Fix bug with circular import detection (#563) --- src/SourceFile.cpp | 9 ++-- src/irgenerator/DebugInfoGenerator.cpp | 4 +- src/irgenerator/GenValues.cpp | 24 +++++----- src/symboltablebuilder/Capture.cpp | 8 ++-- src/symboltablebuilder/Capture.h | 8 ++-- src/symboltablebuilder/SymbolTable.cpp | 2 +- src/symboltablebuilder/TypeSpecifiers.cpp | 47 ++++++++++--------- src/typechecker/TypeChecker.cpp | 9 ++-- src/util/CommonUtil.cpp | 2 +- .../exception.out | 2 +- 10 files changed, 60 insertions(+), 55 deletions(-) diff --git a/src/SourceFile.cpp b/src/SourceFile.cpp index 6346a864c..14fbdaf11 100644 --- a/src/SourceFile.cpp +++ b/src/SourceFile.cpp @@ -512,7 +512,7 @@ void SourceFile::concludeCompilation() { dumpOutput(compilerOutput.typesString, "Type Registry", "type-registry.out"); // Print warning if verifier is disabled - if (parent == nullptr && cliOptions.disableVerifier) { + if (isMainFile && cliOptions.disableVerifier) { const std::string warningMessage = CompilerWarning(VERIFIER_DISABLED, "The LLVM verifier passes are disabled. Please use this cli option carefully.") .warningMessage; @@ -619,7 +619,8 @@ bool SourceFile::isAlreadyImported(const std::string &filePathSearch, // NOLINT( if (std::filesystem::equivalent(filePath, filePathSearch)) return true; // Check parent recursively - return parent != nullptr && parent->isAlreadyImported(filePathSearch, circle); + const auto pred = [&](const SourceFile *d) { return d->isAlreadyImported(filePathSearch, circle); }; // NOLINT(*-no-recursion) + return std::ranges::any_of(dependants, pred); } SourceFile *SourceFile::requestRuntimeModule(RuntimeModule runtimeModule) { @@ -692,7 +693,7 @@ void SourceFile::collectAndPrintWarnings() { // NOLINT(misc-no-recursion) } const SourceFile *SourceFile::getRootSourceFile() const { // NOLINT(misc-no-recursion) - return parent == nullptr ? this : parent->getRootSourceFile(); + return isMainFile ? this : parent->getRootSourceFile(); } bool SourceFile::isRT(RuntimeModule runtimeModule) const { @@ -761,7 +762,7 @@ void SourceFile::dumpOutput(const std::string &content, const std::string &capti } void SourceFile::visualizerPreamble(std::stringstream &output) const { - if (parent == nullptr) + if (isMainFile) output << "digraph {\n rankdir=\"TB\";\n"; else output << "subgraph {\n"; diff --git a/src/irgenerator/DebugInfoGenerator.cpp b/src/irgenerator/DebugInfoGenerator.cpp index b7df8ef4e..1d591e711 100644 --- a/src/irgenerator/DebugInfoGenerator.cpp +++ b/src/irgenerator/DebugInfoGenerator.cpp @@ -180,13 +180,13 @@ llvm::DICompositeType *DebugInfoGenerator::generateCaptureStructDebugInfo(const std::vector fieldEntries; QualTypeList fieldSymbolTypes; for (const auto &[_, capture] : captures) { - QualType captureType = capture.capturedEntry->getQualType(); + QualType captureType = capture.capturedSymbol->getQualType(); // Capture by reference if (capture.getMode() == BY_REFERENCE) captureType = captureType.toRef(node); - fieldEntries.push_back(capture.capturedEntry); + fieldEntries.push_back(capture.capturedSymbol); fieldSymbolTypes.push_back(captureType); fieldTypes.push_back(captureType.toLLVMType(irGenerator->sourceFile)); } diff --git a/src/irgenerator/GenValues.cpp b/src/irgenerator/GenValues.cpp index 9e016a753..934bbb382 100644 --- a/src/irgenerator/GenValues.cpp +++ b/src/irgenerator/GenValues.cpp @@ -544,7 +544,7 @@ std::any IRGenerator::visitLambdaFunc(const LambdaFuncNode *node) { // Pop capture addresses if (hasCaptures) for (const auto &[_, capture] : captures) - capture.capturedEntry->popAddress(); + capture.capturedSymbol->popAddress(); // Conclude debug info for function diGenerator.concludeFunctionDebugInfo(); @@ -686,7 +686,7 @@ std::any IRGenerator::visitLambdaProc(const LambdaProcNode *node) { // Pop capture addresses if (hasCaptures) for (const auto &[_, capture] : captures) - capture.capturedEntry->popAddress(); + capture.capturedSymbol->popAddress(); // Conclude debug info for function diGenerator.concludeFunctionDebugInfo(); @@ -828,7 +828,7 @@ std::any IRGenerator::visitLambdaExpr(const LambdaExprNode *node) { // Pop capture addresses if (hasCaptures) for (const std::pair &capture : captures) - capture.second.capturedEntry->popAddress(); + capture.second.capturedSymbol->popAddress(); // Conclude debug info for function diGenerator.concludeFunctionDebugInfo(); @@ -873,16 +873,16 @@ llvm::Value *IRGenerator::buildFatFctPtr(Scope *bodyScope, llvm::Type *capturesS assert(captures.size() == 1); const Capture &capture = captures.begin()->second; if (capture.getMode() == BY_VALUE) { - llvm::Type *varType = capture.capturedEntry->getQualType().toLLVMType(sourceFile); - capturesPtr = insertLoad(varType, capture.capturedEntry->getAddress()); + llvm::Type *varType = capture.capturedSymbol->getQualType().toLLVMType(sourceFile); + capturesPtr = insertLoad(varType, capture.capturedSymbol->getAddress()); } else { - capturesPtr = capture.capturedEntry->getAddress(); + capturesPtr = capture.capturedSymbol->getAddress(); } } else { capturesPtr = insertAlloca(capturesStructType, CAPTURES_PARAM_NAME); size_t captureIdx = 0; for (const auto &[_, capture] : bodyScope->symbolTable.captures) { - const SymbolTableEntry *capturedEntry = capture.capturedEntry; + const SymbolTableEntry *capturedEntry = capture.capturedSymbol; // Get address or value of captured variable, depending on the capturing mode llvm::Value *capturedValue = capturedEntry->getAddress(); assert(capturedValue != nullptr); @@ -917,14 +917,14 @@ llvm::Type *IRGenerator::buildCapturesContainerType(const CaptureMap &captures) // If we have only one capture that is a ptr, we can just use that ptr type const Capture &capture = captures.begin()->second; - if (captures.size() == 1 && (capture.capturedEntry->getQualType().isPtr() || capture.getMode() == BY_REFERENCE)) + if (captures.size() == 1 && (capture.capturedSymbol->getQualType().isPtr() || capture.getMode() == BY_REFERENCE)) return builder.getPtrTy(); // Create captures struct type std::vector captureTypes; for (const auto &[_, c] : captures) { if (c.getMode() == BY_VALUE) - captureTypes.push_back(c.capturedEntry->getQualType().toLLVMType(sourceFile)); + captureTypes.push_back(c.capturedSymbol->getQualType().toLLVMType(sourceFile)); else captureTypes.push_back(builder.getPtrTy()); } @@ -935,10 +935,10 @@ void IRGenerator::unpackCapturesToLocalVariables(const CaptureMap &captures, llv assert(!captures.empty()); // If we have only one capture that is a ptr, we can just load the ptr const Capture &capture = captures.begin()->second; - if (captures.size() == 1 && (capture.capturedEntry->getQualType().isPtr() || capture.getMode() == BY_REFERENCE)) { + if (captures.size() == 1 && (capture.capturedSymbol->getQualType().isPtr() || capture.getMode() == BY_REFERENCE)) { // Interpret capturesPtr as ptr to the first and only capture llvm::Value *captureAddress = val; - capture.capturedEntry->pushAddress(captureAddress); + capture.capturedSymbol->pushAddress(captureAddress); // Generate debug info diGenerator.generateLocalVarDebugInfo(capture.getName(), captureAddress); } else { @@ -949,7 +949,7 @@ void IRGenerator::unpackCapturesToLocalVariables(const CaptureMap &captures, llv for (const auto &[name, c] : captures) { const std::string valueName = c.getMode() == BY_REFERENCE ? name + ".addr" : name; llvm::Value *captureAddress = insertStructGEP(structType, capturesPtr, captureIdx, valueName); - c.capturedEntry->pushAddress(captureAddress); + c.capturedSymbol->pushAddress(captureAddress); // Generate debug info diGenerator.generateLocalVarDebugInfo(c.getName(), captureAddress); captureIdx++; diff --git a/src/symboltablebuilder/Capture.cpp b/src/symboltablebuilder/Capture.cpp index fa3d8c41a..99953a8e2 100644 --- a/src/symboltablebuilder/Capture.cpp +++ b/src/symboltablebuilder/Capture.cpp @@ -4,7 +4,7 @@ namespace spice::compiler { -Capture::Capture(SymbolTableEntry *entry) : capturedEntry(entry) { +Capture::Capture(SymbolTableEntry *entry) : capturedSymbol(entry) { // Set the capture mode depending on the symbol type // All types with guaranteed size <= 64 bit are captured by value, all others by reference. captureMode = entry->getQualType().isOneOf({TY_STRUCT, TY_INTERFACE}) ? BY_REFERENCE : BY_VALUE; @@ -15,7 +15,7 @@ Capture::Capture(SymbolTableEntry *entry) : capturedEntry(entry) { * * @return Capture name or symbol name if no capture name was set */ -std::string Capture::getName() const { return capturedEntry->name; } +std::string Capture::getName() const { return capturedSymbol->name; } /** * Set the access type of this capture. @@ -36,7 +36,7 @@ void Capture::setAccessType(CaptureAccessType captureAccessType) { * * @return Capture mode */ -CaptureMode Capture::getMode() const { return captureMode; } +CapturePassMode Capture::getMode() const { return captureMode; } /** * Stringify the current capture to a human-readable form. Used to dump whole symbol tables with their contents. @@ -52,7 +52,7 @@ CaptureMode Capture::getMode() const { return captureMode; } */ nlohmann::ordered_json Capture::toJSON() const { nlohmann::json result; - result["name"] = capturedEntry->name; + result["name"] = capturedSymbol->name; result["accessType"] = accessType == READ_ONLY ? "READ_ONLY" : "READ_WRITE"; result["mode"] = captureMode == BY_VALUE ? "BY_VALUE" : "BY_REFERENCE"; return result; diff --git a/src/symboltablebuilder/Capture.h b/src/symboltablebuilder/Capture.h index d8471c0f2..bd8ecb81d 100644 --- a/src/symboltablebuilder/Capture.h +++ b/src/symboltablebuilder/Capture.h @@ -10,7 +10,7 @@ namespace spice::compiler { -enum CaptureMode : uint8_t { +enum CapturePassMode : uint8_t { BY_VALUE, BY_REFERENCE, }; @@ -28,16 +28,16 @@ class Capture { // Public methods [[nodiscard]] std::string getName() const; void setAccessType(CaptureAccessType captureAccessType); - [[nodiscard]] CaptureMode getMode() const; + [[nodiscard]] CapturePassMode getMode() const; [[nodiscard]] nlohmann::ordered_json toJSON() const; // Public members - SymbolTableEntry *capturedEntry; + SymbolTableEntry *capturedSymbol; private: // Members CaptureAccessType accessType = READ_ONLY; - CaptureMode captureMode = BY_VALUE; + CapturePassMode captureMode = BY_VALUE; }; } // namespace spice::compiler \ No newline at end of file diff --git a/src/symboltablebuilder/SymbolTable.cpp b/src/symboltablebuilder/SymbolTable.cpp index 447b51ffc..e3709e507 100644 --- a/src/symboltablebuilder/SymbolTable.cpp +++ b/src/symboltablebuilder/SymbolTable.cpp @@ -124,7 +124,7 @@ SymbolTableEntry *SymbolTable::lookupStrict(const std::string &name) { return &symbols.at(name); // Check if a capture with this name exists in this scope if (captures.contains(name)) - return captures.at(name).capturedEntry; + return captures.at(name).capturedSymbol; // Otherwise, return a nullptr return nullptr; } diff --git a/src/symboltablebuilder/TypeSpecifiers.cpp b/src/symboltablebuilder/TypeSpecifiers.cpp index aa3f5a6ad..2ed0f4fa1 100644 --- a/src/symboltablebuilder/TypeSpecifiers.cpp +++ b/src/symboltablebuilder/TypeSpecifiers.cpp @@ -3,10 +3,16 @@ #include "TypeSpecifiers.h" #include -#include +#include namespace spice::compiler { +/** + * Get default type specifiers for a given super type + * + * @param superType Super type + * @return Default type specifiers + */ TypeSpecifiers TypeSpecifiers::of(uint16_t superType) { switch (superType) { case TY_DOUBLE: // fall-through @@ -14,30 +20,27 @@ TypeSpecifiers TypeSpecifiers::of(uint16_t superType) { case TY_SHORT: // fall-through case TY_LONG: return {/*const*/ false, /*signed*/ true, /*unsigned*/ false, /*heap*/ false}; - case TY_BYTE: // fall-through - case TY_CHAR: // fall-through - case TY_STRING: // fall-through - case TY_BOOL: // fall-through - case TY_PTR: // fall-through - case TY_REF: // fall-through - case TY_ARRAY: // fall-through + case TY_BYTE: // fall-through + case TY_CHAR: // fall-through + case TY_STRING: // fall-through + case TY_BOOL: // fall-through + case TY_PTR: // fall-through + case TY_REF: // fall-through + case TY_ARRAY: // fall-through + case TY_STRUCT: // fall-through + case TY_INTERFACE: // fall-through + case TY_FUNCTION: // fall-through + case TY_PROCEDURE: return {/*const*/ false, /*signed*/ false, /*unsigned*/ true, /*heap*/ false}; case TY_GENERIC: // Generics must be non-signed and non-unsigned at the same time to ensure a proper function matching return {/*const*/ false, /*signed*/ false, /*unsigned*/ false, /*heap*/ false}; - case TY_STRUCT: // fall-through - case TY_INTERFACE: - return {/*const*/ false, /*signed*/ false, /*unsigned*/ true, /*heap*/ false}; - case TY_ENUM: // fall-through - case TY_ALIAS: - return {/*const*/ true, /*signed*/ false, /*unsigned*/ true, /*heap*/ false}; - case TY_FUNCTION: // fall-through - case TY_PROCEDURE: - return {/*const*/ false, /*signed*/ false, /*unsigned*/ true, /*heap*/ false}; + case TY_ENUM: // fall-through + case TY_ALIAS: // fall-through case TY_IMPORT: return {/*const*/ true, /*signed*/ false, /*unsigned*/ true, /*heap*/ false}; - case TY_DYN: - case TY_INVALID: + case TY_DYN: // fall-through + case TY_INVALID: // fall-through case TY_UNRESOLVED: // Return all-false specifiers to not match anything return {/*const*/ false, /*signed*/ false, /*unsigned*/ false, /*heap*/ false}; @@ -47,7 +50,7 @@ TypeSpecifiers TypeSpecifiers::of(uint16_t superType) { } /** - * Merge two type specifiers. If possible, prefer the opposite of the default of the super type + * Merge two sets of type specifiers. If possible, prefer the opposite of the default of the super type * * @param other Other type specifiers object * @return Merged specifiers object @@ -69,14 +72,14 @@ TypeSpecifiers TypeSpecifiers::merge(const TypeSpecifiers &other) const { } /** - * Check if two type specifiers match + * Check if two sets of type specifiers match * * @param otherSpecifiers The rhs specifiers * @param allowConstify Match when the types are the same, but the lhs type is more const restrictive than the rhs type * @return Matching or not */ bool TypeSpecifiers::match(TypeSpecifiers otherSpecifiers, bool allowConstify) const { - TypeSpecifiers thisSpecifiers = *this; + const TypeSpecifiers thisSpecifiers = *this; // If allowConstify is enabled, only allow to match lhs=const and rhs=non-const if (allowConstify && thisSpecifiers.isConst && !otherSpecifiers.isConst) diff --git a/src/typechecker/TypeChecker.cpp b/src/typechecker/TypeChecker.cpp index 383af3183..87cd4b3b4 100644 --- a/src/typechecker/TypeChecker.cpp +++ b/src/typechecker/TypeChecker.cpp @@ -1900,10 +1900,11 @@ bool TypeChecker::visitMethodCall(FctCallNode *node, Scope *structScope, QualTyp // Retrieve field entry SymbolTableEntry *fieldEntry = structScope->lookupStrict(identifier); - if (!fieldEntry) + if (!fieldEntry) { + const std::string name = data.thisType.getBase().getName(false, true); SOFT_ERROR_BOOL(node, ACCESS_TO_NON_EXISTING_MEMBER, - "The type " + data.thisType.getBase().getName(false) + " does not have a member with the name '" + - identifier + "'") + "The type '" + name + "' does not have a member with the name '" + identifier + "'") + } if (!fieldEntry->getQualType().getBase().isOneOf({TY_STRUCT, TY_INTERFACE})) SOFT_ERROR_BOOL(node, INVALID_MEMBER_ACCESS, "Cannot call a method on '" + identifier + "', since it is no struct or interface") @@ -2519,7 +2520,7 @@ bool TypeChecker::checkAsyncLambdaCaptureRules(LambdaBaseNode *node, const Lambd // Check for the capture rules const Capture &capture = captures.begin()->second; - if (captures.size() > 1 || !capture.capturedEntry->getQualType().isPtr() || capture.getMode() != BY_VALUE) { + if (captures.size() > 1 || !capture.capturedSymbol->getQualType().isPtr() || capture.getMode() != BY_VALUE) { const char *warningMessage = "Async lambdas can only capture one pointer by value without storing captures in the caller stack frame, which can lead " "to bugs due to references, outliving the validity scope of the referenced variable."; diff --git a/src/util/CommonUtil.cpp b/src/util/CommonUtil.cpp index d9d2f5fca..4b5d30cc0 100644 --- a/src/util/CommonUtil.cpp +++ b/src/util/CommonUtil.cpp @@ -106,7 +106,7 @@ bool CommonUtil::isValidMangledName(const std::string &mangledName) { /** * Generate a circular import message from the given source files * - * @param sourceFiles Source files building the circular dependency chain + * @param sourceFiles Source files that form the circular dependency chain * @return Error message */ std::string CommonUtil::getCircularImportMessage(const std::vector &sourceFiles) { diff --git a/test/test-files/typechecker/methods/error-this-field-not-existing/exception.out b/test/test-files/typechecker/methods/error-this-field-not-existing/exception.out index 80fcd2a90..891b924c3 100644 --- a/test/test-files/typechecker/methods/error-this-field-not-existing/exception.out +++ b/test/test-files/typechecker/methods/error-this-field-not-existing/exception.out @@ -2,7 +2,7 @@ Unresolved soft errors: There are unresolved errors. Please fix them and recompile. [Error|Semantic] ./source.spice:9:5: -Access to non-existing member: The type TestStruct does not have a member with the name 'nonExisting' +Access to non-existing member: The type 'TestStruct' does not have a member with the name 'nonExisting' 9 ts.f3.f3.nonExisting.testFunc(); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ \ No newline at end of file