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

Name conflicts between base and derived classes #2355

Closed
josh11b opened this issue Oct 26, 2022 · 14 comments
Closed

Name conflicts between base and derived classes #2355

josh11b opened this issue Oct 26, 2022 · 14 comments
Labels
leads question A question for the leads team

Comments

@josh11b
Copy link
Contributor

josh11b commented Oct 26, 2022

Summary of issue:

Do we want:

  • derived classes to shadow base classes: making things context sensitive but matching C++
  • no name conflicts allowed: but then concerns about how you evolve base classes
  • private names to be ignored when looking up names: but then with var derived: Derived, derived.name could mean the private member inside Derived, but the base member outside.

Details:

We allow a base and derived class to have class functions with the same name, and this will be used for things like constructors (Make). These will be accessed through the class name, and so won't generally create ambiguity.

Currently the text of https://github.com/carbon-language/carbon-lang/blob/trunk/docs/design/classes.md#virtual-override-keywords says that non-virtual method names can't match/conflict. The behavior for variables is unspecified, but presumably should match.

https://github.com/carbon-language/carbon-lang/blob/trunk/docs/design/classes.md#private-access says that private virtual and private abstract methods may be implemented/overridden in derived classes. Otherwise, no statement is made about whether a derived class may have variables/methods with the same name as a private member of a base class.

In the future work part of the classes design, https://github.com/carbon-language/carbon-lang/blob/trunk/docs/design/classes.md#overloaded-methods also talks about this issue.

@josh11b josh11b added the leads question A question for the leads team label Oct 26, 2022
@josh11b
Copy link
Contributor Author

josh11b commented Oct 27, 2022

For consistency, it would be good to make extends in an interface behave the same way. Right now, extends in an interface is defined to add aliases for all the members, and produces an error if there are any name conflicts. We could instead say it acts more like & operator, where name conflicts are only a problem on use, and can be fixed by qualification. This has a wrinkle though, what qualification do we use to say use the member in the interface doing the extending?

interface C {
  let X:! Type;
}
interface I {
  let X:! Type;
  extends C;
}
  • Can write C.X to get X from C
  • Not clear whether I.X is considered ambiguous. If it is ambiguous, then there is no way to resolve that ambiguity, without redefining I to put X into another interface that it extends. This at least is an option for interfaces, but may mean that a change to an interface that is extended causes a ripple of changes to those that extend.

@josh11b
Copy link
Contributor Author

josh11b commented Oct 28, 2022

This was discussed a little yesterday, 10-27.

For interface name conflicts, if we say I.X is not ambiguous, and refers to the X defined in I, there are two choices for T.X after binding T:! I:

  • T.X is invalid and has to be written T.(I.X) (or T.(C.X))
  • "shadowing": T.X is allowed and means T.(I.X)

I lean toward the shadowing approach. I think it would cause fewer breakages due to evolution, since C can add X without breaking existing uses of I, and is a bit more consistent about resolving T.foo to be I.foo, to match T:! I.

@chandlerc
Copy link
Contributor

FWIW, I think both @zygoloid and I were generally happy to explore the shadowing approach here. Some of my thoughts from that discussion capturing here, correct me if I've mis-remembered or am missing anything though:

Generally, C++ types do use shadowing even beyond evolutionarily -- for example types that can provide a more refined set of overloads (maybe due to layering exposing more efficient conversion sequences), or types that have a more efficient form of dispatch than (open) virtual dispatch can still short circuit the dispatch when a derived type is statically known. Some of these use cases we can maybe solve more directly with Carbon, but it does seem nice to retain the core expressive functionality here.

While I'd somewhat like to have a nice rule that shadows are "compatible" in some way (IE, calls to the base would always be valid calls to the derived), that seems like it might be difficult or impossible to implement so personally I'd not push hard to enforce this in the initial version.

I'd still like to make private members of a base completely invisible to derived as I think that's an especially important aspect for evolution -- no conflicts, shadowing, anything in as many cases as we can... We can't do the reverse, but I think that's a much less important direction -- I expect derived classes to be heavily constrained by their base classes.

