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

bug: public use should not bring in module name for function call #21640

Open
mppf opened this issue Feb 21, 2023 · 0 comments
Open

bug: public use should not bring in module name for function call #21640

mppf opened this issue Feb 21, 2023 · 0 comments

Comments

@mppf
Copy link
Member

mppf commented Feb 21, 2023

Summary of Problem

After PR #19306, public use should not bring in the module name. However, it still is, for some cases with function calls.

Note: this bug is already resolved in the dyno scope resolver.

Steps to Reproduce

Source Code:

module A {
  proc foo(x: int) {
    writeln("A.foo ", x);
  }
}
module C {
  public use A;
}
module D {
  use C;

  proc main() {
    C.foo(1);
    C.A.foo(2);
  }
}

Here is another similar example

module C1 {
  var c1Symbol: int;
}

module C2 {
  var c2Symbol: bool;
}

module C3 {
  var c3Symbol = 3;
}

module B {
  public use C1, C2, C3;
}

module A {
  proc main() {
    use B;
    writeln(B.C1.c1Symbol);
    writeln(B.C2.c2Symbol);
    writeln(B.C3.c3Symbol);

    writeln(B.c1Symbol);
    writeln(B.c2Symbol);
    writeln(B.c3Symbol);
  }
}

Associated Future Test(s):
test/visibility/use-no-qualified/public-use-no-mod3.chpl #21637

mppf added a commit that referenced this issue Feb 23, 2023
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
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

1 participant