From 49f511d394fb30cc105a946eb3ea1de7cfcdc88b Mon Sep 17 00:00:00 2001 From: Quentin Sabah Date: Wed, 26 Jun 2024 14:17:36 +0200 Subject: [PATCH] robustify the debug and error reports (#2495) * fix crash in error report on empty source location * robustify the type-analysis debug report The type-analysis debug report used to crash on various assertions because it assumed a well-typed program. However the first type-analysis runs before the semantics checker, hence the program may be ill-typed at that time. With this change the type-analysis debug report printer makes no assumptions about the expected types, instead it prints the infered types. --- .github/workflows/CI-Tests.yml | 2 +- CMakeLists.txt | 1 + cmake/SouffleTests.cmake | 7 +- src/ast/analysis/typesystem/Type.cpp | 122 ++++++++--------------- src/ast/analysis/typesystem/Type.h | 12 ++- src/ast/analysis/typesystem/TypeSystem.h | 8 +- src/ast/tests/type_system_test.cpp | 16 +-- src/ast/transform/TypeChecker.cpp | 2 +- src/parser/SrcLocation.cpp | 7 ++ src/parser/SrcLocation.h | 2 + src/reports/ErrorReport.h | 2 +- tests/evaluation/CMakeLists.txt | 2 +- tests/example/euclid/euclid.dl | 6 +- tests/example/euclid/euclid.err | 7 +- 14 files changed, 85 insertions(+), 111 deletions(-) diff --git a/.github/workflows/CI-Tests.yml b/.github/workflows/CI-Tests.yml index f8b865f1034..b5f48c75b6b 100644 --- a/.github/workflows/CI-Tests.yml +++ b/.github/workflows/CI-Tests.yml @@ -67,7 +67,7 @@ jobs: if: ${{ matrix.domain == '32bit' }} uses: ./.github/actions/cmake-test with: - cmake-flags: -DSOUFFLE_CODE_COVERAGE=ON + cmake-flags: -DSOUFFLE_CODE_COVERAGE=ON -DSOUFFLE_TEST_DEBUG_REPORT=ON n-chunks: ${{ needs.Test-Setup.outputs.n-chunks }} chunk: ${{ matrix.chunk }} diff --git a/CMakeLists.txt b/CMakeLists.txt index 5ab514d43d3..7aeef703af9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -104,6 +104,7 @@ option(SOUFFLE_CODE_COVERAGE "Enable coverage reporting" OFF) option(SOUFFLE_BASH_COMPLETION "Enable/Disable bash completion" OFF) option(SOUFFLE_USE_LIBFFI "Enable/Disable use of libffi" ON) option(SOUFFLE_CUSTOM_GETOPTLONG "Enable/Disable custom getopt_long implementation" OFF) +option(SOUFFLE_TEST_DEBUG_REPORT "Enable/Disable generating debug-report for all tests" OFF) cmake_dependent_option(SOUFFLE_USE_LIBCPP "Link to libc++ instead of libstdc++" ON "CMAKE_CXX_COMPILER_ID STREQUAL Clang" OFF) diff --git a/cmake/SouffleTests.cmake b/cmake/SouffleTests.cmake index 2162f9b7fbe..d918b430ad8 100644 --- a/cmake/SouffleTests.cmake +++ b/cmake/SouffleTests.cmake @@ -159,9 +159,10 @@ function(SOUFFLE_RUN_TEST_HELPER) #and input paths #PARAM_FACTS_DIR_NAME - the name of the "facts" subdirectory in each test. #Usually just "facts" but can be different when running multi - tests +#PARAM_DEBUG_REPORT - dump a debug report cmake_parse_arguments( PARAM - "COMPILED;COMPILED_SPLITTED;FUNCTORS;NEGATIVE;MULTI_TEST;NO_PREPROCESSOR;OUTPUT_STDOUT" # Options + "COMPILED;COMPILED_SPLITTED;FUNCTORS;NEGATIVE;MULTI_TEST;NO_PREPROCESSOR;OUTPUT_STDOUT;DEBUG_REPORT" # Options "TEST_NAME;CATEGORY;FACTS_DIR_NAME;EXTRA_DATA" #Single valued options "INCLUDE_DIRS" # Multi-valued options ${ARGV} @@ -182,6 +183,10 @@ function(SOUFFLE_RUN_TEST_HELPER) set(SHORT_EXEC_STYLE "") endif() + if (PARAM_DEBUG_REPORT OR SOUFFLE_TEST_DEBUG_REPORT) + list(APPEND EXTRA_FLAGS "--debug-report=dbg.html") + endif() + if (PARAM_NO_PREPROCESSOR) list(APPEND EXTRA_FLAGS "--no-preprocessor") else () diff --git a/src/ast/analysis/typesystem/Type.cpp b/src/ast/analysis/typesystem/Type.cpp index b50ae83ca6e..6cda7d316a1 100644 --- a/src/ast/analysis/typesystem/Type.cpp +++ b/src/ast/analysis/typesystem/Type.cpp @@ -21,6 +21,7 @@ #include "ast/Aggregator.h" #include "ast/Atom.h" #include "ast/BinaryConstraint.h" +#include "ast/BooleanConstraint.h" #include "ast/BranchInit.h" #include "ast/Clause.h" #include "ast/IntrinsicAggregator.h" @@ -557,11 +558,10 @@ void TypeAnalysis::run(const TranslationUnit& translationUnit) { this->translationUnit = &translationUnit; } -void TypeAnnotationPrinter::branchOnArgument(const Argument* cur, const Type& type) { +void TypeAnnotationPrinter::branchOnArgument(const Argument* cur) { if (isA(*cur)) { - auto var = as(cur); - auto inferedTypeName = argumentTypes.find(var)->second; - os << *as(cur) << "∈" << inferedTypeName; + auto inferedTypeName = argumentTypes.find(cur)->second; + os << *cur << "∈" << inferedTypeName; } else if (isA(*cur)) { os << "_"; } else if (isA(*cur)) { @@ -571,7 +571,7 @@ void TypeAnnotationPrinter::branchOnArgument(const Argument* cur, const Type& ty } else if (isA(*cur)) { print_(type_identity(), *as(cur)); } else if (isA(*cur)) { - print_(type_identity(), *as(cur), *as(getBaseType(&type))); + print_(type_identity(), *as(cur)); } else if (isA(*cur)) { print_(type_identity(), *as(cur)); } else if (isA(*cur)) { @@ -595,18 +595,10 @@ void TypeAnnotationPrinter::print_(type_identity, const Atom& atom) { auto name = atom.getQualifiedName(); os << name << "("; auto args = atom.getArguments(); - auto rel = program.getRelation(atom); - auto atts = rel->getAttributes(); std::size_t i = 0; for (auto cur : args) { - const auto& declaredTypeName = atts[i]->getTypeName(); - const auto& declaredType = typeEnv.getType(declaredTypeName); - assert(typeEnv.isType(declaredTypeName)); - branchOnArgument(cur, declaredType); - if (isA(cur) || isA(cur) || isA(cur)) { - os << "∈{" << declaredTypeName << "}"; - } + branchOnArgument(cur); if (i + 1 < args.size()) { os << ","; } @@ -644,76 +636,52 @@ void TypeAnnotationPrinter::print_(type_identity, const Numeric } } -void TypeAnnotationPrinter::print_(type_identity, const BinaryConstraint& rel) { - auto lhs = rel.getLHS(); - auto lTySet = argumentTypes.find(lhs)->second; - assert(lTySet.size() == 1); - auto lTy = lTySet.begin(); - branchOnArgument(lhs, *lTy); - if (isA(lhs) || isA(lhs)) { - os << "∈{" << lTy->getName() << "}"; - } - - os << " " << rel.getBaseOperator() << " "; +void TypeAnnotationPrinter::print_(type_identity, const BinaryConstraint& bin) { + auto lhs = bin.getLHS(); + auto rhs = bin.getRHS(); + branchOnArgument(lhs); + os << " " << bin.getBaseOperator() << " "; + branchOnArgument(rhs); +} - auto rhs = rel.getRHS(); - auto rTySet = argumentTypes.find(rhs)->second; - assert(rTySet.size() == 1); - auto rTy = rTySet.begin(); - branchOnArgument(rhs, *rTy); - if (isA(rhs) || isA(rhs)) { - os << "∈{" << rTy->getName() << "}"; - } +void TypeAnnotationPrinter::print_(type_identity, const BooleanConstraint& c) { + os << c; } void TypeAnnotationPrinter::print_(type_identity, const IntrinsicFunctor& fun) { auto arguments = fun.getArguments(); if (arguments.size() == 2) { // binary - auto tySet = argumentTypes.find(arguments[0])->second; - assert(tySet.size() == 1); - auto ty = tySet.begin(); os << "("; - branchOnArgument(arguments[0], *ty); - if (isA(arguments[0]) || isA(arguments[0])) { - os << "∈{" << ty->getName() << "}"; - } - + branchOnArgument(arguments[0]); os << " " << fun.getBaseFunctionOp() << " "; - - auto tySet2 = argumentTypes.find(arguments[1])->second; - assert(tySet2.size() == 1); - auto ty2 = tySet2.begin(); - branchOnArgument(arguments[1], *ty2); - if (isA(arguments[1]) || isA(arguments[1])) { - os << "∈{" << ty2->getName() << "}"; - } - + branchOnArgument(arguments[1]); os << ")"; } else { os << fun.getBaseFunctionOp() << "("; for (std::size_t i = 0; i < arguments.size(); ++i) { - TypeAttribute argType = typeAnalysis.getFunctorParamTypeAttribute(fun, i); - auto& ty = typeEnv.getConstantType(argType); - branchOnArgument(arguments[i], ty); + branchOnArgument(arguments[i]); if (i + 1 < arguments.size()) { os << ","; } } os << ")"; } + const TypeSet& ts = argumentTypes.at(&fun); + os << "∈" << ts; } void TypeAnnotationPrinter::print_(type_identity, const UserDefinedFunctor& fun) { auto arguments = fun.getArguments(); os << "@" << fun.getName() << "("; for (std::size_t i = 0; i < arguments.size(); ++i) { - const auto& ty = typeAnalysis.getFunctorParamType(fun, i); - branchOnArgument(arguments[i], ty); + branchOnArgument(arguments[i]); if (i + 1 < arguments.size()) { os << ","; } } os << ")"; + const TypeSet& ts = argumentTypes.at(&fun); + os << "∈" << ts; } void TypeAnnotationPrinter::print_(type_identity, [[maybe_unused]] const Counter& counter) { @@ -727,52 +695,39 @@ void TypeAnnotationPrinter::print_( void TypeAnnotationPrinter::print_(type_identity, const ast::TypeCast& typeCast) { os << "as("; - auto& ty = typeEnv.getType(typeCast.getType()); - branchOnArgument(typeCast.getValue(), ty); - os << "," << ty.getName() << ")"; + branchOnArgument(typeCast.getValue()); + os << "," << typeCast.getType() << ")"; + const TypeSet& ts = argumentTypes.at(&typeCast); + os << "∈" << ts; } -void TypeAnnotationPrinter::print_( - type_identity, const RecordInit& record, const RecordType& type) { +void TypeAnnotationPrinter::print_(type_identity, const RecordInit& record) { auto arguments = record.getArguments(); - const auto& ftypes = type.getFields(); os << "["; for (std::size_t i = 0; i < arguments.size(); ++i) { - branchOnArgument(arguments[i], *ftypes[i]); - if (isA(arguments[i]) || isA(arguments[i])) { - os << "∈{" << (*ftypes[i]).getName() << "}"; - } + branchOnArgument(arguments[i]); if (i + 1 < arguments.size()) { os << ","; } } os << "]"; + const TypeSet& ts = argumentTypes.at(&record); + os << "∈" << ts; } void TypeAnnotationPrinter::print_(type_identity, const BranchInit& adt) { - auto* correspondingType = sumTypesBranches.getType(adt.getBranchName()); - - assert(correspondingType != nullptr); - assert(isA(correspondingType)); - - auto branchTypes = as(correspondingType)->getBranchTypes(adt.getBranchName()); auto branchArgs = adt.getArguments(); - assert(branchTypes.size() == branchArgs.size()); - os << "$" << adt.getBranchName() << "("; for (std::size_t i = 0; i < branchArgs.size(); i++) { - auto arg = branchArgs[i]; - auto argTy = branchTypes[i]; - branchOnArgument(arg, *argTy); - if (isA(arg) || isA(arg)) { - os << "∈{" << argTy->getName() << "}"; - } + branchOnArgument(branchArgs[i]); if (i + 1 < branchArgs.size()) { os << ", "; } } os << ")"; + const TypeSet& ts = argumentTypes.at(&adt); + os << "∈" << ts; } void TypeAnnotationPrinter::print_(type_identity, const Aggregator& agg) { @@ -781,14 +736,13 @@ void TypeAnnotationPrinter::print_(type_identity, const Aggregator& os << baseOperator << " "; auto targetExpr = agg.getTargetExpression(); if (targetExpr /* the target expression can be null */) { - auto tySet = argumentTypes.find(targetExpr)->second; - assert(tySet.size() == 1); - auto ty = tySet.begin(); - branchOnArgument(targetExpr, *ty); + branchOnArgument(targetExpr); } os << " : { "; printBodyLiterals(bodyLiterals, " "); os << " }"; + const TypeSet& ts = argumentTypes.at(&agg); + os << "∈" << ts; } void TypeAnnotationPrinter::printBodyLiterals(std::vector bodyLiterals, const std::string& spc) { @@ -800,6 +754,8 @@ void TypeAnnotationPrinter::printBodyLiterals(std::vector bodyLiterals print_(type_identity(), *as(*cur)); } else if (isA(*cur)) { print_(type_identity(), *as(*cur)); + } else if (isA(*cur)) { + print_(type_identity(), *as(*cur)); } else { os << "(?)"; } diff --git a/src/ast/analysis/typesystem/Type.h b/src/ast/analysis/typesystem/Type.h index ac878288cae..73ef1e54fdc 100644 --- a/src/ast/analysis/typesystem/Type.h +++ b/src/ast/analysis/typesystem/Type.h @@ -18,6 +18,7 @@ #include "AggregateOp.h" #include "FunctorOps.h" +#include "ast/BooleanConstraint.h" #include "ast/Clause.h" #include "ast/NumericConstant.h" #include "ast/TranslationUnit.h" @@ -163,7 +164,7 @@ class TypeAnalysis : public Analysis { */ class TypeAnnotationPrinter { public: - TypeAnnotationPrinter(const TranslationUnit* tu, const std::map argumentTypes, + TypeAnnotationPrinter(const TranslationUnit* tu, const std::map& argumentTypes, std::ostream& os) : tu(tu), argumentTypes(argumentTypes), os(os) {} @@ -177,24 +178,25 @@ class TypeAnnotationPrinter { const SumTypeBranchesAnalysis& sumTypesBranches = tu->getAnalysis(); const TypeAnalysis& typeAnalysis = tu->getAnalysis(); - std::map argumentTypes; + const std::map& argumentTypes; std::ostream& os; - void branchOnArgument(const Argument*, const Type&); + void branchOnArgument(const Argument*); void print_(type_identity, const Atom& atom); void print_(type_identity, const Negation& cur); void print_(type_identity, const StringConstant& cnst); void print_(type_identity, const NumericConstant& constant); void print_(type_identity, const NilConstant& constant); - void print_(type_identity, const BinaryConstraint& rel); + void print_(type_identity, const BinaryConstraint& bin); void print_(type_identity, const IntrinsicFunctor& fun); void print_(type_identity, const UserDefinedFunctor& fun); void print_(type_identity, const Counter& counter); void print_(type_identity, const IterationCounter& counter); void print_(type_identity, const ast::TypeCast& typeCast); - void print_(type_identity, const RecordInit& record, const RecordType&); + void print_(type_identity, const RecordInit& record); void print_(type_identity, const BranchInit& adt); void print_(type_identity, const Aggregator& agg); + void print_(type_identity, const BooleanConstraint& c); void printBodyLiterals(std::vector literals, const std::string& spc); }; diff --git a/src/ast/analysis/typesystem/TypeSystem.h b/src/ast/analysis/typesystem/TypeSystem.h index cba1fd8c73d..a306030f1dd 100644 --- a/src/ast/analysis/typesystem/TypeSystem.h +++ b/src/ast/analysis/typesystem/TypeSystem.h @@ -364,12 +364,12 @@ class AlgebraicDataType : public Type { [](const Branch& left, const Branch& right) { return left.name.lexicalLess(right.name); }); } - const std::vector& getBranchTypes(const QualifiedName& name) const { + std::vector getBranchTypes(const QualifiedName& name) const { for (auto& branch : branches) { if (branch.name == name) return branch.types; } // Branch doesn't exist. - throw std::out_of_range("Trying to access non-existing branch."); + return {}; } /** Return the branches as a sorted vector */ @@ -523,7 +523,9 @@ struct TypeSet { /** Print type set */ void print(std::ostream& out) const { if (all) { - out << "{ - all types - }"; + out << "⊤"; + } else if (types.empty()) { + out << "∅"; } else { out << "{" << join(types, ",", [](std::ostream& out, const Type* type) { out << type->getName(); }) diff --git a/src/ast/tests/type_system_test.cpp b/src/ast/tests/type_system_test.cpp index c3f49eb936b..d1691c4b33c 100644 --- a/src/ast/tests/type_system_test.cpp +++ b/src/ast/tests/type_system_test.cpp @@ -173,11 +173,11 @@ TEST(TypeSystem, GreatestCommonSubtype) { EXPECT_EQ("{B}", toString(getGreatestCommonSubtypes(B, B))); EXPECT_EQ("{C}", toString(getGreatestCommonSubtypes(C, C))); - EXPECT_EQ("{}", toString(getGreatestCommonSubtypes(A, B))); - EXPECT_EQ("{}", toString(getGreatestCommonSubtypes(A, C))); - EXPECT_EQ("{}", toString(getGreatestCommonSubtypes(B, C))); + EXPECT_EQ("∅", toString(getGreatestCommonSubtypes(A, B))); + EXPECT_EQ("∅", toString(getGreatestCommonSubtypes(A, C))); + EXPECT_EQ("∅", toString(getGreatestCommonSubtypes(B, C))); - EXPECT_EQ("{}", toString(getGreatestCommonSubtypes(A, B, C))); + EXPECT_EQ("∅", toString(getGreatestCommonSubtypes(A, B, C))); EXPECT_EQ("{A}", toString(getGreatestCommonSubtypes(A, N))); EXPECT_EQ("{A}", toString(getGreatestCommonSubtypes(N, A))); @@ -185,8 +185,8 @@ TEST(TypeSystem, GreatestCommonSubtype) { EXPECT_EQ("{B}", toString(getGreatestCommonSubtypes(B, N))); EXPECT_EQ("{B}", toString(getGreatestCommonSubtypes(N, B))); - EXPECT_EQ("{}", toString(getGreatestCommonSubtypes(C, N))); - EXPECT_EQ("{}", toString(getGreatestCommonSubtypes(N, C))); + EXPECT_EQ("∅", toString(getGreatestCommonSubtypes(C, N))); + EXPECT_EQ("∅", toString(getGreatestCommonSubtypes(N, C))); // // bring in unions @@ -242,8 +242,8 @@ TEST(TypeSystem, complexSubsetTypes) { EXPECT_EQ("{B}", toString(getGreatestCommonSubtypes(A, BfromA))); EXPECT_EQ("{C}", toString(getGreatestCommonSubtypes(A, CfromA))); - EXPECT_EQ("{}", toString(getGreatestCommonSubtypes(A, BfromA, CfromA))); - EXPECT_EQ("{}", toString(getGreatestCommonSubtypes(BfromA, CfromA))); + EXPECT_EQ("∅", toString(getGreatestCommonSubtypes(A, BfromA, CfromA))); + EXPECT_EQ("∅", toString(getGreatestCommonSubtypes(BfromA, CfromA))); auto* base = &env.createType(qn("B0"), BfromA); for (std::size_t i = 1; i <= 10; ++i) { diff --git a/src/ast/transform/TypeChecker.cpp b/src/ast/transform/TypeChecker.cpp index a85ce330fa3..1589fda51df 100644 --- a/src/ast/transform/TypeChecker.cpp +++ b/src/ast/transform/TypeChecker.cpp @@ -495,7 +495,7 @@ void TypeCheckerImpl::visit_(type_identity, const BranchInit& adt) { // We know now that the set "types" is a singleton auto& sumType = *as(*types.begin()); - auto& argsDeclaredTypes = sumType.getBranchTypes(adt.getBranchName()); + auto argsDeclaredTypes = sumType.getBranchTypes(adt.getBranchName()); auto args = adt.getArguments(); diff --git a/src/parser/SrcLocation.cpp b/src/parser/SrcLocation.cpp index 69c9520ef64..6e375fdcdf7 100644 --- a/src/parser/SrcLocation.cpp +++ b/src/parser/SrcLocation.cpp @@ -88,6 +88,9 @@ void SrcLocation::setFile(const std::shared_ptr& f) { } std::string SrcLocation::extloc() const { + if (!isValid()) { + return ":0:0"; + } std::ifstream in(file->Reported); std::stringstream s; if (in.is_open()) { @@ -135,6 +138,10 @@ void SrcLocation::print(std::ostream& out) const { out << getReportedFilename() << " [" << start << "-" << end << "]"; } +bool SrcLocation::isValid() const { + return (bool)file; +} + void ScannerInfo::push( const std::filesystem::path& Physical, const SrcLocation& IncludeLoc, bool reducedWhitespaces) { auto NewFile = std::make_shared( diff --git a/src/parser/SrcLocation.h b/src/parser/SrcLocation.h index 44dc0104fde..cf1b4032afa 100644 --- a/src/parser/SrcLocation.h +++ b/src/parser/SrcLocation.h @@ -132,6 +132,8 @@ class SrcLocation { range.print(out); return out; } + + bool isValid() const; }; enum class CommentKind { diff --git a/src/reports/ErrorReport.h b/src/reports/ErrorReport.h index cb820048da8..29d86e4c2dd 100644 --- a/src/reports/ErrorReport.h +++ b/src/reports/ErrorReport.h @@ -33,7 +33,7 @@ namespace souffle { class DiagnosticMessage { public: DiagnosticMessage(std::string message, SrcLocation location) - : message(std::move(message)), hasLoc(true), location(std::move(location)) {} + : message(std::move(message)), hasLoc(location.isValid()), location(std::move(location)) {} DiagnosticMessage(std::string message) : message(std::move(message)), hasLoc(false) {} diff --git a/tests/evaluation/CMakeLists.txt b/tests/evaluation/CMakeLists.txt index 7d7898b3230..15baa64f285 100644 --- a/tests/evaluation/CMakeLists.txt +++ b/tests/evaluation/CMakeLists.txt @@ -26,7 +26,7 @@ positive_test(aggregates_nested) positive_test(aggregates_non_materialised) positive_test(aggregates7) positive_test(aggregate_witnesses) -positive_test(aliases) +positive_test(aliases DEBUG_REPORT) positive_test(arithm) positive_test(average) positive_test(bad_regex) diff --git a/tests/example/euclid/euclid.dl b/tests/example/euclid/euclid.dl index 973a2338945..0ca88f236cf 100644 --- a/tests/example/euclid/euclid.dl +++ b/tests/example/euclid/euclid.dl @@ -70,9 +70,11 @@ Simplify([head,tail],[newHead,newTail]) :- // Helper method for simplify ToGcdInt(numerator,denominator) :- ToSimplify([head,tail]), - Simplify(tail,newTail), head = [numerator,denominator] , - (newTail != nil; numerator != 0). + ( Simplify(tail, newTail), + newTail != nil + ; numerator != 0 + ). // -- Add polynomials recursively -- // diff --git a/tests/example/euclid/euclid.err b/tests/example/euclid/euclid.err index 5f576e3da39..1284c383b6e 100644 --- a/tests/example/euclid/euclid.err +++ b/tests/example/euclid/euclid.err @@ -1,12 +1,9 @@ Warning: Variable denominator only occurs once in file euclid.dl at line 61 head = [0,denominator]. --------------^------------- -Warning: Variable newTail only occurs once in file euclid.dl at line 73 - Simplify(tail,newTail), -------------------^--------- -Warning: Variable denominator only occurs once in file euclid.dl at line 128 +Warning: Variable denominator only occurs once in file euclid.dl at line 130 head = [0,denominator]. --------------^------------- -Warning: Variable denominator only occurs once in file euclid.dl at line 132 +Warning: Variable denominator only occurs once in file euclid.dl at line 134 head = [numerator,denominator], numerator!=0. ----------------------^---------------------------