@chandlerc
Copy link
Contributor

Let's call my last post a decision then!

@josh11b
Copy link
Contributor Author

josh11b commented Dec 1, 2022

I think this is a reasonable decision, but there was some significant debate about which path to take in open discussion on Nov 10. Specifically there was debate about how to handle names that are public in the base and have a distinct private definition in the derived class. I'd very much like to better understand and document the lead's rationale for this path rather than the alternatives considered. @chandlerc

@chandlerc
Copy link
Contributor

After discussing, I think that the Nov 1 comment gives viable direction for private-in-base and for same access in both base and derived. But I think @josh11b is surfacing that it doesn't actually clarify how exactly we should handle cases with different access levels in base and derived outside of private-in-base.

My inclination is to start off restrictive here. In the direction of "we'd like shadowing names to be generally 'compatible' with the name being shadowed", where there are clear incompatibilities like changing the access from non-private to private, I would suggest rejecting that as an incompatible shadow.

I would personally be fine allowing other access level changes via shadowing, but I don't feel very strongly about them either way if restricting it would actually be easier to implement or specify in some way.

That said, we should make sure we actually have agreement for all of this. =D

@chandlerc chandlerc reopened this Dec 5, 2022
@josh11b
Copy link
Contributor Author

josh11b commented Dec 7, 2022

Just to be complete:

base derived result
private anything shadow
protected private ??
protected protected shadow
protected public shadow
public private ??
public protected ??
public public shadow

My rationale that a protected name in the base should be shadowed by the same name in the derived:

  • re-declaring it protected should definitely be allowed. For one, it passes the "compatible" test. It is also useful for the same reason as re-declaring something public in the base as public in the derived, it allows you to define a specialized implementation that is more efficient but with the same semantics.
  • declaring something protected in base as public in the derived seems useful too: it allows the base to provide functionality that derived types may optionally want to expose
  • The least clear-cut is declaring something protected in base as private in the derived. My thinking is it would be for the case where the base provides some functionality that derived types may optionally want to expose, but is inappropriate for a particular derived class. Unclear if this could be bypassed in a further-derived class using qualified member access to name the base member directly.

If the name is public in the base, we have some choices:

  • Error if the same name is declared private or protected in derived, as described in Name conflicts between base and derived classes #2355 (comment) .
  • Shadow, to have the simple rule of "derived always shadows base".
  • Error by default, but allow a keyword modifier prefix to opt-in to shadowing -- so there is an escape hatch if you want to shadowing intentionally.

@jonmeow
Copy link
Contributor

jonmeow commented Dec 23, 2022

A related question I'll ask to be resolved here: the design suggests children should be allowed to shadow class functions so that both the child and parent can have a Create function. Can a child shadow a parent's class function with a method, or vice versa?

Separately, the previous comment says redeclaring a parent protected method as public seems useful. If a parent has a protected virtual method, can the child have a public non-virtual method of the same name?

(I think this could yield somewhat complex rules)

@josh11b
Copy link
Contributor Author

josh11b commented Dec 28, 2022

Proposal (after some discussion with @chandlerc ) about how to handle virtual functions:

  • If the same name is used in both base and derived, either they are both virtual or neither are. If one is virtual and the other is not, it is an error. (And there are no virtual static class functions.)
  • As a result the only thing that can be done with a function declared virtual (or abstract) in the base, is impl it in the derived (or do nothing).
  • When doing impl fn, look up the name in the set of virtual functions of the base, ignoring access specifiers. This way, you can implement a private virtual function in the base (as described in the section about private access in the class design document).
  • impl fn doesn't introduce a new name, and so doesn't have its own access specifier. When doing any other name lookup, use the usual rules to determine if you find the declaration in the base class.

@josh11b
Copy link
Contributor Author

josh11b commented Jan 4, 2023

A related question I'll ask to be resolved here: the design suggests children should be allowed to shadow class functions so that both the child and parent can have a Create function. Can a child shadow a parent's class function with a method, or vice versa?

