Chapel: Should always checking the type's definition point for methods replace or supplement traversing private uses and imports?

[lydia-duncan]

As a result of discussion on #15946, we've decided when resolving methods to first look at the scope where the type is defined, then look from the scope where the call was made. This seemed pretty straight forward (and a bit surprising that we weren't already doing it for function resolution, as it does happen for scope resolution). We think it will be a better way of fixing problems where a type instance was accessed in a scope where the type definition itself was not already visible, which is something that has frequently been encountered by users in a variety of ways. A previous strategy for resolving that problem was to follow any uses and imports available to the scopes being searched, ignoring whether the uses and imports were private when we were resolving methods - as #15946 showed, that strategy was not sufficient to prevent us from running into yet another "I have an instance but I can't do anything with it" problem.

The question for this issue is: does this new strategy seem like it should be a replacement for following private uses and imports? Or an addition to it?

Argument in favor of "replacement":

• Looking at the type's scope may be sufficient for the cases that following private uses and imports handled
• We'd arguably be more honest about what "private" means in that case
• Less special rules to keep track of when determining if a secondary method can be found

Argument in favor of "in addition":

• Jumping to the type's scope only helps with primary methods and fields. If someone is relying on private uses and imports being followed for method resolution (which could be, since that change was in the last release), removing that traversal could break their code.
• Theoretically less likely we'll have another case of a user expecting to find a symbol from a scope that isn't visible in a few months

Michael and I lean towards "replacement", but are a bit worried that doing so will cause us to run into another case.

[bradcray]

I'm definitely curious what impact "replacement" would have when run on the current test suite.

[lydia-duncan]

I can run with it and report back, after I finish the implementation of checking the type's definition.

[vasslitvinov]

"Replacement" appeals to me, too, and I'd like to know how it works on the current test suite.

If we can have "private" mean "private", it is a big bonus.

Jumping to the type's scope only helps with primary methods and fields.

I thought the proposal was "look at the scope where the type is defined". So if a secondary method definition is next to the type definition, it should be visible.

Lydia, could you clarify whether "look at the scope where the type is defined" includes only public declarations?

[lydia-duncan]

I thought the proposal was "look at the scope where the type is defined". So if a secondary method definition is next to the type definition, it should be visible.

Yes, that's a good clarification.

Lydia, could you clarify whether "look at the scope where the type is defined" includes only public declarations?

That's my intention. Well, I should say, the intention is to appropriately take private into account - so if we're resolving a method call from within another method at the same scope, that should still resolve regardless of if the callee is private or public, but if we're in an outside scope then only public ones should be visible (though this isn't immediately relevant since we don't support private methods today). E.g. I would expect this to resolve in the future:

proc Foo.one() {
  this.two();
}
private proc Foo.two() {
  ...
}

while this would probably fail (depending on how we define private methods):

module Other {
  use FooDef;
  proc Foo.one() {
     this.two();
  }
}
module FooDef {
  class Foo { ... }
  private proc Foo.two() { ... }
}

[vasslitvinov]

I suggest that given this code:

module FooDef {
  class Foo { ... }
  private proc Foo.two() { ... }
}

Foo.two not be visible outside of module FooDef, whether by hook or by crook. Allowing a secondary method on Foo in another module to access Foo.two means that anybody can bypass the privacy annotation.

[lydia-duncan]

So, here are the failures for respecting private uses and imports again:

---

Seems fine, just needs an expected output update due to candidate rearrangement:
[Error matching .bad file for distributions/ferguson/dist-assoc-bulkadd]
[Error matching .bad file for types/unions/union-copy-init3]

Needs to update the type check so that it looks at parent types as well (unsurprising):
[Error matching compiler output for functions/generic/poi/fieldDifferentModInheritance]

Probably due to needing own use of IO module for string.format (yeah, seems reasonable):
[Error matching compiler output for library/packages/TOML/test/checkParseLoop]

Test was to lock in explicitly finding secondary methods in different modules:
[Error matching compiler output for visibility/empty/qualified-diffModule-okay-except]
[Error matching compiler output for visibility/empty/qualified-diffModule-okay]

