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

Public and private use statements #6093

Closed
lydia-duncan opened this issue Apr 21, 2017 · 21 comments · Fixed by #13253
Closed

Public and private use statements #6093

lydia-duncan opened this issue Apr 21, 2017 · 21 comments · Fixed by #13253

Comments

@lydia-duncan
Copy link
Member

Today, uses of modules spread transitively through use chains, meaning that the following code works:

module A {
  var a = 10;
}
module B {
  use A;

  var b = 12;
}
module C {
  use B;

  var c = a + b; // "a" is visible here because it was visible to module B
}

While this means that Chapel code which relies on groups of modules that work together ultimately need to write fewer explicit use statements, it is easy to see situations where a module writer would like to not give their clients access to the modules they themselves require. To provide this control, we would like to be able to specify a use statement of the form:

private use Foo; // or
public use Bar;

similar to how symbols defined within a module can be declared as public or private. The keywords public and private will not be required, and if omitted will use the default setting.

Should the default visibility of a use statement remain public? We plan on going through our use statements in the library and built-in modules and making a decision based on both that and user input.

The big question though is in regards to our point of instantiation rules. Today, Chapel instantiates generic functions completely at the call site - on the one hand, this allows them to be called on types that were not available from the scope in which the function is defined:

module A {
  proc foo(arg) { ... }
}

module B {
  use A;

  class MyClass { ... }

  ...
  var c = new MyClass();
  foo(c); // Works because of point of instantiation
  ...
}

On the other hand, it allows function calls within the body of the generic function to be hijacked to a more local version at the call site, potentially causing security risks. Our instantiation of generic functions works today in part because of the transitivity of module uses as described above. Changing the behavior of module uses is a good time to revisit our rules about point of instantiation, to ensure that this set up continues to be the right decision going forward.

@mppf
Copy link
Member

mppf commented May 9, 2017

re public & private use, another way to look at is something like Python's import, where the symbols are available only when prefixed by the module name. Such a thing would break most transitive uses.

re. point of instantiation rules, I wonder if we could prefer the version at the definition point and fall back on a version at the call site if the required symbol is not available at the definition point?

@lydia-duncan
Copy link
Member Author

For the public/private comment, that would be providing the same access as we do today: you can access a symbol in another module with the module name prefix as long as Chapel knows the module exists. If we wanted to tighten up imports like what you are describing, we would probably want to require use statements to make those accesses, and I think we didn't want to do that when we last discussed it

For the point of instantiation rules, that could work. Though maybe the intention is to allow for an overload for the specific local type (to prefer foo(x: bar) at the callsite over foo(x) at the definition point). There's more details to this situation that I haven't been able to dive into yet (though I have 20 tabs open on it in another browser window)

@mppf
Copy link
Member

mppf commented May 15, 2017

@lydia-duncan - I happened to be poking at a related area in the compiler and I noticed a problem with point-of-instantiation (where it is not implemented correctly). See issue #6252. I'd like to discuss more what (if any) alternatives there are to point-of-instantiation. However I suspect we'll want to have such a discussion is some point-of-instantiation issues (probably not on this issue).

@mppf
Copy link
Member

mppf commented May 19, 2017

See #6272 adding CHIP 20 about point-of-instantiation.

@bradcray
Copy link
Member

@lydia-duncan / @mppf: Given that Bryant (arguably) ran into this as mentioned on issue #13041 (which I've forked off to a standalone issue #13118), I'm wondering this week what it would take to implement private uses. I know the reason that we originally felt we had to make use transitive by default was due to point-of-instantiation challenges or concerns. But I also think that our world has changed a bit w.r.t. point of instantiation since then, so have lost track of whether we believe we have a clear path forward for this item or not. Do either of you have a sense of that?

@lydia-duncan
Copy link
Member Author

Iirc, the path forward is relatively straight-forward, but if we want to flip the switch on whether use statements are public or private by default, there's going to be a fair bit of fallout to dig through.

@bradcray
Copy link
Member

I'm not proposing switching the default (at least at this point); just looking to provide the option to have a private use (and probably public use as well, though that would be no different than what we have today).

@BryantLam
Copy link

BryantLam commented May 29, 2019

A note of caution as the proposed change would impact the module discussions in #12923 (comment) and #10946.

public use means something completely different in Rust. In Rust, "pub use" is for re-exporting symbols from one scope to another, which is a handy feature for doing internal refactoring of code without changing the exposed public API (the symbols that are moved can be re-exported to the top package scope from its new location). For example:

// Rust
// I don't think Chapel has a way to do something similar.
mod A {
    pub use self::B::*;
    mod B {
        pub use self::C::*;
        mod C {
            pub fn hello() {
                println!("Hello!");
            }
        }
    }
}

fn main() {
    use A;
    A::hello();
}

