simplify use/import shadowing #19306

mppf · 2022-02-24T22:27:26Z

This PR describes and implements some candidate language adjustments to shadowing behavior for use and import statements. We need to do something in this area because the definition of shadowing is currently inconsistent between variables and functions (#19167). This PR attempts to simplify the language design in this area.

The adjustments to the language design in this PR are as follows:

isMoreVisible in function disambiguation as well as scope resolution use the same rules for determining shadowing with use/import statements
isMoreVisible starts it search from the POI where the candidates were discovered (see issue problem with isMoreVisible for functions and point of instantiation #19198 -- not discussed further here)
private use statements still have two shadow scopes
public and private import statements now do not introduce a shadow scope
public use statements now do not introduce a shadow scope
public use M does not bring in M (but public use M as M does)
methods are no longer subject to shadowing

Note that the above design leads to less shadowing of symbols from the automatic library (see the section "Less Shadowing of Symbols from the Automatic Standard Library" in #19306 (comment))

Discussion

Elements of the language design direction are discussed in these issues:

Please see also #19306 (comment) which discusses pros and cons of these language design choices.

Testing and Review

Reviewed by @lydia-duncan - thanks!

This PR passes full local futures testing.

Resolves the future test/modules/vass/use-depth.chpl.
Also fixes #19198 and adds a test based on the case in that issue.

full local futures testing
gasnet testing

Future Work

There are two areas of future work that we would like to address soon:

should it be possible to explicitly use/import the automatic modules? #19313 which relates to changes in this PR to the test modules/bradc/userInsteadOfStandard/foo2. The behavior for trying to replace a standard module has changed (presumably due to minor changes in how the usual standard modules are now available). I think that changing the compiler to separately list each standard module would bring the behavior back the way it was. (Or we could look for other implementation changes to support it).
need warnings for differences between public and private use #19780 to add warnings for cases where the difference between public use M and private use M might be surprising

test/classes/diten/breakList.good

test/constrained-generics/random/MyRandom.chpl

test/performance/thomasvandoren/TestHelpers.chpl

test/reductions/thomasvandoren/scanCountsInherit.chpl

...dies/cholesky/jglewis/version2/elemental/fully_blocked/elemental_cholesky_fully_blocked.chpl

test/types/enum/lydia/use/useMultiple.chpl

test/types/string/sungeun/c_string/decls.chpl

lydia-duncan · 2022-02-25T19:18:21Z

No Shadow Scopes for Public Use

Another con of this approach is that changing the privacy of a use will impact code inside the module with the use statement, instead of just impacting code outside the module with the use statement. This means that public use M and private use M are effectively different statements to the module containing them, which might be confusing.

mppf · 2022-02-25T19:20:03Z

Thanks, I will include it in my list of Cons.

lydia-duncan

I have some comments on the implementation (I think it makes sense for me to be the reviewer as well) and some comments on changes made to tests for the purpose of the PR and for the purpose of the language proposal change.

My main concern is the change to the last test - it seems like the user has no recourse that allows them to call D's explicitly defined checkFoo, where before they could use qualified naming. That seems like a problem to me.

compiler/passes/scopeResolve.cpp

doc/rst/language/spec/modules.rst

compiler/passes/scopeResolve.cpp

compiler/resolution/functionResolution.cpp

test/modules/diten/testIfModuleShadowsLocal.good

test/release/examples/primers/modules.chpl

test/visibility/except/deepUseWouldConflict.chpl

test/visibility/private/uses/definesMultiUse.chpl

test/visibility/private/uses/weirdEnumHiding.chpl

modules/standard/ChapelEnv.chpl

test/functions/generic/poi/issue-19198.good

test/modules/diten/test_use_chain_resolution.good

test/modules/shadowing/issue-19167-7-x.chpl

test/modules/shadowing/var-shadows-private-use.chpl

test/modules/vass/use-depth.good

test/reductions/thomasvandoren/sorted.chpl

test/release/examples/primers/modules.chpl

test/types/string/sungeun/c_string/casts.chpl

test/visibility/import/edgeCases/anotherMultipleDef.good

test/visibility/import/rename/same_name_in_mod.good

test/visibility/private/uses/weirdEnumHiding.chpl

PR chapel-lang#19306 addressed this issue but I didn't see tests for two cases mentioned in the issue. Also added a comment to two other tests related to the issue with the issue number. --- Signed-off-by: Michael Ferguson <[email protected]>

Add tests for issue 19160 PR #19306 addressed issue #19160 but I didn't see tests for two cases mentioned in the issue. Also added a comment to two other tests related to the issue with the issue number. Test change only - not reviewed.

@lydia-duncan

Cache module-level scope resolution PR #19306 made scope resolution significantly slower. I think part of the reason for that is it removed some caching. This PR adds some caching for module-level name lookups to make the process faster in the face of uses and imports. That allows scope resolution to run slightly faster (in my experiments) than before PR #19306. The caching needed some adjustment in order to work with extern block support. See also PR #12235 for history in this area. See also * Cray/chapel-private#3375 Reviewed by @lydia-duncan - thanks! - [x] full local testing

--- Signed-off-by: Michael Ferguson <[email protected]>

@lydia-duncan

Add details to spec about public use and method shadowing Follow-up to PR #19306. Reviewed by @lydia-duncan - thanks!

@lydia-duncan

Add workarounds to get userInsteadOfStandard working for now We started seeing failures in userInsteadOfStandard in some configurations after #19884 due to the additional checking it enabled. At the same time, PR #19306 temporarily accepted this test no longer behaving as desired. This PR applies workarounds to both the compiler and the test to get it working again. However, it's my opinion that this pattern is not worth supporting, as I describe in this comment added to the top of `userInsteadOfStandard/foo2.chpl` by this PR: ``` // This test replaces an automatically-included standard library file with // a user one because AutoMath.chpl exists as a sibling to this file. // While this test uses AutoMath, as of this writing, there are 4 automatic // standard libraries: AutoMath, Errors, ChapelIO, and Types. // A workaround in parser.cpp allows this replacing process for these // except for Errors (because it runs into another workaround in the parser). // See issue #19313 for a non-workaround proposal to address this. // One challenge here is that there can only be one copy of a top-level // module in a given compilation. That means that if this test uses // ./AutoMath.chpl as the AutoMath module, the internal library will // refer to it for things like min/max instead of the real one. // A related challenge to that, and part of the issue with Errors, is that in // order for this to work, the first use of AutoMath that the compiler seeks // to resolve has to be this one (vs one coming from somewhere else, e.g. an // already-parsed included module). That presents an ordering constraint that // causes problems when doing incremental or separate compilation. This issue, // combined with the limited number of modules to which this can apply, make // it potentially not worthwhile to support this pattern. ``` Reviewed by @lydia-duncan - thanks! - [x] full local testing

@DanilaFe

Fix scope resolution of except lists in dyno Fixes bugs with scope resolution of use/imports with `except` lists in Dyno: - Search for symbols *not* named in an `except` list - Don't bring in the module name itself from a `public use` (see #19306). - In production scope-resolver, fix gathering types with method calls defined on them in the searched scope. Resolves Cray/chapel-private#3814. [reviewed by @DanilaFe] Testing: - [x] checking against tests listed in Cray/chapel-private#3814 (see individual check-offs on that list) - [x] primers with --dyno - [x] paratest

@DanilaFe

This PR updates the dyno scope resolver to follow the current language design rules about shadow scopes. These evolved a bit after the dyno scope resolver was created (see PR #19306). See also [this spec section](https://chapel-lang.org/docs/language/spec/modules.html#using-and-importing). To do so: * added a field to `VisibilitySymbols` to indicate which shadow scope (if any) the use/imported symbols go into * renamed `doLookupInImports` to `doLookupInImportsAndUses` and have it only consider symbols at the current shadow scope level * created `doLookupInAutoModules` (pulled out of `doLookupInImports`) * adjusted `doLookupInScope` to check the non-shadow scope and then the two shadow scopes * adjusted `doResolveUseStmt` and `doResolveImportStmt` to provide a shadow scope value when constructing a `VisibilitySymbols` * improved stringify/dump for `VisibilitySymbols` and `ResolvedVisibilityScope` * added testing for some of these cases to `testScopeResolve.cpp` This fixes a number of tests that fail with `--dyno` (which activates the dyno scope resolver): ``` modules/shadowing/issue-19167-3-x modules/shadowing/issue-19167-5-x modules/shadowing/var-shadows-private-import modules/shadowing/var-shadows-public-import modules/shadowing/var-shadows-public-use visibility/import/rename/same_name_in_mod visibility/import/rename/same_name_in_mod2 ``` Reviewed by @DanilaFe - thanks! - [x] full local testing - [x] full local `--dyno` testing

This is a belated follow-up to PR chapel-lang#19306. It is a bug that the production compiler allows this pattern today. --- Signed-off-by: Michael Ferguson <[email protected]>

@DanilaFe

This PR is intended to fix a failure with `--dyno` for the test test/visibility/rename/renameUsedMod/renameModRenameSymBadAccess2.chpl Previously, the dyno resolver was not causing an error, and then the production compiler's error handling did not print out the hint is used to, because the identifier `Bar` had been resolved to `Foo` by the dyno scope resolver. This PR takes the following steps: * adds `IllegalUseImport` error and emit it in post-parse checks. The wording of this error message is migrated completely from the production compiler where it was added in PR #18731. * Renames the `AmbiguousIdentifier` error to `AmbiguousVisibilityIdentifier` and adds new errors `UnknownIdentifier` and `AmbiguousIdentifier`. Note that these two new errors are currently disabled for `--dyno` (but they are emitted when using the full dyno type and call resolver, e.g. in the dyno tests). * Adds a new `NotInModule` error for the case such as `renameModRenameSymBadAccess2.chpl` described above. * Adds `lookupNameInScopeTracing` which can record where matches came from for use in emitting errors * Adjusts `class Scope` to track if it contained an extern block (`extern { }`) so that further steps can be taken when looking for symbols in such a block. * Added `LOOKUP_EXTERN_BLOCKS` to enable search in extern blocks Tests that now have different error message output (and will need .good file updates for `--dyno`): * Added `CountIdentifiersWithName` to support errors with parts like "first use this function" (the idea is that it's misleading to print that if the name is only used once in the function) * Added workaround code in Resolver.cpp to ignore results that match an extern block (since we have not yet implemented extern block support in the compiler library). * Adjusted name lookup functions to check extern blocks for a match after other things. This is draft code today & it doesn't yet have the ability to establish that a name is or is not provided by an extern block. But, symbols from an extern block should only be used if no other symbol with that name is present. So, for now it assumes that any name not found otherwise will resolve to an extern block (if present). This needs to be fixed in the future but it's OK for now because the Resolver has workaround code to ignore extern block matches. * Updates a few tests (including tests in the spec) that rely `M.N.symbol` where `M` contains a `public use N`. After PR #19306, `public use` should not bring in the module name. I've created issue #21640 about how these cases are compiling in production when they should not. #### NotInModule Example ``` chapel module A { var x: int; } module Main { use A; proc main() { A.y; } } ``` ``` aa.chpl:7: In function 'main': aa.chpl:8: error: cannot find 'y' in module 'A' ``` ``` ─── error in aa.chpl:8 [NotInModule] ─── Cannot find 'y' in module 'A' | 8 | A.y; | ⎺⎺⎺ | ``` #### IllegalUseImport Example (The wording of this error message is migrated completely from the production compiler where it was added in PR #18731.) ``` chapel module Main { public import (if p then super.a else super.b) as A; } ``` ``` aa.chpl:1: In module 'Main': aa.chpl:2: error: Illegal expression in 'import' statement aa.chpl:2: note: only identifiers and 'dot' expressions are supported ``` ``` ─── error in aa.chpl:2 [IllegalUseImport] ─── Illegal expression in 'import' statement | 2 | public import (if p then super.a else super.b) as A; | Only identifiers and 'dot' expressions are supported ``` #### UnknownIdentifier Example (note, this error is currently disabled for `--dyno` as a workaround to problems unrelated to this PR) ``` chapel module M { x; } ``` ``` aa.chpl:1: In module 'M': aa.chpl:2: error: 'x' cannot be found ``` ``` ─── error in aa.chpl:2 [UnknownIdentifier] ─── 'x' cannot be found | 2 | x; | ⎺ | ``` #### AmbiguousIdentifier Example (note, this error is currently disabled for `--dyno` as a workaround to problems unrelated to this PR) ``` chapel module A { var x: int; } module B { var y: real; } module C { public import B.{y as x}; } module M { use A, C; x; } ``` ``` aa.chpl:14: In module 'M': aa.chpl:16: error: 'x' is ambiguous aa.chpl:15: note: first, through the 'use' statement here aa.chpl:2: note: found 'x' defined here aa.chpl:15: note: additionally, through the 'use' statement here aa.chpl:11: note: and then through the 'import' statement here aa.chpl:8: note: and then through the 'import' statement providing 'y' here aa.chpl:5: note: found 'z' defined here ``` ``` ─── error in aa.chpl:16 [AmbiguousIdentifier] ─── 'x' is ambiguous | 16 | x; | ⎺ | First, through the 'use' statement here: | 15 | use A, D; | ⎺ | Found 'x' defined here: | 2 | var x: int; | ⎺⎺⎺⎺⎺⎺⎺ | Additionally, through the 'use' statement here: | 15 | use A, D; | ⎺ | And then through the 'import' statement here: | 11 | public import C.{y as x}; | ⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺ | And then through the 'import' statement providing 'y' here: | 8 | public import B.{z as y}; | ⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺ | Found 'z' defined here: | 5 | var z: real; | ⎺⎺⎺⎺⎺⎺⎺⎺ | ``` ``` chapel module A { var x: int; var x: real; } module M { use A; proc main() { x; } } ``` ``` bb.chpl:9: error: 'x' is ambiguous bb.chpl:7: note: through the 'use' statement here bb.chpl:1: In module 'A': bb.chpl:2: note: found 'x' defined here bb.chpl:3: note: and found 'x' defined here ``` ``` ─── error in bb.chpl:9 [AmbiguousIdentifier] ─── 'x' is ambiguous | 9 | x; | ⎺ | Through the 'use' statement here: | 7 | use A; | ⎺ | Found 'x' defined here: | 2 | var x: int; | ⎺⎺⎺⎺⎺⎺⎺ | And found 'x' defined here: | 3 | var x: real; | ⎺⎺⎺⎺⎺⎺⎺⎺ | ``` #### Tests Needing .good Update for `--dyno` After this PR, these tests will need .good file updates for `--dyno`: ``` modules/deitz/test_module_access1 modules/diten/modulescopes visibility/import/edgeCases/dontLeakSymbols1 visibility/rename/badAccessWithPrefix ``` Future Work: * handle extern blocks in the dyno scope resolver / resolver (Cray/chapel-private#3414) * get `domain` to scope resolve in dyno and enable the dyno identifier-not-found error for `--dyno` * change `doLookupIn...` functions to work with a struct instead of too many arguments. Reviewed by @DanilaFe - thanks! - [x] full local testing - [x] no unexpected failures in `--dyno` testing

@DanilaFe

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

@DanilaFe

This PR improves the performance of the new scope resolver. We have observed performance problems with `--dyno-scope-bundled`. These are more pronounced with developer builds, but are also present with release builds. The magnitude of the performance problems presents a challenge to turning on `--dyno-scope-bundled` by default. This table summarizes the score, with times in seconds: | | main | this PR | |------------------------------------------------------------------------|------|---------| |`chpl --dyno-scope-bundled hello.chpl` (quickstart with CHPL_DEVELOPER) | 16.1 | 12.8 | |`chpl --dyno-scope-bundled hello.chpl` (default, no CHPL_DEVELOPER) | 3.0 | 2.6 | |`chpl hello.chpl` (quickstart with CHPL_DEVELOPER) | 9.7 | 9.5 | |`chpl hello.chpl` (default, no CHPL_DEVELOPER) | 2.5 | 2.4 | This PR takes the following steps in order to achieve this: * Changes BorrowedIdsWithName to copy IDs rather than referring to OwnedIdsWithName. The idea of referring to a list of overloads with the same name was always an optimization. However, it did not seem to help performance and it added some complexity. Along with this change, renamed BorrowedIdsWithName to MatchingIdsWithName. There is no longer a need for `std::vector<BorrowedIdsWithName>` -- `MatchingIdsWithName` covers that. * Adds a query to compute the public contents of a module. After the changes in PR #19306, the public symbols available in a module are always fixed and at a single level (for shadowing purposes). This is responsible for most of the speed improvement here for `--dyno-scope-bundled`. * Removes certain `public use`s in the standard library. The motivation here is to remove the possibility for modules that `public use` each other (potentially through an intermediate module). This case is difficult to support efficiently in the new query to compute the public contents of a module. Removing these `public use` statements can be viewed as a continuation of PR #16119. * Adds a Flags to IdAndFlags to track Module-ness and Type-ness. These are useful to optimize some of the code using scope lookup. In particular, such code can use these flags instead of using `parsing::idToTag`. * Adjusts the strategy for handling conditionals with variables (see PR #21140) and do-while statements (see PR #22564). Both of these have scoping behavior where the parent scope is not the scope for the parent ID. Before this PR, that was handled with `nextHigherScope` which used queries to check for these patterns. This PR changes the process of constructing a `Scope` to take these patterns into account, so that `scope->parentScope()` can be used even in these cases. Future Work: * It might be possible to further improve the new scope resolver by caching the scopes that need to be traversed when looking for a symbol. I prototyped something that saves such a linearized view of scopes in https://github.com/mppf/chapel/tree/save-scopes-visited . However, it will take more work to get this functional, and it's currently unclear if this offers enough performance improvement to be worthwhile. - [x] full comm=none testing - [x] spot-checked that a program can compile and run with `CHPL_LOCALE_MODEL=gpu` Reviewed by @DanilaFe - thanks!

mppf marked this pull request as draft February 24, 2022 22:27