Fix spurious 'can't reuse name' errors when a multi-file package has a syntax error

.release-notes/5147.md

@@ -0,0 +1,22 @@
+## Fix spurious 'can't reuse name' errors when a multi-file package has a syntax error
+
+Fix [issue 4160](https://github.com/ponylang/ponyc/issues/4160) that contains
+and example reproducing the problem.
+When a package `use`d another package containing multiple `.pony` files, and
+one of these files had a syntax error, the compiler output wrong and extra
+error messages, particularly when the first code had multiple `for` loops
+using the same loop variable.
+
+The issue was caused by the compiler's package loading logic. When a package
+containing multiple files was loaded, if one file had a syntax error, the
+package was left in a partially-processed state. Subsequent attempts to use
+or "catch up" that package to the current compiler pass would re-run passes
+(like the scope pass) on the existing AST. Since the scope pass had already
+partially populated the symbol tables, re-running it caused it to encounter
+its own previously defined symbols and report them as illegal name reuses.
+
+The fix sets `AST_FLAG_PRESERVE` on the package when `parse_files_in_dir` fails.
+This stops future passes from visiting the broken package.
+
+The compiler tests were adapted to support testing with a package containing
+multiple source files.

src/libponyc/pkg/package.c

@@ -1052,15 +1052,26 @@ ast_t* package_load(ast_t* from, const char* path, pass_opt_t* opt)
         return NULL;
     } else if(magic->mapped_path != NULL) {
       if(!parse_files_in_dir(package, magic->mapped_path, opt))
+      {
+        // If source file parsing failed, don't run future passes on the
+        // partially-initialised package.
+        ast_setflag(package, AST_FLAG_PRESERVE);
         return NULL;
+      }
     } else {
       return NULL;
     }
   }
   else
   {
     if(!parse_files_in_dir(package, full_path, opt))
+    {
+      // If source file parsing failed, don't run future passes on the
+      // partially-initialised package, producing spurious errors in
+      // those files.
+      ast_setflag(package, AST_FLAG_PRESERVE);
       return NULL;
+    }
   }
 
   if(ast_child(package) == NULL)

test/libponyc/CMakeLists.txt

@@ -67,13 +67,16 @@ target_include_directories(libponyc.tests
 )
 
 set(PONY_PACKAGES_DIR "${CMAKE_SOURCE_DIR}/packages")
+set(PONY_TEST_LIBPONYC_DIR "${CMAKE_SOURCE_DIR}/test/libponyc")
 
 if (MSVC)
     string(REPLACE "/" "\\\\" PONY_PACKAGES_DIR ${PONY_PACKAGES_DIR})
+    string(REPLACE "/" "\\\\" PONY_TEST_LIBPONYC_DIR ${PONY_TEST_LIBPONYC_DIR})
 endif (MSVC)
 
 target_compile_definitions(libponyc.tests
     PRIVATE PONY_PACKAGES_DIR="${PONY_PACKAGES_DIR}"
+    PRIVATE PONY_TEST_LIBPONYC_DIR="${PONY_TEST_LIBPONYC_DIR}"
 )
 
 target_link_directories(libponyc.tests

test/libponyc/badpony.cc

@@ -1446,3 +1446,30 @@ TEST_F(BadPonyTest, MatchArrayPatternWithBareIntegerLiterals)
     "couldn't find 'eq' in 'Array'",
     "this pattern element doesn't support structural equality");
 }
+
+TEST_F(BadPonyTest, SpuriousErrorMessageGeneration)
+{
+  // From issue #4160
+  // A parse error in one file of a package must not produce spurious
+  // "can't reuse name" errors in other files of the same package that
+  // reuse a loop variable name across consecutive for loops.
+  // The fixture package has two files: bad.pony (parse error) comes before
+  // good.pony (valid, with consecutive "for i" loops) alphabetically.
+  add_package_path("pkg", PONY_TEST_LIBPONYC_DIR "/fixtures/spurious-error-message-generation");
+  const char* src =
+    "use \"pkg\"\n"
+    "actor Main\n"
+    "  new create(env: Env) =>\n"
+    "    for i in Range(0, 3) do\n"
+    "      None\n"
+    "    end\n"
+    "    for i in Range(0, 3) do\n"
+    "      None\n"
+    "    end";
+
+  TEST_ERRORS_2(src,
+    "syntax error: unexpected token = after type, interface, trait, primitive,"
+    " class or actor definition",
+    "can't load package 'pkg'");
+}
+

test/libponyc/fixtures/spurious-error-message-generation/bad.pony

@@ -0,0 +1,4 @@
+// Intentional syntax error: = instead of =>
+class Bad
+  new create() =
+    None

test/libponyc/fixtures/spurious-error-message-generation/good.pony

@@ -0,0 +1,16 @@
+// Syntactically valid file in the same package as bad.pony.
+// The loop variable 'i' is reused across consecutive for loops; before the
+// fix for issue #4160, a parse error in bad.pony caused spurious
+// "can't reuse name 'i'" errors here.
+class Good
+  new create() =>
+    let a: Array[U8] = Array[U8]
+    for i in a.values() do
+      None
+    end
+    for i in a.values() do
+      None
+    end
+    for i in a.values() do
+      None
+    end

test/libponyc/util.cc

@@ -310,6 +310,12 @@ void PassTest::add_package(const char* path, const char* src)
 }
 
 
+void PassTest::add_package_path(const char* path, const char* real_dir)
+{
+  package_add_magic_path(path, real_dir, &opt);
+}
+
+
 void PassTest::default_package_name(const char* path)
 {
   _first_pkg_path = path;

test/libponyc/util.h

@@ -49,6 +49,9 @@ class PassTest : public testing::Test
   // Add an additional package source
   void add_package(const char* path, const char* src);
 
+  // Add an additional package backed by a real filesystem directory
+  void add_package_path(const char* path, const char* real_dir);
+
   // Override the default path of the main package (useful for package loops)
   void default_package_name(const char* path);