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

Overloading paren-less vs. paren-ful routines should result in an ambiguity error #18177

Closed
bradcray opened this issue Aug 7, 2021 · 4 comments

Comments

@bradcray
Copy link
Member

bradcray commented Aug 7, 2021

Summary of Problem

For years, I've been mistakenly thinking and saying that Chapel would give an ambiguity error when a program contains overloads of a routine with and without parenthesis since, in cases like the following, there's no clear answer as to which routine should be called:

proc foo {
  return [1.0, 2.0];
}

proc foo(x: int) {
  return x+1;
}

writeln(foo(1));

However, in Chapel 1.24.1 and a few other versions I've checked back to Chapel 1.10.0, I'm finding that instead it just arbitrarily picks one of them (seemingly whichever overload is defined second?). This seems problematic to me since it can represent confusion on the part of a user and is somewhat similar to overloading a variable and a procedure or a field and a method.

It can also come up in more subtle cases like class hierarchies:

class C {
  proc foo {
    writeln("In C.foo");
  }
}

class D: C {
  proc foo(x: int) {
    writeln("In D.foo");
  }
}

var myD = new D();
myD.foo(1);

where it seems we pick the parent class method.

Configuration Information

  • Output of chpl --version: chpl version 1.25.0 pre-release (2bf652fc09)
@mppf
Copy link
Member

mppf commented Aug 9, 2021

I'm finding that instead it just arbitrarily picks one of them (seemingly whichever overload is defined second?)

That is odd.

We could consider fixing this bug in the new resolver and leaving the old resolver. I'm somewhat worried that the current behavior (at least in the case where the order matters) is relying on some interaction between scope resolution and function resolution that might be hard to tease out).

@bradcray
Copy link
Member Author

bradcray commented Aug 9, 2021

I'm going to look into what it would take to fix it in the current compiler as I think it's pretty terrible behavior, and suspect it could be low-hanging fruit. Specifically, I'm not sure it is a scope resolution vs. function resolution issue. E.g., here's the --explain-call output for the orderings of the functions for the code in the OP:

testit.chpl:11: note: call: foo(1)
testit.chpl:1: note: visible functions are: foo [210494]
testit.chpl:6: note:                        foo(x: int) [210535]
testit.chpl:6: note: candidates are: foo(x: int) [210535]
testit.chpl:6: note: best candidate is: foo(x: int) [210535]

@bradcray
Copy link
Member Author

bradcray commented Aug 9, 2021

Ah, but I think you're right that the fact that the order matters probably implies that scopeResolve is involved. But it also makes me think that it shouldn't be too difficult to make scopeResolve treat both cases the same since function resolution sees both versions above.

mppf added a commit that referenced this issue 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
@bradcray
Copy link
Member Author

I'm closing this issue because the first case has gotten better in dyno, and the second case is different / unique enough (while also having seemingly changed behavior) that I forked it off into its own issue (#22768). That said, there is also an inconsistency between overloaded methods with different paren-ful-ness when defined within a type, which I've forked into #22767.

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

2 participants