Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .release-notes/5147.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
## Fix spurious 'can't reuse name' errors when a multi-file package has a syntax error

When a package contained multiple `.pony` files and one of them had a syntax error, the compiler would produce spurious `can't reuse name` errors for unrelated valid code in the other files. This was most visible when a valid file reused a loop variable name across consecutive `for` loops — a perfectly legal pattern that the compiler would incorrectly flag.

The spurious errors no longer appear. You'll still see the real syntax error, but the compiler won't pile on with bogus errors from other files in the same package.

11 changes: 11 additions & 0 deletions src/libponyc/pkg/package.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use a brief comment here. Even just a pointer to the comment on the other branch below so someone reading this code path doesn't have to go hunting for why the flag is being set.

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)
Expand Down
3 changes: 3 additions & 0 deletions test/libponyc/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,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
Expand Down
27 changes: 27 additions & 0 deletions test/libponyc/badpony.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1735,3 +1735,30 @@ TEST_F(BadPonyTest, MatchTuplePatternFromAliasedTupleUnsound)
"this capture is unsound",
"this capture is unsound");
}

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");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im not a fan of adding fixtures to a testing style we are trying to move away from. @jemc what are your thoughts?

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'");
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// Intentional syntax error: = instead of =>
class Bad
new create() =
None
16 changes: 16 additions & 0 deletions test/libponyc/fixtures/spurious-error-message-generation/good.pony
Original file line number Diff line number Diff line change
@@ -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
6 changes: 6 additions & 0 deletions test/libponyc/util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 3 additions & 0 deletions test/libponyc/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down