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.

[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?