@chandlerc I think we were saying that shadowing class functions with non-virtual methods and the other way around were both reasonable to us. This is to expose whether the functions need to use the receiver at the calling convention level, while syntactically uniformly using the object.F(); form to be source compatible with either option.

@josh11b
Copy link
Contributor Author

josh11b commented Feb 3, 2023

Rereading this comment, it seems the suggestion is to forbid non-private in base to be declared private in derived, which gives this chart:

base derived result
private anything shadow
protected private forbidden
protected protected shadow
protected public shadow
public private forbidden
public protected ??
public public shadow

I think reasonably simple rule (for the non-virtual case) would be:

In the case both base and derived declare the same name, derived is required to have the same access as base or be more public, and in that case the derived version of the name shadows the base version.

which gives:

base derived result
private anything shadow
protected private forbidden
protected protected shadow
protected public shadow
public private forbidden
public protected forbidden
public public shadow

@josh11b
Copy link
Contributor Author

josh11b commented Feb 9, 2023

Proposal (after some discussion with @chandlerc ) about how to handle virtual functions:

  • If the same name is used in both base and derived, either they are both virtual or neither are. If one is virtual and the other is not, it is an error. (And there are no virtual static class functions.)
  • As a result the only thing that can be done with a function declared virtual (or abstract) in the base, is impl it in the derived (or do nothing).
  • When doing impl fn, look up the name in the set of virtual functions of the base, ignoring access specifiers. This way, you can implement a private virtual function in the base (as described in the section about private access in the class design document).
  • impl fn doesn't introduce a new name, and so doesn't have its own access specifier. When doing any other name lookup, use the usual rules to determine if you find the declaration in the base class.

Refining these rules:

  • A private non-virtual base name will always be ignored in the derived
  • A non-private non-virtual base name may not be shadowed by a virtual derived name
  • A virtual base name prevents anything with the same name in derived
  • The only thing that can be done with a function declared virtual (or abstract) in the base, is impl it in the derived (or do nothing)
  • When doing impl fn, look up the name in the set of virtual functions of the base, ignoring access specifiers. This way, you can implement a private virtual function in the base (as described in the section about private access in the class design document).
  • impl fn doesn't introduce a new name, and so doesn't have its own access specifier. When doing any other name lookup, use the usual rules to determine if you find the declaration in the base class.

@zygoloid
Copy link
Contributor

zygoloid commented Feb 9, 2023

Some rationale for the above rules:

  • We broadly are aiming for fairly tight coupling between the public interface of the base class and the derived class. Adding to the interface of the base class must be done cautiously to not create a surprising interface where the same name on a Base* and a Derived* are drastically different. Similarly, adding to the interface of the derived class should be done with the knowledge of which base class names exist and should be avoided. Allowances for shadowing / name hiding are only made to address some specific use cases.
  • The set of virtual names in the vtable of a type is always visible to derived classes regardless of access control, so every virtual function is always part of an interface exposed by the type, even if it's private.

So, one way to think about the rules is: we disallow "arbitrary" name hiding between base and derived, but allow two exceptions that end up being quite broad.

  • Exception 1: a base class adding a name that is not part of any interface that it exposes (neither public, protected, nor virtual) should not risk breaking derived classes. But it still shouldn't hide any exposed name from a base class. So private, non-virtual names can "be shadowed" (they're unobservable in derived classes) but cannot shadow anything (except other private, non-virtual names).
  • Exception 2: non-private names in the base class can be refined by non-private names in the derived class. The intent is that the meaning of the name is either the same (eg, the derived class can provide a more efficient implementation of a method) or broader (eg, the derived class can accept more arguments). But for virtual functions, we expect refinement to instead be done by impl fn, so we don't allow those to be refined by name hiding. Because the intent is to support only refinement, the derived class version must be at least as accessible as the base class version.

[Given the above, I could imagine tightening up exception 2, for example to only apply in the case where the base and derived members are both member functions. But maybe there are cases where you want something else, such as refining a class function with a function-like object or vice versa.]

@chandlerc
Copy link
Contributor

Marking this as decided with the results and rationale in the last two comments here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
leads question A question for the leads team
Projects
None yet
Development

No branches or pull requests

4 participants