Test was to lock in explicitly finding secondary methods due to private uses:
[Error matching compiler output for visibility/private/uses/accessSecondary]

Less okay (how do you get the secondary method without all the other symbols?):
[Error matching compiler output for visibility/only/canSeeSecondaryMethod2]

---

So all in all, it seems like the impact was fairly low - the last failure is the only one that gives me pause. I am planning on waiting to resolve the inheritance failure until we decide whether to go forward with this branch or not based on that other failure.

Sidebar:

While working on this branch, I also wondered if we still needed to track module references to resolve, a solution that was implemented in #15803. Here are the additional failures from removing that

---

Can't find the appropriate candidate, only seeing one with a where clause that evaluated to false (probably needs its own use to find the candidate):
[Error matching compiler output for library/packages/LinearAlgebra/correctness/no-dependencies/correctness-Sparse-import]

Seems concerning since a candidate that should work seems already listed, but I think I'm not understanding forwarding resolution, maybe?:
[Error matching compiler output for library/standard/Heap/type/testTuple2]

---

As a result, I think we probably still need to maintain the list of modules that were explicitly named and search based on them, but I probably should think about that more deeply if we decide to still go ahead with no longer following private uses - I feel like I have a good sense of the overall picture for how we resolve methods in a "supplement" world, but haven't considered as deeply the world where we jump to the type's definition and look in modules that were explicitly named but don't follow private uses and imports (as you're probably getting, it's a complex picture)

[vasslitvinov]

Re visibility/only/canSeeSecondaryMethod2:

module OuterModule {
  use Extender only Foo;

  module Extender {
    public use FooDef only Foo;
    proc Foo.newMethod() {...}
  }