I'd personally advocate for default-use-is-private (or import if changing the default on use is too unappealing). Also, I'd want a more powerful wildcard -- it would let us get rid of the standout use Enum behavior..

module A {
  var a: int = 1;
  module B {
    var b: int = 2;
  }

  enum Color {
    red, green, blue
  }
}

//
// Today's Chapel with default-use-is-public
//
{
  use A;
  writeln(a);
  writeln(B.b);
}
{
  use A.Color;
  writeln(red);
  // I understand why this behavior exists, but ...
}
{
  use A only Color as C;
  writeln(C.red); // Better for defensive programmers.
}

//
// Modified Chapel with default-use-is-private
//
{
  use A;
  writeln(A.a);
  writeln(A.B.b);
}
{
  use A.B;
  writeln(B.b);
}
{
  use A.Color;
  writeln(Color.red);
}
{
  use A.Color.*;
  // More wildcard support. Emulates "public" behavior if truly desired.
  writeln(red);
}

@bradcray
Copy link
Member

bradcray commented May 29, 2019

public use means something completely different in Rust.

I wasn't understanding what was completely different between this example and what Chapel does today before transliterating. For those in a similar boat, the answer is that while this would work:

module A {
    use B;
    module B {
        use C;
        module C {
            proc hello() {
                writeln("Hello!");
            }
        }
    }
}

proc main() {
    use A;
    hello();
}

And this would work:

  C.hello();

This would not:

  A.hello();

because we still consider hello() to be in module C even though A has made it available through the transitive/public uses; we don't think of A as defining hello() itself.

@bradcray
Copy link
Member

Thinking about the above use-case some more on my way home tonight, it seems to me that forwarding would be the more Chapel-consistent way to have C's symbols be directly available within A:

module A {
    forwarding B /* optionally: only hello */;
    module B {
        forwarding C /* optionally: only hello */;
        module C {
            proc hello() {
                writeln("Hello!");
            }
        }
    }
}

proc main() {
    use A;
    A.hello();
}

@mppf
Copy link
Member

mppf commented May 30, 2019

Another syntax option would be something like use B.* as . or something along those lines.

@lydia-duncan
Copy link
Member Author

I'd personally advocate for default-use-is-private

I suspect we all feel that way to varying degrees. But the fact that we've been in the other world for so long means that there will be fallout from making the switch. Still, I would strongly advocate for it being a pre-Chapel 2.0 decision.

@lydia-duncan
Copy link
Member Author

I think I prefer @mppf's renaming strategy to expanding the cases where forwarding is used, but I won't lose sleep over it

@bradcray
Copy link
Member

I'd personally advocate for default-use-is-private

