From 06f0effb848c6b938e03da8a61e44b3d032380e8 Mon Sep 17 00:00:00 2001 From: Tim Foley Date: Fri, 26 Jan 2018 15:30:23 -0800 Subject: Fix handling of errors in imported modules (#387) * Fix handling of errors in imported modules - If a semantic error is detected in an imported module, then don't try to generate IR code for it - Also, if a module (transitively) imports itself, then report that as an error - The way I'm checking for this is a bit hacky (I'm adding the module to the map of loaded modules, but in an "unfinished" state, and then using that unfinished state to detect the import of a module already being imported). This isn't a 100% complete solution for any of the related problems, but it improves the user experience for the common case. * Remove #import test. The feature is slated to be removed, so it isn't worth fixing up this test case. --- source/slang/diagnostic-defs.h | 2 ++ source/slang/slang.cpp | 36 +++++++++++++++++++---- tests/bugs/import-with-error-extra.slang | 11 +++++++ tests/bugs/import-with-error.slang | 11 +++++++ tests/bugs/import-with-error.slang.expected | 6 ++++ tests/diagnostics/recursive-import-extra.slang | 6 ++++ tests/diagnostics/recursive-import.slang | 6 ++++ tests/diagnostics/recursive-import.slang.expected | 6 ++++ tests/preprocessor/import.hlsl | 18 ------------ tests/preprocessor/import.slang.h | 12 -------- 10 files changed, 78 insertions(+), 36 deletions(-) create mode 100644 tests/bugs/import-with-error-extra.slang create mode 100644 tests/bugs/import-with-error.slang create mode 100644 tests/bugs/import-with-error.slang.expected create mode 100644 tests/diagnostics/recursive-import-extra.slang create mode 100644 tests/diagnostics/recursive-import.slang create mode 100644 tests/diagnostics/recursive-import.slang.expected delete mode 100644 tests/preprocessor/import.hlsl delete mode 100644 tests/preprocessor/import.slang.h diff --git a/source/slang/diagnostic-defs.h b/source/slang/diagnostic-defs.h index bae501a3c..f8b09a196 100644 --- a/source/slang/diagnostic-defs.h +++ b/source/slang/diagnostic-defs.h @@ -257,6 +257,8 @@ DIAGNOSTIC(38102, Error, accessorMustBeInsideSubscriptOrProperty, "an accessor d DIAGNOSTIC(38020, Error, mismatchEntryPointTypeArgument, "expecting $0 entry-point type arguments, provided $1.") DIAGNOSTIC(38021, Error, typeArgumentDoesNotConformToInterface, "type argument `$1` for generic parameter `$0` does not conform to interface `$1`.") +DIAGNOSTIC(38200, Error, recursiveModuleImport, "module `$0` recursively imports itself") + // // 4xxxx - IL code generation. // diff --git a/source/slang/slang.cpp b/source/slang/slang.cpp index e4bf251fc..773b1dd09 100644 --- a/source/slang/slang.cpp +++ b/source/slang/slang.cpp @@ -499,16 +499,31 @@ void CompileRequest::loadParsedModule( Name* name, String const& path) { + // Note: we add the loaded module to our name->module listing + // before doing semantic checking, so that if it tries to + // recursively `import` itself, we can detect it. + RefPtr loadedModule = new LoadedModule(); + mapPathToLoadedModule.Add(path, loadedModule); + mapNameToLoadedModules.Add(name, loadedModule); + + int errorCountBefore = mSink.GetErrorCount(); checkTranslationUnit(translationUnit.Ptr()); + int errorCountAfter = mSink.GetErrorCount(); RefPtr moduleDecl = translationUnit->SyntaxNode; - - RefPtr loadedModule = new LoadedModule(); loadedModule->moduleDecl = moduleDecl; - loadedModule->irModule = generateIRForTranslationUnit(translationUnit); - mapPathToLoadedModule.Add(path, loadedModule); - mapNameToLoadedModules.Add(name, loadedModule); + if (errorCountAfter != errorCountBefore) + { + // There must have been an error in the loaded module. + } + else + { + // If we didn't run into any errors, then try to generate + // IR code for the imported module. + loadedModule->irModule = generateIRForTranslationUnit(translationUnit); + } + loadedModulesList.Add(loadedModule); } @@ -591,7 +606,16 @@ RefPtr CompileRequest::findOrImportModule( // If so, return it. RefPtr loadedModule; if (mapNameToLoadedModules.TryGetValue(name, loadedModule)) - return loadedModule->moduleDecl; + { + if (!loadedModule) + return nullptr; + + if (!loadedModule->moduleDecl) + { + // We seem to be in the middle of loading this module + mSink.diagnose(loc, Diagnostics::recursiveModuleImport, name); + } + } // Derive a file name for the module, by taking the given // identifier, replacing all occurences of `_` with `-`, diff --git a/tests/bugs/import-with-error-extra.slang b/tests/bugs/import-with-error-extra.slang new file mode 100644 index 000000000..259f6b5d1 --- /dev/null +++ b/tests/bugs/import-with-error-extra.slang @@ -0,0 +1,11 @@ +//TEST_IGNORE_FILE: + +// This file exists to be imported. +// +// It contains a semantic error that should lead to compilation failure +// in the module doing the importing. + +void broken() +{ + int a = b; +} diff --git a/tests/bugs/import-with-error.slang b/tests/bugs/import-with-error.slang new file mode 100644 index 000000000..f50bb1adb --- /dev/null +++ b/tests/bugs/import-with-error.slang @@ -0,0 +1,11 @@ +//TEST:SIMPLE:-use-ir + +// Confirm that we correctly issue a diagnostic when +// we `import` a module that has some errors in it. + +import import_with_error_extra; + +void main() +{ + broken(); +} diff --git a/tests/bugs/import-with-error.slang.expected b/tests/bugs/import-with-error.slang.expected new file mode 100644 index 000000000..b71dba0f1 --- /dev/null +++ b/tests/bugs/import-with-error.slang.expected @@ -0,0 +1,6 @@ +result code = -1 +standard error = { +tests/bugs/import-with-error-extra.slang(10): error 30015: undefined identifier 'b'. +} +standard output = { +} diff --git a/tests/diagnostics/recursive-import-extra.slang b/tests/diagnostics/recursive-import-extra.slang new file mode 100644 index 000000000..4525346fa --- /dev/null +++ b/tests/diagnostics/recursive-import-extra.slang @@ -0,0 +1,6 @@ +//TEST_IGNORE_FILE: + +// This file creates an `import` loop with +// `recursive-import.slang` + +import recursive_import; diff --git a/tests/diagnostics/recursive-import.slang b/tests/diagnostics/recursive-import.slang new file mode 100644 index 000000000..ff74025f8 --- /dev/null +++ b/tests/diagnostics/recursive-import.slang @@ -0,0 +1,6 @@ +//TEST:SIMPLE: + +// A file that recursively imports itself +// (including transitive cases) should be diagnosed. + +import recursive_import_extra; diff --git a/tests/diagnostics/recursive-import.slang.expected b/tests/diagnostics/recursive-import.slang.expected new file mode 100644 index 000000000..ad5cf1975 --- /dev/null +++ b/tests/diagnostics/recursive-import.slang.expected @@ -0,0 +1,6 @@ +result code = -1 +standard error = { +tests/diagnostics/recursive-import.slang(6): error 38200: module `recursive_import_extra` recursively imports itself +} +standard output = { +} diff --git a/tests/preprocessor/import.hlsl b/tests/preprocessor/import.hlsl deleted file mode 100644 index f0618a667..000000000 --- a/tests/preprocessor/import.hlsl +++ /dev/null @@ -1,18 +0,0 @@ -//TEST:SIMPLE:-target none -profile vs_5_0 - -// Confirm that `#import` interacts with preprocessor as expected - -// Here is a macro that flows from parent to child file -#define FOO float - -// Here we import the child file -#import "import.slang.h" - -// Here we use a macro that flows the other way (child->parent) -BAR g( FOO x ) { return f(x); } - -// Here we confirm that importing the file again is a no-op -#import "import.slang.h" - -void main() -{} diff --git a/tests/preprocessor/import.slang.h b/tests/preprocessor/import.slang.h deleted file mode 100644 index a97a199f0..000000000 --- a/tests/preprocessor/import.slang.h +++ /dev/null @@ -1,12 +0,0 @@ -// Confirm that `#import` interacts with preprocessor as expected - -// We add a guard to ensure that this file isn't imported more than once -#ifdef BAR -#error File imported more than one! -#endif - -// Here we use a macro from the parent file -FOO f( FOO y ) { return y; } - -// Here is a macro that flows from child to parent -#define BAR float -- cgit v1.2.3