  module FooDef {
    class Foo {...}
    proc Foo.outerMethod() {...}
}

To my eye, use ... only Foo means "bring in only the symbol(s) Foo". If I want to bring Foo's methods, I need to use/import their names explicitly, or not exclude them from a use.

So, newMethod should not be visible in OuterModule.

Cf. outerMethod is visible in OuterModule is OK by our rule because it is visible from Foo's definition.

[lydia-duncan]

The trouble is that you can't really list method names in only/except lists, iirc. So there isn't really a way to only bring in the method if you don't want all the symbols in the module to be in scope

[vasslitvinov]

That sounds like a shortcoming. Is this because of implementation or did we choose this intentionally? (Issue#?)

[vasslitvinov]

One step further, what happens if we gather visible candidates the same way for methods and non-methods, with the exception of checking the type's definition point for methods as the first step?

That would simplify my POI implementation work a lot. :)

[lydia-duncan]

That sounds like a shortcoming. Is this because of implementation or did we choose this intentionally? (Issue#?)

It has rubbed me a bit in the past (but not enough for an issue) - if I remember right, it was to do with primary methods being defined within the class type and so not being visible during scope resolution when we ensure that there's at least a symbol with the given name in an only/except list (even though I believe they are more visible by the time function resolution rolls around). That said there was probably a way to work around it that I wasn't thinking of at the time.

One step further, what happens if we gather visible candidates the same way for methods and non-methods, with the exception of checking the type's definition point for methods as the first step?

That would simplify my POI implementation work a lot. :)

I didn't link my branch yet, but it does try to do some reuse of the code you split out into separate functions and it more closely mimics the ordering when it doesn't share the code. I think I want to keep it more separate for now, at least until we decide what our story with private methods looks like (one day we'll prioritize that) - if private methods end up being treated almost identically to private functions, then I think we can almost completely share the code again, but if there's something like protected going on, or if private for a method means "private to the type" rather than "private to the module where the function is defined", then we're going to have to do something different there and I'd rather we not rearrange the code until we know (to avoid unnecessary work).

[bradcray]

I think something's broken in @vasslitvinov's example above:

module OuterModule {
  use Extender only Foo;

  module Extender {
    use FooDef only Foo;
    proc Foo.newMethod() {...}
  }

  module FooDef {
    class Foo {...}
    proc Foo.outerMethod() {...}
}

So, newMethod should not be visible in OuterModule.

Cf. outerMethod is visible in OuterModule is OK by our rule because it is visible from Foo's definition.

OuterModule shouldn't be able to see Foo through Extender because Extender's use is private by default.

If it were a public use, then I'd be inclined to have newMethod as visible as outerMethod and Foo.primaryMethod() (if one were defined) because I think of the scoping of methods as being more related to the type they're defined on than the module they're defined within. That said, I would hope that a use FooDef in a scope with no visibility into Extender would only see primaryMethod() and outerMethod not newMethod() (if we can make those two statements co-exist simultaneously).

[vasslitvinov]

Yes it was broken. I edited to public use.

[vasslitvinov]

I think of the scoping of methods as being more related to the type they're defined on than the module they're defined within.

I suspect this will wide-open the door to hijacking. The difference is that the hijacker will need to have visibility of the type that the method is on.

[bradcray]

It would definitely be a way to add new methods to established types. Can you give an example where it would result in a hijack situation? (I'm not saying it wouldn't, but am not able to construct one myself offhand).

[vasslitvinov]

module FooDef { class Foo {...} }

module Lib1 { use FooDef; proc Foo.run(arg) {...} }

module Lib2 { use FooDef; proc Foo.run(arg: int) {...}
                          proc Foo.other() {...} }
module Application {
  use FooDef; var myFoo = new Foo();
  proc dosomething1() {
    use Lib1;
    myFoo.run(1);   // Foo.run() from Lib1 is expected
  }
  proc dosomething2() {
    use Lib2;
    myFoo.other();
  }
}

With class-based visibility, the call run(1) will see Foo.run() definitions in both Lib1 and Lib2. It will choose the latter as more specific, against user expectations.

Foo.run() in Lib2 will cause trouble even if it is added in a later version of Lib2 after Application has been developed and tested.

[bradcray]

Would our overload set warning not catch this case? It seems similar to what it I think of it as being designed to help with: identifying overloads of a function from distinct sources where it's not clear whether the shared name was intentional or not.

[vasslitvinov]

Yes, it would catch, good point.

However, if we are adding overload-set checking to the mix, we are giving up "the scoping of methods as being more related to the type they're defined on than the module they're defined within". Which is what got us here to begin with.

Alas, overload-set checking has a more technical problem. Consider this example:

module FooDef { class Foo {...} }

module Lib1 {
  private use FooDef;
  proc work1() {
    (new Foo()).run(1);
  }

  proc Foo.run(arg) {
    .....
  }
}

module Lib2 {
}

module Application {
  proc dosomething1() {
    use Lib1;
    work1();
  }
  proc dosomething2() {
    use Lib2;
  }
}

So far so good. Now you can see where I am heading...

The developers update to the next version of Lib2. Which happens to have its own version of run()...

module Lib2 {
  private use FooDef;
  proc Foo.run(arg:int) {
    ...
  }
}

Not only the application gets an overloaded-sets error, there is also no way to fix it! The only way out is to change something about Lib1 (ex. clone it and rename run()) or about Lib2 (ex. revert to earlier version).

While there: when we go directly to a module that is not used/imported, we take away the ability to rename its methods at the point of use.

[lydia-duncan]

Attempting to summarize, it sounds like we would like to have some way to get secondary methods in another module without having to bring every symbol from that module into scope. It sounds like Vass would like us to be able to name methods as part of an except/only list (and to allow them to be renamed), and that always bringing the methods in is worrisome because of potential hijacking and overload set issues.

In the past, we had a solution in the compiler that tracked names that were related to a type if the type was in an only list (or not excluded via an except list). It was extended in #10439, and removed in #15003 when we decided to follow private uses and imports for method resolution. I believe we could restore that last version of the concept - my main reservation is how this still feels like whack-a-mole, but I don't really know of a way to avoid that while still finding methods for types we can't see directly.

[lydia-duncan]

@bradcray - have you had a chance to look at the latest comment on this issue yet?

[bradcray]

Yes, I have.

[lydia-duncan]

So should I interpret that as "yeah, let's restore the old related names tracking implementation and see what happens"?

[bradcray]

No, I don't currently have a clear vision as to what I think we should do here.

[lydia-duncan]

Closing this issue, since the original question has been answered (we went with "replace", but we do have some questions about operators remaining - that should be handled separately, though)