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 beyond inheritance #2745

Open
josh11b opened this issue Apr 5, 2023 · 7 comments
Open

Name conflicts beyond inheritance #2745

josh11b opened this issue Apr 5, 2023 · 7 comments
Labels
leads question A question for the leads team

Comments

@josh11b
Copy link
Contributor

josh11b commented Apr 5, 2023

Summary of issue:

#2355 defines the behavior of name conflicts arising from inheritance. There are two other kinds of name conflicts that can arise in classes from using extend (of #995) to extend the API of a class:

  • A class can extend things other than a base class, and some member in the class can conflict with a member with the same name in the thing being extended: extending adapter, mixin, and internal impl.
  • A class can extend more than one thing, and more than one of the things being extended can have a member of the same name.

We can have both of these issues, where there are members with the same name in the class and multiple things being extended. This is in fact a likely way people will want to resolve the ambiguity of having a name conflict in the things being extended.

What are the rules for resolving these name conflicts, and how do they interact with the restrictions from #2355?

@josh11b josh11b added the leads question A question for the leads team label Apr 5, 2023
@chandlerc
Copy link
Contributor

My initial thoughts --

I'd be happy to generalize the rule we have for inheritance in #2355 for other cases of a class extending something and explicitly declaring a name -- the explicit name shouldn't be obviously incompatible but otherwise shadows. I don't have strong feelings on whether we want anything beyond "doesn't change the access level" for "obviously incompatible". While any restrictions here do present some evolution hazards, I feel like the balance we struck in #2355 is similarly reasonable elsewhere (to the extent it applies).

I'm much less clear on what we should do for extending two things and resolving a conflict between them.

Part of my hesitation with rejecting in the face of conflicts is that it doesn't seem like it leaves the user many good options at all.

Would be interested to hear other ideas for how we should or shouldn't handle these.

@josh11b
Copy link
Contributor Author

josh11b commented Apr 11, 2023

To be concrete, here is an example:

// Both interface `I` and base class `B` define a method named `F`:
interface I { fn F[self: Self](); }
base class B { fn F[self: Self](x: i32) -> i32; }

// Name conflict between two things being `extend`ed
class C1 {
  extend base: B;
  extend impl as I;
}

// Resolve the name conflict of `C1` to use `B.F`
class C2 {
  extend base: B;
  extend impl as I;
  alias F = B.F;
}

// Resolve the name conflict of `C1` to use `I.F`
class C3 {
  extend base: B;
  extend impl as I;
  alias F = I.F;
}

// Name conflict between `I.F` and `C4.F`
class C4 {
  extend impl as I;
  fn F[me: Self]() -> bool;
}


// Name conflict between `B.F` and `C5.F`
class C5 {
  extend adapt B;
  fn F[me: Self]() -> bool;
}

To me, it seems like C1 should act like it doesn't have a member F, but it does have members B.F and I.F:

var c1: C1 = {};
c1.(B.F)(2);
c1.(I.F)();
// ❌  Error: `c1.F` is ambiguous, could mean either `B.F` or `I.F`
c1.F();

This is consistent with our approach to the & operator applied to interfaces, and other cases of member access, see #989 and https://github.com/carbon-language/carbon-lang/blob/trunk/docs/design/expressions/member_access.md#lookup-ambiguity .
(At some point we will want to allow users to define C1.F to be an overload of B.F and I.F, since they take different parameter types.)

Another choice, specifically the one we chose before when implementing two interfaces internally with a name in common, was to report an error C1. This is a conservative choice, with the idea that Carbon will reject programs at compile time rather than doing something surprising. However it doesn't give many options for the user to work around problem, and seems like it may cause more programs than necessary to fail to compile in the face of evolution.

The cases of C2 and C3 I think are particularly well motivated: specifically using alias to disambiguate. We've talked about what criteria something in the class can shadow something it extends, and this use case motivates allowing an alias to shadow.

Case C4 seems the most dubious, since it seems like a conflict that could have been avoided by externally implementing I. On the other hand, it seems like something that could arise due to evolution. And it is similar to the case of C5 using an adapter, where we don't have as good other options for controlling the resulting API of the class.

It does seem like we'd like the policy to be consistent across extending an interface, a base class, and an adapter. This will make it easier to teach and remember, and make it more likely that the policy also applies when we want to answer this question for mixins in the future as well.

@josh11b
Copy link
Contributor Author

josh11b commented Sep 20, 2023

My suggested resolution is:

  • Names defined in the class definition shadow names in an extension (something extended) when used without qualification.
  • Otherwise a name used without qualification and found in multiple extensions is an ambiguity error.
  • A definition in a class that isn't just an alias to one of the qualifications of that name may be disallowed, based on criteria like in Name conflicts between base and derived classes #2355.

