Enforce private and protected access modifiers for class member access

[brymer-meneses]

Print diagnostics for invalid class member access. This doesn't take into account compound member access.

[brymer-meneses]

I'm also not sure whether it's best to split the test file into multiple files for this feature.

[jonmeow]

I'm focusing for the moment on the way name lookup is done in DiagnoseInvalidMemberAccess's get_member_info versus Context::LookupQualifiedName (called by LookupMemberNameInScope).

Hash table lookup is typically going to be a little more expensive, because it's indirect. Sometimes it's unavoidable, but had you considered either implementing as part of LookupQualifiedName, or making more direct use of its results in order to avoid extra lookup?

Note, Context::LookupQualifiedName currently can't have multiple extended scopes, but it may end up having ambiguities which are non-ambiguous when accounting for access control.

That said, I think already the separate implementations create a risk of divergent name lookup results. In particular, how about a test such as:

base class A {
  fn F();
}

base class B {
  extend base: A;
  private fn F() -> i32;
}

base class C {
  extend base: B;
  fn G[self: Self]() -> () { return self.F(); }
}

Here, F() should resolve to A::F and compile successfully. The difference in return types should help identify if DiagnoseInvalidMemberAccess returns success while LookupQualifiedName continues to return B::F.

[brymer-meneses]

had you considered either implementing as part of LookupQualifiedName, or making more direct use of its results in order to avoid extra lookup?

access_kind is not exposed through LookupQualifiedName so I had to expose most of the logic to another function. I'm not sure if this is the right approach because of the code duplication.

[brymer-meneses]

Here, F() should resolve to A::F and compile successfully. The difference in return types should help identify if DiagnoseInvalidMemberAccess returns success while LookupQualifiedName continues to return B::F.

So running through the tester, I get:

base class A {
  fn F();
}

base class B {
  extend base: A;
  private fn F() -> i32;
}

base class C {
  extend base: B;
  // CHECK:STDERR: lookup_private_access.carbon:[[@LINE+3]]:37: ERROR: Cannot access inherited private field `F` of type `C` inherited from `B`.
  // CHECK:STDERR:   fn G[self: Self]() -> () { return self.F(); }
  // CHECK:STDERR:                                     ^~~~~~
  fn G[self: Self]() -> () { return self.F(); }
}

I assume it has something to do with GetEntryInLookupScope which is mostly from LookupQualifiedName

[jonmeow]

To be sure, what I'm trying to say is that I think this kind of access control check probably belongs in LookupQualifiedName, not separate from it. The current GetEntryInLookupScope approach is problematic because it creates a separate implementation of name lookup, which carries two problems:

• It risks divergence from the LookupQualifiedName implementation.
  • (this is what the example code is showing)
• It will repeat substantial work, even on valid code.
  • (In theory, we're more okay with doing extra work for invalid code, e.g. to improve diagnostics -- but valid code should be efficient)

My suggestion, to offer a little detail would be:

• Set aside the current work for a moment. It's useful for understanding the things which will come up, but may not make sense to use directly.
• Add an argument to LookupQualifiedName to indicate whether it should be allowing protected/private access. When allowed:
  • On the first scope, LookupQualifiedName would allow access to private members.
  • On extended scopes, LookupQualifiedName would only allow access to protected members.
  • (when not allowed, only public is allowed)
• LookupQualifiedName could track if it had seen inaccessible names, so that instead of DiagnoseNotFound it could instead diagnose the access restriction.

There's a bit more to flesh out there, but to be sure, member_access.cpp may only need a change to indicate to LookupQualifiedName which kind of access enforcement it requires. Rather than doing separate enforcement, it would be integrated into the existing lookup code.

Let me know if there's a little more I can clarify here, sorry it took some time to sort out my thoughts here.

[brymer-meneses]

I think I have implemented all your suggestions. What do you think about printing a diagnostic for self.base.x? Should this PR handle that too?

[jonmeow]

Thanks for the changes! I have a lot of comments, but I want to be clear, I do think this is a big improvement. I just tend to add a lot of comments for style things (a chunk of these, for example, are just small style things in the test file that happen to stick out to me).

I want to highlight two that are more structural:

• Moving the diagnostic call to where DiagnoseNameNotFound is found.
  • comment here
  • I think this will shift how you track skipped calls, and also may lead to more changes to DiagnoseInvalidAccess.
• Avoiding querying whether the current function is an instance function before determining what to do with it.
  • comment here

I'd suggest starting with these two comments, because they may still shift your approach a little.

[jonmeow]

NB, just a few comments since it looks like you're still addressing old comments, but I wanted to point out a couple small things.

[jonmeow]

Thanks! Just a few last things.

Since this is almost ready to merge, can you also update against trunk? Looking at GitHub, there are a few files with merge conflicts.

[jonmeow]

This looks basically ready to merge, but I see one string_view use got missed. Can you please fix the argument to ClassInvalidMemberAccess?

[jonmeow]

Thanks! This is great. I'll kick off a merge. :)