The proposed import variant (#13119) could be private by default (which is what you proposed as well when reading beyond this excerpt).

bradcray added a commit that referenced this issue May 30, 2019
Add a test showing that innoucuous `use`s can pollute namespaces

[trivial, not reviewed]

This test demonstrates that because of the `use ChapelStandard`
injected by the Chapel compiler to get automatically-included modules,
a module that looks completely innocuous like `Library` here can
eclipse user-level variables inadvertantly.  I believe the fix here is
to implement	`private` (non-transitive) `use` statements (issue #6093)
and then to have the compiler-injected `use ChapelStandard` be a
`private use`.
@lydia-duncan
Copy link
Member Author

I found some old tests I wrote back when we first thought about this feature, so would like to run the more controversial of them by people before locking a particular behavior in.

Test A (access of secondary method):

definesSecondary.chpl:

module TypeDefiner {
  class Foo {
    var a = 1;

    proc methodB(x: int) {
      return a + x;
    }
  }
}

module SecondaryDefiner {
  use TypeDefiner;

  proc Foo.other() {
    writeln("I'm a secondary on Foo!");
  }
}

module User {
  private use SecondaryDefiner;

  // Ensures that using a module privately doesn't break secondary methods
  // defined in modules which don't contain the original type definition.
  proc main() {
    var foo: Foo = new Foo(3);
    writeln(foo.methodB(2)); // Should be 5
    foo.other();
  }
}

accessSecondary.chpl:

use User; // from definesSecondary.chpl
use TypeDefiner; // same

// Verifies that you cannot make use of secondary methods obtained via
// another module's private use.
var foo: Foo = new Foo(5);
foo.other();

I expect this pair of .chpl files to give an unresolved call error when accessSecondary is the main module, and to work when the first file is compiled on its own. Do folks agree?

Test B (access of type defined in a privately used module):

includesType.chpl:

module Inner {
  class Foo {
    var a = 1;

    proc methodB(x: int) {
      return a + x;
    }
  }

  proc Foo.other() {
    writeln("I'm a secondary on Foo!");
  }
}

module User {
  private use Inner;

  // Verifies that making a use private doesn't interfere with the availability
  // of types defined in that use to the scope which uses it.
  proc main() {
    var foo = new Foo(11);
    writeln(foo.methodB(4));
    foo.other();
  }
}

accessType.chpl:

use User; // from includesType.chpl

// Verifies that you cannot make use of types obtained via another module's
// private use.
var foo: Foo = new Foo();
writeln(foo.a);

I expect this pair of tests to give an error finding Foo when accessType is the main module, and to work when the first file is compiled on its own. Do folks agree?

@mppf
Copy link
Member

mppf commented Jun 13, 2019

Test A: definesSecondary.chpl seems reasonable when compiled on its own. accessSecondary.chpl looks like it has errors any time it's compiled (main module or not) because foo.other() will not resolve.

Test B: Similarly, includesType.chpl seems reasonable when compiled on its own. accessType.chpl has an error because Foo is not a visible symbol in it (main module or not).

@BryantLam

This comment has been minimized.

@mppf
Copy link
Member

mppf commented Jun 14, 2019

Is private use equivalent to use only

No, private use just means that the symbols now available from use are private to the module using them. private use A; still makes all the (public) symbols in A visible in the module doing the using, so it can do e.g. someFunctionInA().

Not importing all of the symbols and moving to a default supporting things like use Inner; .. Inner.Foo is another issue.

@bradcray
Copy link
Member

Not importing all of the symbols and moving to a default supporting things like use Inner; .. Inner.Foo is another issue.

(specifically issue #13119).

@lydia-duncan
Copy link
Member Author

@BryantLam - the important thing about a private use is its impact on clients of the module with the use statement. It means that you can bring the symbols of A into your module's scope for your convenience without worrying about their impact on your users.

@lydia-duncan
Copy link
Member Author

That's why the distinction between accessType.chpl and includesType.chpl is important - we expect private use to impact accessType as a client of the module in includesType with the use statement, but not to impact includesType itself.

lydia-duncan added a commit that referenced this issue Jun 17, 2019
Support public and private use statements
[reviewed by @benharsh]

Prior to this change, all use statements were public, with no option to
change that.  This meant that symbols needed for your module would
get bled into your users' namespaces.  For instance, in the following
code:
```chapel
module A {
  proc someFunc() { ... }
}
module B {
  use A;
  ...
}
module C {
  use B;
}
```
module C would have `someFunc` as part of its namespace.  This could
lead to problems like name conflicts, shadowing due to one function having
a more specific signature, etc.

We would frequently work around this by limiting the scope of the
use statement, e.g. putting the use inside of the function where the
symbols were needed - but this could lead to redundant use statements
in the case where multiple functions in a module needed the same
module.

This change adds the ability to declare a use statement as public or
private.  Public is still the default (though we may look into changing
that).  Declaring a use as private will mean that the symbols it brings
into scope are only accessible by the scope in which the use is declared.

The changes to support it are fairly minor.  We needed to add parser
support and a new field to UseStmts (impacting how we create them),
and then check that field when performing scope and function resolution.
The most effort was put into function resolution, where we needed to
check the visibility relative to the scope where the call occurred, and
relative to the instantiation point of the call if it was within a generic
function.

I also renamed a method on ModuleSymbol - the method would drop
use statement information about pretty much everything except the
module being used on the floor, but won't cause problems since it is
only used by dead code elimination.  In discussing with Michael, we
don't like that it does this, but don't think it is worth taking on the
effort to fix it until it causes problems.  I left a note at the offending
line for if we get back to it - hopefully renaming will ensure that no
one tries to use it when doing so would cause problems.

Resolves #6093 

This change adds 30 tests of the new feature.  They check:
- except lists on the private use to ensure the avoided symbols are not
accidentally exposed, both in the module with the private use and in
any clients of that module
- private use statements containing multiple modules, to ensure that
all modules used are used privately
- only lists on the private use to ensure the symbols that aren't named are
not accidentally exposed, both in the module with the private use and
in any clients of that module
- renaming symbols via a private use, to ensure that the rename works in the
module with the use, and that the new name is not visible to clients of that
module
- privately using a module that defines a secondary method, to ensure the
method is not exposed beyond the module with the private use.
- privately and publicly using the same module so that some symbols are available
to clients and the rest only to the module with the use statement
- privately using a module that defines a type, to ensure the type is not exposed
beyond the module with the private use
- that privately using a module with a public use doesn't behave badly
- and various point of instantiation tests that are difficult to describe succinctly
(ran them by Michael to be sure I wasn't crazy, though most of them resemble
tests that already exist and are not a serious departure from them)

Passed a full paratest with futures.

Later todos:
- modify error message so that it isn't confusing when a symbol is not found
because of privacy
- private uses by default?
- private use of:
  - root module
  - ChapelStandard
  - Fortran module
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants