Chapel: Add compiler warning when `use` statement is not at start of scope

[BryantLam]

Feature request for a compiler warning. Low priority until someone else runs into this behavior.

Create a warning for #12744 when a use statement is not at the start of scope. Otherwise, bugs will occur when users encounter this behavior.

module A {}
module B { var x = 13; }

var x = 42;

proc main() {
  writeln(x); // 13
  use A;
  use B;
}

[bradcray]

To me, this seems more related to the plan to support a Python-style import that makes nothing available by default than to where the use statement occurs. I.e., I don't think putting the use statement at the top of the scope would make a user any less surprised that they were referring to the wrong x if they didn't realize that module B defined one? Or are you thinking that they're well-aware that B defines an x but didn't expect it to go into effect until the use was reached? If that's the case then issue #13042 seems much more useful to me.

Maybe put another way, I think it's often useful to put a use M only foo; statement into the middle of a block near where foo is needed similar to how it's useful to declaring variables where they're needed rather than being required to declare them at the top of a scope as in old-style C, so wouldn't want a warning barking at me if I chose to use that style. It could be an opt-in warning of course, but then it's likely that users wouldn't know to opt into it until they'd been bitten (again making #13042 seem more useful).

[BryantLam]

To me, this seems more related to the plan to support a Python-style import that makes nothing available by default than to where the use statement occurs.

Agreed.

Maybe put another way, I think it's often useful to put a use M only foo; statement into the middle of a block near where foo is needed similar to how it's useful to declaring variables where they're needed rather than being required to declare them at the top of a scope as in old-style C, so wouldn't want a warning barking at me if I chose to use that style. It could be an opt-in warning of course, but then it's likely that users wouldn't know to opt into it until they'd been bitten (again making #13042 seem more useful).

This is where I disagree with the rationale for why use or import statements should be allowed in the middle of a block. Is a naive user going to be surprised about the behavior of this code?

module A {
  var keepWorking = false;
}
var keepWorking = true;
proc main() {
  while keepWorking {
    writeln("critical operation!");
    var someCondition = true;
    if someCondition {
      break;
    }
  }

  use A only keepWorking; // Remark: Why, user?! Why? Such bad code...
  if keepWorking {
    writeln("operation just for A");
  }
}
// Hint: Nothing gets executed.

The proposed warning would force the user to introduce an indented scope if they want a use statement in the middle of their code. In fact, that could even be part of the warning; tell them to introduce a scope if they really want to use a module where they put that statement.

[BryantLam]

Ironically, I hit a problem when abiding by this guideline. The following code broke with a nested scope:

var e: [1..8] int = 1..8;
{
  use UnorderedCopy;
  // UnorderedCopy or its dependencies has a symbol conflict on `e`.

  ref eArray = e; // error: Cannot set a reference to a param variable.
  writeln(eArray);
}

This error took a bit for me to debug because the compiler error didn't have a note on where the param variable was. This error is easily remedied:

1. Recommended workaround: use M only
2. Feature request: use with private by default / Python-style import
3. De-scope the block much to my disappointment in this instance.

[bradcray]

I agree that this example is bad news / super confusing.

This is a pretty interesting case because UnorderedCopy doesn't have any (explicit) dependencies (i.e., no use clauses within it). So then what's going wrong? The Chapel compiler auto-inserts uses into every typical module (user, standard, or package) to result in the auto-use of things like I/O, Math, etc. And then this bites you because the Math module defines a param e... and because uses are currently always transitive.

I think the fix here is to implement issue #6093 (adding support for private, non-transitive uses), and then to have the compiler-generated uses of the standard modules be a private use.

[bradcray]

I've forked off issue #13118 to capture the case Bryant ran into just above.

[bradcray]

Just noting for anyone who didn't subscribe to my forked-off #13118 that the issue Bryant ran into above is now resolved on master due to @lydia-duncan 's PRs #13372 and #13253 (as well as other supporting PRs).

[mppf]

Somehow

proc f() {
  innerFn();
  proc innerFn() { }
}

seems fundamentally less confusing to me than

proc f() {
  someFn();
  use M; // assuming M defines someFn
}

It looks like a trade-off between reducing one arguably surprising use of modules and making them in some way more consistent with locally-defined symbols. Which would be more useful to easy understanding of the language itself and code written in it?

Personally, I think the use M case above is fundamentally different from the proc innerFn case, because the use M does not have to specifically name what symbols it makes visible. (For this reason, import M; does not seem to be nearly as problematic).

Anyway, the rule can't possibly be "use has to be at start of scope" because interpreted literally that would disallow

proc f() {
  use A;
  use B; // not at start of scope
  ...
}

I think instead the proposal I would advocate is that the compiler emit a warning if code relies on a symbol brought it by a later use statement. Or, alternatively, to not allow the code before the use statement to have access to the symbols it brought in at all. Either of these would still allow a use statement in the middle of a block near where it is needed.

[BryantLam]

I think instead the proposal I would advocate is that the compiler emit a warning if code relies on a symbol brought it by a later use statement. Or, alternatively, to not allow the code before the use statement to have access to the symbols it brought in at all. Either of these would still allow a use statement in the middle of a block near where it is needed.

I'm okay with both proposed solutions. I would advocate for the second. This issue was a consequence from the confusing behavior I found in https://github.com/chapel-lang/chapel/issues/12744. Maybe there's more support for changing this behavior now that module symbols are treated a bit uniquely as of 1.20.