Note that there is some extra complexity to this last point, since we may have two different things in extensions that are being "refined" by the new definition in the class, as in:

class C6 {
  extend base: B;
  extend impl as I;
  // Does this have to refine both `B.F` and `I.F`?
  fn F[me: Self]() -> bool;
}

@josh11b
Copy link
Contributor Author

josh11b commented Sep 22, 2023

Another question is how extend in interfaces handles name conflicts. If the extending interface shadows the names, this could allow extending an incomplete interface. Not sure if that is an important use case.

@officialhemant511
Copy link

officialhemant511 commented Oct 24, 2023

@josh11b

To be concrete, here is an example:

// Both interface `I` and base class `B` define a method named `F`:
interface I { fn F[self: Self](); }
base class B { fn F[self: Self](x: i32) -> i32; }

// Name conflict between two things being `extend`ed
class C1 {
  extend base: B;
  extend impl as I;
}

// Resolve the name conflict of `C1` to use `B.F`
class C2 {
  extend base: B;
  extend impl as I;
  alias F = B.F;
}

// Resolve the name conflict of `C1` to use `I.F`
class C3 {
  extend base: B;
  extend impl as I;
  alias F = I.F;
}

// Name conflict between `I.F` and `C4.F`
class C4 {
  extend impl as I;
  fn F[me: Self]() -> bool;
}


// Name conflict between `B.F` and `C5.F`
class C5 {
  extend adapt B;
  fn F[me: Self]() -> bool;
}

To me, it seems like C1 should act like it doesn't have a member F, but it does have members B.F and I.F:

var c1: C1 = {};
c1.(B.F)(2);
c1.(I.F)();
// ❌  Error: `c1.F` is ambiguous, could mean either `B.F` or `I.F`
c1.F();

This is consistent with our approach to the & operator applied to interfaces, and other cases of member access, see #989 and https://github.com/carbon-language/carbon-lang/blob/trunk/docs/design/expressions/member_access.md#lookup-ambiguity . (At some point we will want to allow users to define C1.F to be an overload of B.F and I.F, since they take different parameter types.)

Another choice, specifically the one we chose before when implementing two interfaces internally with a name in common, was to report an error C1. This is a conservative choice, with the idea that Carbon will reject programs at compile time rather than doing something surprising. However it doesn't give many options for the user to work around problem, and seems like it may cause more programs than necessary to fail to compile in the face of evolution.

The cases of C2 and C3 I think are particularly well motivated: specifically using alias to disambiguate. We've talked about what criteria something in the class can shadow something it extends, and this use case motivates allowing an alias to shadow.

Case C4 seems the most dubious, since it seems like a conflict that could have been avoided by externally implementing I. On the other hand, it seems like something that could arise due to evolution. And it is similar to the case of C5 using an adapter, where we don't have as good other options for controlling the resulting API of the class.

It does seem like we'd like the policy to be consistent across extending an interface, a base class, and an adapter. This will make it easier to teach and remember, and make it more likely that the policy also applies when we want to answer this question for mixins in the future as well.

can we modify this example like this.... to resolve this name conflict error..

// Both interface `I` and base class `B` define a method named `F`:
interface I {
    fn F[self: Self]();
}

base class B {
    fn F[self: Self](x: i32) -> i32 {
        // Implementation of method F in base class B
        x * 2  // some computation
    }
}

// Name conflict between two things being `extend`ed
class C1 {
    extend base: B;
    extend impl as I;
}

// Resolve the name conflict of `C1` to use `B.F`
class C2 {
    extend base: B;
    extend impl as I;
    alias F = B.F;
}

// Resolve the name conflict of `C1` to use `I.F`
class C3 {
    extend base: B;
    extend impl as I;
    alias F = I.F;
}

// No name conflict, method F only exists in C4
class C4 {
    extend impl as I;
    fn F[me: Self]() -> bool {
        // Implementation of method F in class C4
        true
    }
}

// No name conflict, method F only exists in C5
class C5 {
    extend adapt B;
    fn F[me: Self]() -> bool {
        // Implementation of method F in class C5
        false
    }
}

ig there are many ways but we can do this also i hope this make it will help 😊....

@geoffromer
Copy link
Contributor

@officialhemant511 FYI I've edited your comment to fix the formatting.

It looks like the only differences in your version are:

  • B.F, C4.F, and C5.F have definitions, not just declarations.
  • In C4 and C5, the name conflict is resolved in favor of the declarations of F in those classes.
    Am I overlooking anything?

Are you suggesting that the name resolution behavior should change depending on whether the function body of F is inside the class/interface body (rather than being declared out-of-line)?

@josh11b
Copy link
Contributor Author

josh11b commented Oct 25, 2023

@officialhemant511 I'm not sure if my post was clear: the goal is to show various examples where name conflicts can occur and decide what happens in each of those situations.

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