Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should use statements have two shadow scopes? #19219

Closed
mppf opened this issue Feb 8, 2022 · 14 comments
Closed

Should use statements have two shadow scopes? #19219

mppf opened this issue Feb 8, 2022 · 14 comments

Comments

@mppf
Copy link
Member

mppf commented Feb 8, 2022

This is a follow-up to #11262 (and other related issues are #14013 #14014 and #19167).

I am writing this issue because in #19167 I am proposing simplifying the visibility rules and this issue discusses a related simplification.

Consider this program which has a use M where the module M also defines a symbol named M. After the use M, what should M refer to?

module M {
  var M: int;
}

module N {
  use M;

  proc main() {
    writeln(M);
  }
}

Currently, this program compiles and runs, printing 0, which shows that after the use M, M refers to M.M. It has done so since at least 1.19 (although PR #13930 changed how it was implemented). The explanation for the current behavior is in #14014 (comment) where it describes the use statement adding two hidden/implicit/shadow scopes. However, the language specification only says this on the matter ( https://chapel-lang.org/docs/language/spec/modules.html#using-and-importing ):

The names that are made visible by a use or import statement are inserted in to a new scope that immediately encloses the scope within which the statement appears.

What should we do here?

  1. Keep the current behavior and document in the language specification that use statements work with two shadow scopes. The program discussed above would continue to compile.
  2. Simplify the rules for use statements to only work with one shadow scope. The program discussed above would result in an ambiguity error because module M and var M would be considered to be in the same scope (namely, the new/hidden/shadow scope of symbols brought in by the use statement). This would mean that it's not possible to use a module defining a symbol with the same name as the module name (see also issue Should modules be able to define symbols that share their name? #13925). It would be possible to import such a module, though.
  3. Change the rules for use / import to not use a shadow scope at all. This would mean one could no longer shadow something brought in with the use with a function/variable declaration. This is the same as Option C. in definition of shadowing is inconsistent between variables and functions #19167 and:
  4. Change the rules for public use / public import to not use a shadow scope at all. This is the same as Option C'. in definition of shadowing is inconsistent between variables and functions #19167. We would still have to choose one of the other options for the behavior of private use/import.
@mppf mppf changed the title What should happen when use-ing something with a symbol sharing the module name? Should use statements have two shadow scopes? Feb 8, 2022
@lydia-duncan
Copy link
Member

I'm divided. On the one hand, option 1 means less breaking changes and it feels like we've done a lot of reshuffling of the rules in scope resolution, so I have some change fatigue there. I also suspect some users will dislike having the symbols conflict in that case.

On the other hand, 2 does sound like an intriguing simplification and would be both easier to explain and implement/maintain.

@mppf
Copy link
Member Author

mppf commented Feb 8, 2022

@lydia-duncan - I also added a more aggressive Option 3 - what do you think about that?

@lydia-duncan
Copy link
Member

I think use and import statements should continue to add at least one shadow scope. I don't think implicitly included symbols (which could be at an arbitrary distance, depending on if the module author has been too incautious with public uses or imports) should conflict with explicitly defined symbols.

@mppf
Copy link
Member Author

mppf commented Feb 10, 2022

Say that we did Option 3. I think that might present a problem when we want to add functionality to the set of included-by-default operations.

Here is a scenario.

Start with this Program:

// Program

// implicitly has a 'use ChapelStandardLibrary' (whatever that is)
proc foo() { }

foo();

Now imagine that in a new version, the standard library adds a foo function with the same signature:

// ChapelStandardLibrary

proc foo() { }

Now when Program is compiled, with Option 3, it would fail with an ambiguity error. That is not desirable because we would like to be able to add functions to the standard library without considering that a backwards-breaking change.

(Interestingly, there are reasons why even with the other options, it would still be a problem. In particular, if the program wanted to pass c_ptrTo(foo) somewhere; or if the two functions in question here had different signatures but the call site can implicitly convert to both - in that case we should get an overloads set error -- see https://chapel-lang.org/docs/technotes/overloadSets.html -- if the chosen one would change to the new library function).

@mppf
Copy link
Member Author

mppf commented Feb 23, 2022

I have been exploring a combination of Option 2 and Option 4. However I am running into a pattern that we have discussed before in #13925 :

module MyDist {
  class MyDist {
  }
}
module Program {
  use MyDist;
  ...MyDist... // does this refer to the module or the class?
}

Some cases in our standard library where I am seeing this are:

  • module LocaleModel that contains a class LocaleModel
  • module DimensionalDist2D containing class DimensionalDist2D
  • similarly for ReplicatedDim etc

It seems not ideal to use this pattern because if there are symbol conflicts that come up, it won't be possible to resolve them with qualified naming. However, on #13925 there was some rationale for continuing with the current behavior, which will require two shadow scopes (effectively) for use statements.

@mppf
Copy link
Member Author

mppf commented Feb 24, 2022

Even if we continue with the two shadow scopes though, a public use of a module defining a class with the same name as the module will run in to the problem, at least with what I view as the current direction in #19167, which is that public use does not introduce any shadow scopes.

@bradcray
Copy link
Member

bradcray commented Mar 1, 2022

@mppf: A couple of quick thoughts on your latest comments:

  • It seems to me that users frequently want the ability to have a module and a type within that module to share a name; i.e., if we took away the ability to do this, some would be unhappy (I think you're saying you're aware of this too, but wanted to re-assert it). That said, we could poll users to see whether I'm blowing things out of proportion (I feel like some user just brought it up really recently in a "can I do this?" sense, which is partially what makes me worry).

  • I find myself wondering whether saying "use just doesn't work well for such cases; use import in such cases instead" would be sufficient here. Or does it have a symmetrical problem? (seems like it wouldn't, but I'm not confident). Or maybe it just feels too icky to require...?

  • I think this only helps a little, if at all, but: What if, in the public use M; case, only the contents of M are made available outside of the current scope, not module M itself? I've been arguing that one should be able to refactor code into a publicly used module without anyone being the wiser, in which case, that would seem to make sense. However, it wouldn't help for code within the module containing public use M; that wanted to distinguish between M and M.M I guess?

@mppf
Copy link
Member Author

mppf commented Mar 1, 2022

@bradcray - thanks for your thoughts.

  • It seems to me that users frequently want the ability to have a module and a type within that module to share a name; i.e., if we took away the ability to do this, some would be unhappy (I think you're saying you're aware of this too, but wanted to re-assert it). That said, we could poll users to see whether I'm blowing things out of proportion (I feel like some user just brought it up really recently in a "can I do this?" sense, which is partially what makes me worry).

Yes I think it is nice if we can avoid banning it outright. IIRC it is a common pattern in Python.

  • I find myself wondering whether saying "use just doesn't work well for such cases; use import in such cases instead" would be sufficient here. Or does it have a symmetrical problem? (seems like it wouldn't, but I'm not confident). Or maybe it just feels too icky to require...?

Yes, it is my understanding that working with import only completely makes these problems go away.

  • I think this only helps a little, if at all, but: What if, in the public use M; case, only the contents of M are made available outside of the current scope, not module M itself? I've been arguing that one should be able to refactor code into a publicly used module without anyone being the wiser, in which case, that would seem to make sense. However, it wouldn't help for code within the module containing public use M; that wanted to distinguish between M and M.M I guess?

This is an interesting proposal and I think of it as bringing up #13978 again. In the past I was in favor of a design that all use statements wouldn't bring in the name of the module, but I lost that argument. My understanding was that somebody wanting to re-export while hiding implementation details would use public use M as _.

Anyway, not bringing in the symbol M itself for public use M would be nice in that it solves this problem but it would make public use even more different from private use (and the potentially surprising differences between these two is probably the main drawback of the design in #19306). But, maybe this is a case where adding this difference between public use and private use makes the situation no worse, if they are already different in how shadowing works with them.

So, I would lean towards doing that, because I think it makes public use work in a more obvious way for re-exporting, which I view as its purpose.

@mppf
Copy link
Member Author

mppf commented Mar 1, 2022

Oh, and another possible direction here is this from #13978 (comment)

Over on #13979 (and specifically #13979 (comment) ) I think you suggested allowing use statements to bring in the module name, but treat that module name as private (even for a public use). That proposal would be fine with me.

It seems weird to me that the public use would behave differently within and outside of the module though, so I would prefer we make public use M hide the module name M (and you can add import M if you want to expose that).

@mppf
Copy link
Member Author

mppf commented Mar 8, 2022

In testing with the "public use does not bring in the module name" change, one interesting case that failed is this test -- https://github.com/chapel-lang/chapel/blob/main/test/visibility/rename/renameUsedMod/renamePublicUse2.chpl . It is interesting because it raises the question of what public use Foo as F should do. Here I can imagine two possibilities:

  1. If you wrote an as renaming the module, it should be respected. That means that public use Foo as Foo would do something different from public use Foo (the 1st would allow Foo.x but the second would not).
  2. We make this pattern with public use and as renaming the module into a compilation error.

Any thoughts on which way to go here? I am slightly inclined to do 1.

@bradcray
Copy link
Member

bradcray commented Mar 8, 2022

At this moment, I'd be fine with either approach. One minor argument in favor of approach 2 is that it can be changed later without breaking existing code (and if users found they wanted it, they could request it).

@lydia-duncan
Copy link
Member

I'm in favor of continuing to support using as to rename the module.

mppf added a commit that referenced this issue May 10, 2022
simplify use/import shadowing

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 #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:
 * #19167
 * #19160
 * #19219 and #13925
 * #19312
 * #19352
 
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.

- [x] full local futures testing
- [x] gasnet testing

### Future Work

There are two areas of future work that we would like to address soon:
 * #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).
 * #19780 to add warnings for cases where the difference between `public
   use M` and `private use M` might be surprising
@bradcray
Copy link
Member

I feel as though through the discussion around #19306, we got pretty comfortable with the notion of private use continuing to have two shadow scopes, one for the symbols, and a slightly outer one for the module name itself. That makes me think we could close this issue unless I'm missing some thread that's unresolved.

@mppf
Copy link
Member Author

mppf commented May 12, 2022

Yes I consider this issue resolved after PR #19306. Closing.

@mppf mppf closed this as completed May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants