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 it be possible to shadow a symbol brought in with import? #19160

Closed
mppf opened this issue Feb 2, 2022 · 9 comments
Closed

should it be possible to shadow a symbol brought in with import? #19160

mppf opened this issue Feb 2, 2022 · 9 comments

Comments

@mppf
Copy link
Member

mppf commented Feb 2, 2022

In https://chapel-lang.org/docs/language/spec/procedures.html#determining-more-specific-functions it says that X is more specific than Y if X shadows Y.

I can understand why "shadowing" should be allowed for a use statement but it is not so clear to me that we should allow it for the corresponding example with import.

Here is an example with a function:

module M {
  import N.f;

  proc f() {
    writeln("M.f");
  }

  proc main() {
    f();
  }
}

module N {
  proc f() {
    writeln("N.f");
  }
}

This compiles today and prints M.f. I think it should not compile, because I think of import as creating a local symbol just like the proc declaration does.

Here is a similar example for scope resolution:

module M {
  import N.x;

  var x = "M";

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

module N {
  var x = "N";
  proc f() {
    writeln("N.f");
  }
}

Which also compiles and prints M.

Both of the above examples currently behave the same with public import.

@mppf mppf changed the title should import be considered a different scope for detecting shadowing in disambiguation? should it be possible to shadow a symbol brought in with import? Feb 2, 2022
@mppf
Copy link
Member Author

mppf commented Feb 2, 2022

@lydia-duncan tells me that it is this way intentionally and that the rationale can be summarized as keeping import as consistent with use as possible (but with different default behavior).

@bradcray
Copy link
Member

bradcray commented Feb 2, 2022

I feel like I could live with either the current behavior or the proposed behavior here as long as we're only talking about imports and symbol definitions at the same scope and not cases where they're at staggered scopes like:

import M.x;
{
  var x = ...;
  ...x...  // local var x should win here
}

Even if we didn't go so far as making the cases in the OP an error, I think it could merit a warning since it seems that something is wrong in the code (either you didn't need to import it or you didn't mean to shadow it).

Your examples also have the property that the function signatures are identical, but for the paren-ful subroutine case presumably I could add local overloads of a function using a different signature and call either version as appropriate (i.e., it would add to the overload set rather than eclipsing the imported version(s) of the procedure).

@mppf
Copy link
Member Author

mppf commented Feb 2, 2022

I feel like I could live with either the current behavior or the proposed behavior here as long as we're only talking about imports and symbol definitions at the same scope and not cases where they're at staggered scopes like

Yes, and I agree. It seemed odd to me at first but I'm not convinced we need to change it, either.

Your examples also have the property that the function signatures are identical, but for the paren-ful subroutine case presumably I could add local overloads of a function using a different signature and call either version as appropriate (i.e., it would add to the overload set rather than eclipsing the imported version(s) of the procedure).

Yes

@skaller
Copy link

skaller commented Feb 4, 2022

See #19167

@mppf
Copy link
Member Author

mppf commented Feb 4, 2022

In particular the relevant part is

So now for the second issue. When you import modules into another exclusively for the implementation of that module and not for re-exportation, you need to be able to resolve conflicts, but only inside that module.

which is saying that the ability to shadow a symbol brought in with import can be useful when using shadowing to resolve conflicts.

I think the counter-argument to that is that you can just rename it when you import it e.g. import C as A;

@mppf
Copy link
Member Author

mppf commented Feb 8, 2022

It occurred to me to think about how public import would impact the situation.

From the last paragraph in https://chapel-lang.org/docs/language/spec/modules.html#conflicts :

Symbols brought in directly by a public import are treated as though defined at the scope with the public import for the purpose of determining conflicts (see Re-exporting). This means that if the public use in module B of the previous example was instead replaced with a public import A.x, A’s x would conflict with C.x when resolving the main function’s body.

But, both of the examples in the original post behave the same with public import (shadowing and ending up working with the symbol in M).

So for public import it is not behaving as specified, at least.

And then, supposing we fixed public import, then I think I would not want toggling public/private on the import to change the behavior of these programs, and so I would argue that it shouldn't be possible to shadow in this way, and the conflict should be reported immediately.

@mppf
Copy link
Member Author

mppf commented Feb 25, 2022

Correcting something from #19160 (comment)

The spec does indicate that public import introduces a shadow scope -- https://chapel-lang.org/docs/language/spec/modules.html#re-exporting

Making a use or import public causes the symbols brought in by that statement to be visible as though they were defined in the scope with the use or import, a strategy which will be referred to as re-exporting. However, symbols with the same name in the scope with the use or import will still take precedence.

The earlier paragraph gave the impression it did not have a shadow scope but linked here.

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

@mppf: After #19306 this seems to be working in a much more satisfying way. What more needs to be done to resolve this?

@mppf
Copy link
Member Author

mppf commented May 12, 2022

Yes, #19306 makes the import no longer introduce a shadow scope. To make sure we didn't lose the test case in the OP, I've created #19798 to add two variations it (and to note in a comment two other tests are related to this issue). Closing this issue.

@mppf mppf closed this as completed May 12, 2022
mppf added a commit that referenced this issue May 12, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants