Should it be possible to use modules with conflicting symbols?

[dlongnecke-cray]

Should it be possible to use modules with conflicting symbols?

Context: While bug-fixing the dyno scope resolver, I encountered a
semantic conundrum.

Specifically, if I define the symbol x in module A and then
use the module B which also defines x, what should happen?

When should an error occur, and when is the code OK? If an error
should occur, then where should it occur? At the point of use or
import? At the point of definition for x (which one)? At the
point of mention for x (e.g., writeln(x))?

Here's the code:

module B {
  var x = "B";
}

module A {
  var x = "A";

  // Here are some different things we could do at this point:
  // public use B;
  // import B.x;
  // import B.x as x;
  // public import B.x;
  // public import B.x as x;

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

And here's what happens today for any flavor:

TestRedef.chpl:6: error: symbol x is multiply defined
TestRedef.chpl:2: note: also defined here

Note that the behavior for plain-old use B is correct, and prints
A (no concern about private use). For documentation, see:

https://chapel-lang.org/docs/language/spec/modules.html#conflicts

Errors will not occur if an ambiguous symbol is not mentioned

Looking at production compiler code - errors are triggered at the
point-of-mention. For every writeln(x) I add to the code, I get
another error and note.

Point-of-mention has an interesting semantic consequence. Today, if I
remove the writeln(x), I will no longer see an error, and the
program will compile, regardless of if any (or ALL) visibility
statements are used!

Erroring at point-of-mention vs import/use

If errors are at import/use then users will get the errors
as early as possible. This is useful if you are a library author,
because any conflicts between library components become evident
immediately.

If errors are at point-of-mention then an error will only occur
if an ambiguous symbol is actually used. This seems more useful
for a developer who is authoring an end-program.

Key difference between import and use

In any import statement, the set of symbols imported are either
known (e.g., import B.x introduces one symbol named x), or if
the entire module is imported, it is prefixed so as not to be
ambiguous (have to write B.x).

When a use occurs, the entire contents of the used module are
dumped into a shadow-scope.

In other words, in an import, the set of possible conflicts are
known and usually small in number. In a use, they cannot be known
without first collecting all the symbols introduced by the use.
This number of symbols can grow quite large.

Should errors be at point-of-mention or at import/use?

Since the set of symbols in an import are known, it seems reasonable
to emit redefinition errors for import at the import statement
itself, regardless of whether or not the import is public or
private.

This leaves only public use. Because the intention of public is
to make the symbols available for outside use (e.g, as a library
author), it also seems reasonable to emit redefinition errors at
the point of public use rather than the point-of-mention.

[lydia-duncan]

I don't have a problem with making errors occur at import/use. However, I do feel like falls under the umbrella of "do we error for code that is written and not used", so think it would be consistent to adjust it at the same time as other errors of that kind. If my memory serves, we have the same problem for functions with the same signature defined in the same scope as long as they aren't called, right?

[mppf]

My opinion on this (which I think David knows already):

I think it makes good sense to have the errors at the import/use for all imports and public use (since a library author would like to know this information before shipping their library).

I'm pretty convinced that we need conflicting symbols from multiple private uses to occur at the point-of-mention. In particular, the code that is useing separate private modules doesn't necessarily have control over the modules being used (they could be separate libraries) and making sure they have no symbol name conflicts seems like busy-work if none of the conflicting symbols are actually used. Also, if we decide that they should occur at the (private) use statement, then it would seem that we are backing away from the strategy of using qualified access to resolve ambiguities, like Module1.foo(), and that feels like a big change.

[bradcray]

I agree with Michael here. Is this what we already have implemented? (I'd guess so, especially after recent improvements) If not, what remains? If so, can we close this?

[mppf]

AFAIK, the production compiler only considers this class of error an ambiguity (when the symbol is used). David made the issue here because we were talking about how the dyno scope resolver should do this checking. We are thinking that the dyno scope resolver should emit "multiple declaration" errors even if there is no mention of the conflicting variable -- with the exception of private use as I described above. That is slightly tricky to do, so the point of this issue is to try to understand what we are implementing before we go and do it.

Note that this could be viewed as a language change. For example, here are two programs that compile today but won't compile with the proposed direction here:

module A {
  var x: int;
}

module B {
  var x: real;
}

module Main {
  import A.x;
  import B.x;
  // but, x never used

  proc main() {
  }
}

module A {
  var x: int;
}

module B {
  var x: real;
}

module Main {
  public use A;
  public use B;
  // but, x never used

  proc main() {
  }
}

I do not think it is by any means a big language change, however. Perhaps we can argue that the change here represents a bug fix.

[bradcray]

OK, thanks. For some reason, I was thinking after your recent changes to use, we were now issuing errors for these cases. I agree that if we document this as the intended behavior, we could consider it a bug fix if it doesn't happen that way until dyno comes on-line (edit: and if we file a public issue bug for it to document the expected change, even better).

[mppf]

@dlongnecke-cray - would you be able to check the language specification in this area and make a PR to improve it, if it is unclear about what would happen in these cases? Thanks

[mppf]

I have been implementing this for the dyno scope resolver (i.e. --dyno) in #21734. I wanted to bring up the test changes I made to support it. None of these give me pause.

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)