Add shadowed formal warning to dyno scope resolver #21614
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mppf
force-pushed
the
shadowed-formal-warning
branch
2 times, most recently
from
7441fae
to
adbcc34
Compare
DanilaFe
approved these changes
Feb 16, 2023
Focus on using idToTag instead of idToAst to check for the erroneous conditions. idToTag is much more likely than idToAst to remain the same between incremental recompiles. As a result, whatever query is running this code is more likely to be able to reuse the result. --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
Without this change, I was seeing the "In function ..." prefix, but no warning. --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
mppf
force-pushed
the
shadowed-formal-warning
branch
from
adbcc34
to
cc211af
Compare
2 tasks
bradcray
added a commit
to bradcray/chapel
that referenced
this pull request
Mar 3, 2023
(watch out for those #21x33 PRs, kids!) This fixes a few futures that I must've missed in my last paratest run (prior to paratest running futures by default!) as well as an obvious problem with the test of the bash autocompletion which I'm not sure how it slipped past me. It also fixes an issue from Ben McDonald's chapel-lang#21733 which I happened to notice since he's likely out for the evening. While here, I noticed that an erroneous condition that I'd noted earlier in chapel-lang#21652 has been resolved, probably by chapel-lang#21614 as Michael predicted, so I added a compopts line to lock that behavior in (and removed another that, in retrospect, felt overly paranoid). --- Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
bradcray
added a commit
that referenced
this pull request
Mar 3, 2023
[trivial, not reviewed] This fixes a few futures that I must've missed in my last paratest run (prior to paratest running futures by default!) as well as an obvious problem with the test of the bash autocompletion which I'm not sure how it slipped past me. It also fixes an issue from Ben McDonald's #21733 which I happened to notice since he's likely out for the evening. While here, I noticed that an erroneous condition that I'd noted earlier in #21652 has been resolved, probably by #21614 as Michael predicted, so I added a compopts line to lock that behavior in (and removed another that, in retrospect, felt overly paranoid).
mppf
added a commit
that referenced
this pull request
Mar 8, 2023
) Addresses #18177 and #21157 but only with `--dyno` Resolves Cray/chapel-private#4409 This PR improves the multiply-defined symbol error in the dyno scope resolver (currently available with `--dyno`). The improvements are: * do the multiply-defined symbol checking in a query to avoid redundant errors (e.g. for `proc f(x: int, x)`). * include symbols `public use` and `public` or `private` `import` in the checking * add checking for symbols of a different category in the same scope #### Details This PR: * adds `parsing::idIsFunction` * updates the Redefinition error to take in a trace indicating the source of the symbols (relevant for errors with `public use`) * adds `emitMultipleDefinedSymbolErrors` which invokes a query that emits the error if necessary and can be called in multiple places without worry of emitting redundant errors * adds to `IdAndVis::SymbolTypeFlags` `PARENFUL_FUNCTION` and `NOT_PARENFUL_FUNCTION` which is useful for the Redefinition and may be important in the future for other errors / behavior adjustments for parenless functions. Adjusts the code creating OwnedIdsWithName to pass this information & added `isParenfulFunction` in `scope-queries.cpp` to help compute it. * adds `BorrowedIdsWithNameIter::curIdAndFlags` so that it is possible to extract the `IdAndFlags::Flags` when iterating over the matches in a `BorrowedIdsWithName` * added a new `LookupConfig` setting: `LOOKUP_SKIP_SHADOW_SCOPES`. This is needed so that the redefinition error can focus on `public use` / `import` but ignore redefinitions from `private use`, per discussion in #21157. * adds `Scope::collectNames` and `Scope::names` to help with the redefinition error. Moved `lookupName` out of the header file since it has a loop so might not be inline-able. * adds calls to `emitMultipleDefinedSymbolErrors` in `Resolver::enter(const NamedDecl* decl)`, `Resolver::enter(const Use* node)`, and `Resolver::enter(const Import* node)` and also in `resolveModule` and `scopeResolveModule`. * Replaces `describeSymbolSource` with `describeSymbolTrace` and creates per-error functions for the remaining logic (e.g. `describeAmbiguousMatch`). This way, it is easier to use the trace reporting code while customizing the description of the found symbols. Updated `ErrorRedefinition::write` to use the new trace information and print out something reasonable for conflicts just from `import` or `public use`. * Added `collectAllNames` to gather all names defined in a scope (not counting shadow scopes). Used this to implement `emitMultipleDefinedSymbolErrors`. * Added tests to `test/errors` for a variety of redefinition error patterns; as well as for the shadowed formal warning (follow-up to PR #21614). * Renames `OwnedIdsWithName::appendIdAndVis` to `OwnedIdsWithName::appendIdAndFlags` (I missed this when doing the broader rename in PR #21779). * Fixes a GCC warning when compiling a test after PR #21764. #### Updated tests There were a number of test changes to keep everything working with the additional checking for conflicts with public use and import. Tests that have a helper file that `public use BlockDist, CyclicDist` run into a conflict because the `config param testFastFollowerOptimization` is defined in each of them. * test/distributions/robust/arithmetic/driver.chpl * test/optimizations/autoLocalAccess/common.chpl These tests were using a `public use` within `main`. I don't think that is relevant to what is being tested here & I think PR #14353 just migrated all of the `use` statements in these tests to `public use`. * test/modules/diten/test_use_chain_resolution.chpl * test/modules/diten/test_use_chain_resolution2.chpl The modules primer had an example describing a multiple definition error with `import` where the use of the variable was commented out (since it doesn't compile) but now that still results in an error. So my PR just comments out the conflicting declaration. * test/release/examples/primers/modules.chpl The AMR tests followed the Python pattern of having a little test function per module in `proc main`. These AMR tests also do a lot of `public use` (and I don't think there is a fundamental reason it has to work that way; it is just what these tests were doing when they were written). The result is that we get symbol conflicts for `proc main` being defined multiple times. The fix for these is just to use `private proc main` instead (and using `public use Bla except main` would also work, but involves changing more lines). * test/studies/amr/ (many files) #### Tests with different error messages Tests now needing .good updates for `--dyno` after this PR: ``` functions/bradc/queryClassArgGenerics-bad2.chpl functions/bradc/resolution/arrayVsFn modules/shadowing/issue-19167-1-f.chpl modules/shadowing/issue-19167-1-x.chpl modules/shadowing/issue-19167-2-f.chpl modules/shadowing/issue-19167-2-x.chpl modules/shadowing/issue-19167-3-f.chpl modules/shadowing/issue-19167-3-x.chpl modules/shadowing/issue-19167-4-f.chpl modules/shadowing/issue-19167-4-x.chpl modules/shadowing/issue-19167-5-f.chpl modules/shadowing/issue-19167-5-x.chpl modules/shadowing/var-shadows-private-import.chpl modules/shadowing/var-shadows-public-import.chpl modules/shadowing/var-shadows-public-use.chpl visibility/import/edgeCases/anotherMultipleDef2 visibility/import/edgeCases/multipleDef2.chpl visibility/import/edgeCases/multipleDef3.chpl visibility/import/rename/same_name_in_mod.chpl visibility/import/rename/same_name_in_mod2.chpl ``` #### Future Work * Adjust the error checking according to the resolution of #20509. #### Testing and Review Reviewed by @riftEmber - thanks! - [x] full local testing - [x] no unexpected failures in `--dyno` testing
mppf
added a commit
that referenced
this pull request
Oct 5, 2023
Resolves Cray/chapel-private#5220 Resolves #19780 Resolves #19782 This PR introduces a warning for cases where symbol shadowing might be surprising. In particular, the warning is focused on cases where a `use` statement brings in a symbol that conflicts with a more obviously available symbol in an outer scope. This warning is motivated by the example in #14014 and the the hijacking example in #14014 (comment). Additionally, in review of PR #19306, it was requested to make a warning about the case that a `public use` symbol shadows a `private use` one and this PR implements that request. The new warning only applies to scope lookups for the innermost match; that means that the new warning does not apply to functions and calls. What is the logic for the warning? * when a symbol is available through a bulk import from a `use` statement, warn if it's defined in another available scope (another shadow scope or outer scope) except: * do not warn for locally defined symbols that shadow a symbol from a private use (shadow scope 1 or 2) * do not warn for conflicts between a name brought in by a private use and the module name brought in by the same private use The warning is implemented to consider all parent scopes and the names of toplevel modules, if we are considering a `use`/`import`. Considering the parent scopes seems natural to me but the toplevel module part is necessary to give the warning in the case of #14014 (comment) . We considered avoiding warning in the case that the toplevel module in question is in a different file but opted not to do that. Since the warning is (for the most part) a superset of the warning for hidden formals added in PR #21614, this PR removes the hidden formal warning. * The case where a function is found through a `use` statement but has the same name as a formal is no longer warned about because the new warning does not apply to function lookups (test/functions/iterators/recursive/recursive-iter-findfilesfailure) * The case of a local variable that shadows something from a `use` statement that in turn shadows a formal is no longer warned about, by the design of the new warning (test/modules/diten/testIfModuleShadowsLocal) Future Work: * consider a similar error for function calls Reviewed by @DanilaFe and @lydia-duncan - thanks! - [x] full comm=none testing
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a warning when a use/import statement brings in a symbol that shadows a function formal. This helps to get the following test working with
--dyno:While there, I noticed that
--no-warningswas still causing an "In function" header to be printed, so I also fixed that.The implementation approach here is to add checking in a new query run at the end of
resolveVisibilityStmtsfor use/import clauses within functions.Current error messages:
Reviewed by @DanilaFe - thanks!
--dynotesting