Chapel: Nested module name sharing

[lydia-duncan]

Summary of Problem

Today, it is legal to name an inner module the same as its enclosing module, both in the explicit case and in the case of the implicit module name generated from the file name. This leads to confusion, though, when you think you are "using" the top level module but are actually "using" the file level module that includes it.

Question Should this be:

• allowed?
• a compiler warning? (perhaps one that can be turned off with a flag?)
• a compiler error?

Steps to Reproduce

Source Code:
In a file SameName.chpl:

var topVar = 13;

module SameName {
  var inner = 7;

  module SameName {
    var innerinner = 3;
  }
}

In order to access inner, you must write either:

use SameName;
use SameName;

writeln(inner);

or

use SameName.SameName;

writeln(inner);

Compile command:
chpl useSameName.chpl

Associated Future Test(s):
pending

Configuration Information

• Output of chpl --version: Chapel 1.15

[lydia-duncan]

@panzone brought this up in chat. @mppf sounds in favor of an error message. I'm leaning towards just a warning.

[mppf]

I think it should be a compiler error specifically in the case when the outer module is one the compiler added. In other words I think this should be an error:

Test.chpl:

writeln("Hello");
module Test {
  writeln("Goodbye");
}

because the compiler is defining an implicit outer Test module for the user, but the user almost certainly doesn't expect that, based upon what they are writing.

[lydia-duncan]

I agree that they didn't expect it, but we do specify that we will insert an enclosing module with the same name as the file if the file contains top-level contents that are not a module declaration, so their confusion is due to not having internalized the details of how that occurs

[lydia-duncan]

In all likelihood, they don't want anyone to write "Test.Test" to access their module, but I'm not sure we should explicitly prevent them from doing so if they really, really want to be that annoying

[mppf]

That's true but I was very confused by the module system when I was learning Chapel. Anything we can do to help new users with such problems is probably worth it.
Regarding somebody who actually wanted users of the module to write Test.Test, it's easy enough under my proposal, they just have to write

module Test {
  writeln("Hello");
  module Test {
    writeln("Goodbye");
  }
}

[bradcray]

I'd be up for at least a warning (error seems severe given that it's technically legal to shadow symbols -- similar to what Lydia is arguing here).

At times, we've also talked about (and I thought, implemented, for some case) a warning for the more general issue of having file-scope code outside of a named module for some patterns that seemed likely to be a mistake/misunderstanding). The latter might be more robust since a user may have named their file hello-1.1.chpl and their module hello; but it's also harder to know exactly under what conditions to issue the warning.

[mppf]

I'd be OK with a warning but I think the problematic case has to do with compiler-added modules in particular.

[bradcray]

That's true but I was very confused by the module system when I was learning Chapel. Anything we can do to help new users with such problems is probably worth it.

As a new user, I'm guessing you would have paid attention to warnings?

[lydia-duncan]

I can limit the warning to just compiler inserted ones, just wanted to be sure we didn't want to apply it more broadly

[lydia-duncan]

Should we add a flag to turn the warning off?

[mppf]

I don't think opting out of the warning is necessary if it's limited to compiler-added modules (see my workaround described above)

[lydia-duncan]

If the file is pretty large, though, maybe they don't want to have to indent everything?

[mppf]

You don't have to indent everything to use an outer module.

[bradcray]

What does indentation have to do with anything?

[lydia-duncan]

It's purely style related.

[mppf]

Nested modules with the same name are almost certainly a style faux-paus so I wouldn't complain there :)

[lydia-duncan]

Okay, so no flag, but make it a warning. I'll give others a chance to comment, but will aim for having it ready for review by the end of the day.

[bradcray]

Here's the existing warning I was thinking of: topleveluse.chpl. I think the rope we tried to walk here was "if the file-scope module only contains non-executable code, we should issue the warning. Unfortunately, variable declarations are hard to think of as non-executable code since their initializers may do arbitrary things... (and maybe someone wants an outer module just to contain declarations of variables?). If only we had user-mind-reading capabilities...

[lydia-duncan]

Huh, interesting. If that had been applied to require statements as well, that probably would have helped @panzone and @saru95

[bradcray]

Extending it to handle requires would make a lot of sense.

[lydia-duncan]

Would that be sufficient, or should I also have the "same name" warning?

[lydia-duncan]

(I guess that's a question for Andrea and Sarthak)

[mppf]

I think we should do both if it's not too hard.

[bradcray]

Was their example different than the one you showed above? If so, and it was a require rather than a variable declaration, I'd definitely extend the existing warning to include requires. I find the "same name" warning to be a bit special/fragile if extending the existing warning would have helped their case. (Esp. since we used to get user questions about this fairly frequently and haven't that I can recall since the original warning was added). But I wouldn't object if it magically appeared.

[mppf]

yes but just had top-level requires and then a module: https://github.com/panzone/crypto-chapel/blob/7e13009dc7fa07c82a4a8a2b4d0aa48c08bd4933/Crypto.chpl

[lydia-duncan]

I think solving both issues will be easy, so I am also inclined to do both

[lydia-duncan]

@mppf - #6813 solves the original warning. I was having some difficulty with the "require" warning, so decided to make it a separate PR. Would you mind reviewing the implicit module PR?

[lydia-duncan]

6835 partially handled the 'require' warning. I'm going to consider this issue closed and open a new one to track completing the 'require' warning