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

Feature interface impl without cast #2628

Closed

Conversation

pmqtt
Copy link
Contributor

@pmqtt pmqtt commented Feb 23, 2023

Hello folks,
with this PR it is possible to write this Carbon code:

package sample api;

interface Printable {
  fn PrintIt[self: Self]();
}

impl String as Printable {
  fn PrintIt[self: String]() {
    Print(self);
  }
}

class Vector(T:! type) {
  var x: T;
}

// Conditionally implement the API for certain `T`s.
impl forall [U:! Printable] Vector(U) as Printable {
  fn PrintIt[self: Self]() {
    Print("{ ");
    self.x.PrintIt();
    Print(" }");
  }
}

fn Main() -> i32 {
  var v: Vector(String) = {.x = "test"};
  v.(Printable.PrintIt)();
  v.PrintIt();
  return 42;
}

I hope it is usefull

Releated issues #2583 and #2580

@github-actions github-actions bot added the explorer Action items related to Carbon explorer code label Feb 23, 2023
@pmqtt pmqtt requested a review from chandlerc February 23, 2023 11:58
@jonmeow jonmeow requested review from zygoloid and removed request for chandlerc February 23, 2023 18:40
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, nice to see you contributing again!

This change looks good to me, but I'm going to flag it for zygoloid since he's been working on this area and might have more insight than myself. :)

explorer/interpreter/type_checker.cpp Outdated Show resolved Hide resolved
@pmqtt
Copy link
Contributor Author

pmqtt commented Feb 23, 2023

Hi, nice to see you contributing again!

This change looks good to me, but I'm going to flag it for zygoloid since he's been working on this area and might have more insight than myself. :)

The last months of last year and the first months of the new year are extremely stressful because of work. So I can not really participate in private projects. My family comes first!

But I love this project and the community here!

@zygoloid
Copy link
Contributor

zygoloid commented Mar 1, 2023

We intend to distinguish between two cases:

  1. "Internal implementation"s, where member names of the interface become member names of the class.
  2. "External implementation"s, where member names of the interface do not become member names of the class.

Prior to #995, that looked like this:

class Vector(T:! type) {
  var x: T;
  // Internal implementation: declared in the class
  // without using the `external` keyword.
  // Inside class, can access private members of Vector.
  // Member names of Printable1 are
  // member names of Vector.
  impl as Printable1 { ... }

  // External implementation: declared inside or outside the class
  // using the `external` keyword.
  // Inside class, can access private members of Vector.
  // Member names of Printable2 are *not*
  // member names of Vector.
  external impl as Printable2 { ... }
}

// External implementation: declared inside or outside the class
// using the `external` keyword.
// Outside class, cannot access private members of Vector.
// Member names of Printable3 are *not*
// member names of Vector.
external impl forall [T:! type] Vector(T) as Printable3 { ... }

var v: Vector(i32);
// OK
v.Print1();
// Error
v.Print2();
v.Print3();
// OK
v.(Printable1.Print1)();
v.(Printable2.Print2)();
v.(Printable3.Print3)();

Under #995, the syntax for this has changed a little; the equivalent syntax after #995 is:

class Vector(T:! type) {
  var x: T;
  // Internal impl.
  extend impl as Printable1 { ... }
  // External impl.
  impl as Printable2 { ... }
}

// External impl.
impl forall [T:! type] Vector(T) as Printable3 { ... }

Conditional internal conformance, such as in your

// Conditionally implement the API for certain `T`s.
impl forall [U:! Printable] Vector(U) as Printable {

example, is discussed in #2580. @chandlerc suggested that we might not want to allow it at all. If we do allow it, the leading syntax (option (3) in #2580, adjusted to match #995) is:

class Vector(T:! type) {
  // Name lookup into `Vector(T)` also always looks into `Printable`.
  extend Printable;
  // For some types `U`, `Vector(U)` implements `Printable`.
  impl forall [U:! Printable] Vector(U) as Printable { ... }
}
impl String as Printable { ... }

var vs: Vector(String) = ...
var vp: Vector(i32*) = ...
// OK
vs.Print();
// Equivalent to `vp.(Printable.Print)()`.
// Error: `Vector(i32*)` does not implement `Printable`.
vp.Print();

class Widget { ... }
impl Vector(Widget) as Printable { ... }
var vw: Vector(Widget) = ...
// OK
vw.Print();

I think that for experimentation purposes it seems reasonable to implement option (3) from #2580 in explorer for now.

Perhaps the best way to make progress here would be to change the GetInterfacesOfType function to look at the impl declarations within the class body, ignoring those declared external. That would get us to the state prior to #995, then we can start looking at implementing #995's syntax changes and one of the options from #2580.

[Edit: extends -> extend in post-#995 syntax examples.]

@chandlerc
Copy link
Contributor

Just FYI, I think a minor correction to the above -- extend impl ... not extends impl ...:

class Vector(T:! type) {
  var x: T;
  // Internal impl.
  extend impl as Printable1 { ... }
  // External impl.
  impl as Printable2 { ... }
}

// External impl.
impl forall [T:! type] Vector(T) as Printable3 { ... }

@pmqtt
Copy link
Contributor Author

pmqtt commented Mar 2, 2023

Just FYI, I think a minor correction to the above -- extend impl ... not extends impl ...:

class Vector(T:! type) {
  var x: T;
  // Internal impl.
  extend impl as Printable1 { ... }
  // External impl.
  impl as Printable2 { ... }
}

// External impl.
impl forall [T:! type] Vector(T) as Printable3 { ... }

Hi,
is this what we want to implement ? If yes, I will try...! If not, I start with the remark from Zygoloid.

@zygoloid
Copy link
Contributor

zygoloid commented Mar 9, 2023

Just FYI, I think a minor correction to the above -- extend impl ... not extends impl ...:

Hi, is this what we want to implement ?

I've updated my comment to use extend rather than extends to match current thinking. It makes sense to me to implement this in explorer -- even though it's getting a little ahead of the approved design, we have growing confidence that this is the direction we want to go in.

lnihlen added a commit to lnihlen/carbon-lang that referenced this pull request Apr 21, 2023
Fixes carbon-language#2583. Note that this still results in a compliation error pending the merge
of carbon-language#2628, this just prevents the assertion from happening.
jonmeow pushed a commit that referenced this pull request Apr 24, 2023
Note that this code still results in a compilation error pending the merge of #2628, this just prevents the assertion from happening.

Closes #2583.
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See prior discussion for requested changes.

@github-actions
Copy link

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please comment or remove the inactive label.

This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

@github-actions github-actions bot added the inactive Issues and PRs which have been inactive for at least 90 days. label Sep 13, 2023
@github-actions
Copy link

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active or becomes active again, please reopen it.

This PR was closed and archived because there has been no new activity in the 14 days since the inactive label was added.

@github-actions github-actions bot closed this Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
explorer Action items related to Carbon explorer code inactive Issues and PRs which have been inactive for at least 90 days.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants