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

problem with isMoreVisible for functions and point of instantiation #19198

Closed
mppf opened this issue Feb 7, 2022 · 1 comment · Fixed by #19306
Closed

problem with isMoreVisible for functions and point of instantiation #19198

mppf opened this issue Feb 7, 2022 · 1 comment · Fixed by #19306

Comments

@mppf
Copy link
Member

mppf commented Feb 7, 2022

Related to #19167.

Here is another program showing incorrect behavior of isMoreVisible for functions. This one shows that isMoreVisible is leading to incorrect behavior with the point of instantiation. In particular, this should fail to compile should fail due to ambiguity between the two N.helper calls but it does not. I think this is because isMoreVisible cannot find any of the helper1 candidates since they all come from POI and so returns true. We stopped using POI in isMoreVisible in PR #8079 but I think that as of PR #16158, we should already be preferring a candidate in a closer POI, so isMoreVisible now just needs to handle differentiating between visibility levels at a given point of instantiation.

module G {
  proc genericfunc(param p) {
    helper1();
  }
}

module M {
  use G;
  proc mCallFunc() {
    writeln("mCallFunc");
    genericfunc(1);
  }
  proc helper1() { writeln("M.helper1"); }
}

module N {
  use G,M;
  proc helper1() { writeln("N.helper1"); }
  proc helper1() { writeln("N.helper1 mark two"); }
  proc nCallFunc() {
    writeln("nCallFunc");
    genericfunc(1);
  }

  proc main() {
    nCallFunc();
    mCallFunc();
  }
}

Removing module M makes the bug go away.

@skaller
Copy link

skaller commented Feb 7, 2022

In this case, I think you have the wrong idea for params. A param is not generic. It should be replaced in the AST by whatever the user specified on the command line or configuration script or wherever params come from: its basically the same as a macro in C, macros are replaced by the pre-processor. I think Brad mentioned this in reference to using them for conditional compilation. However, the original problem would still exist if you replace param p with p:?1.

So here's what should happen in the example assuming p:?1: the compilation should fail immediately in module G, genericfunc on seeing the call helper1 because that function is not dependent on the generic type parameter, and it is not in scope.

POI lookup rules ONLY apply to dependent names.

mppf added a commit to mppf/chapel that referenced this issue Feb 24, 2022
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
mppf added a commit to mppf/chapel that referenced this issue Mar 4, 2022
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
mppf added a commit to mppf/chapel that referenced this issue Mar 8, 2022
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
mppf added a commit to mppf/chapel that referenced this issue Mar 17, 2022
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
mppf added a commit to mppf/chapel that referenced this issue Mar 28, 2022
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
mppf added a commit to mppf/chapel that referenced this issue May 4, 2022
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
mppf added a commit to mppf/chapel that referenced this issue May 10, 2022
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants