Fix spurious 'can't reuse name' errors when a multi-file package has a syntax error#5147
Fix spurious 'can't reuse name' errors when a multi-file package has a syntax error#5147pmetras wants to merge 6 commits intoponylang:mainfrom
Conversation
|
Hi @pmetras, The changelog - fixed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do. Release notes are added by creating a uniquely named file in the The basic format of the release notes (using markdown) should be: Thanks. |
src/libponyc/pkg/package.c
Outdated
| // partially-initialised package. Without this, passes that run on the | ||
| // whole program tree (e.g. the scope pass) would visit modules that | ||
| // were successfully parsed in this package but whose for-loop sugar | ||
| // has not been applied, producing spurious errors in those files. |
There was a problem hiding this comment.
The comment here ties the explanation to for-loop sugar, but the underlying problem is more general. Any pass running on a partially-processed package could produce spurious errors. The for-loop case is just how this manifested in #4160. I'd drop the for-loop detail and keep it general — "producing spurious errors in those files" is enough. The for-loop specifics are better suited to the commit message where they're tied to the issue report.
| } else if(magic->mapped_path != NULL) { | ||
| if(!parse_files_in_dir(package, magic->mapped_path, opt)) | ||
| { | ||
| ast_setflag(package, AST_FLAG_PRESERVE); |
There was a problem hiding this comment.
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.
.release-notes/5147.md
Outdated
| 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. |
There was a problem hiding this comment.
paragraphs in release notes shouldn't have linebreaks. they should flow to fit the space they are in,
|
The release notes in this PR contain implementation details that aren't relevant to users. Here's a suggested replacement: ## 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. |
.release-notes/5147.md
Outdated
| 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. |
There was a problem hiding this comment.
please remove the hard wrapping in this file.
There was a problem hiding this comment.
we don't do any line breaks in release notes except for paragraphs so they can flow to the viewport in the various places they are displayed.
| // 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"); |
There was a problem hiding this comment.
im not a fan of adding fixtures to a testing style we are trying to move away from. @jemc what are your thoughts?
Fix the problem demonstrated into issue #4160: getting extra error messages in situations where one
.ponyfiles of an included (used) module has a syntax error and another of the module files that the main code is importing has 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 is to set
AST_FLAG_PRESERVEon the package whenparse_files_in_dirfails (same pattern already used whenast_passes_subtreefails). This stops future passes from visiting the broken package.The
magic->mapped_pathbranch also needed the same guard, becauseadd_package_pathuses it andparse_files_in_dirhas the same partial-parse behavior in both branches.Added
BadPonyTest.SpuriousErrorMessageGenerationtest with the newadd_package_pathutility method to be able to test on multi-files packages.