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

Generics details 6: remove facets #950

Merged
merged 40 commits into from
Jan 6, 2022
Merged

Generics details 6: remove facets #950

merged 40 commits into from
Jan 6, 2022

Conversation

josh11b
Copy link
Contributor

@josh11b josh11b commented Nov 12, 2021

There were some concerns about facet types leaking out of generic code in return types. Some initial fixes for this were done in PR #900, but there remain concerns, for example when associated types are involved.

In particular, given an interface method with return type using an associated type, as in:

interface Deref {
  let Result:! Type;
  fn DoDeref[me: Self]() -> Result;
}

class IntHandle {
  impl as Deref {
    let Result:! Type = i32;
    fn DoDeref[me: Self]() -> Result { ... }
  }
}

Since Result has type Type, we had the problem that IntHandle.DoDeref would have to return i32 as Type, instead of the desired i32.

We also think we can simplify the model by eliminating the facet type concept and syntax.

This proposal removes facet types, introduces archetypes in their place, clarifies how associated types work outside of a generic function, and specifies how a generic let statement in a function body works.

@josh11b josh11b added the proposal A proposal label Nov 12, 2021
@josh11b josh11b added this to Draft in Proposals via automation Nov 12, 2021
@josh11b josh11b requested a review from a team November 12, 2021 22:02
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Nov 12, 2021
Copy link
Collaborator

@wolffg wolffg left a comment

Choose a reason for hiding this comment

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

Some minor fixes.

### Path from regular functions

Replacing a regular, non-parameterized function with a generic function should
be straightforward without affecting existing callers of the function. There may
Copy link
Collaborator

Choose a reason for hiding this comment

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

"straightforward" is a little evalulative. I think you could just say "should not affect..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it, but I would think "straightforward" was more okay as a goal than as a description of a design.

@@ -80,7 +80,7 @@ Summary of how Carbon generics work:
- A function with a generic type parameter can have the same function body as
an unparameterized one. Functions can freely mix generic, template, and
regular parameters.
- Interfaces can require other interfaces be implemented, or
- Interfaces can require other interfaces be implemented or
Copy link
Collaborator

Choose a reason for hiding this comment

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

I realize someone just clipped out a comma here, but this whole sentence makes me wonder: Are these two bullet points? Is this either/or? That is, interfaces can exist on their own. They can extend other interfaces. Also, separately, interfaces can require that interfaces inside them be implemented.

Put another way, why are these two things in the same bullet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extending is requiring plus some extra stuff. I've made it two bullets, but it isn't obviously better to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The way it was written before made it seem like you could one or the other; the new way makes it clear both are allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way you write the code, you either require an interface or extend an interface, but it happens that extending an interface implies that you require it.

docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/terminology.md Outdated Show resolved Hide resolved
docs/design/generics/terminology.md Outdated Show resolved Hide resolved
docs/design/generics/terminology.md Outdated Show resolved Hide resolved
docs/design/generics/terminology.md Outdated Show resolved Hide resolved
docs/design/generics/terminology.md Outdated Show resolved Hide resolved
@josh11b josh11b marked this pull request as ready for review December 6, 2021 21:50
@github-actions github-actions bot added the proposal rfc Proposal with request-for-comment sent out label Dec 6, 2021
docs/design/generics/details.md Outdated Show resolved Hide resolved
docs/design/generics/details.md Outdated Show resolved Hide resolved
docs/design/generics/details.md Outdated Show resolved Hide resolved

```
let Point2DInterface:! auto = NSpacePoint where .N = 2;
let template Point2DInterface:! auto = NSpacePoint where .N = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the template mean here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This proposal defines generic let, but it wasn't clear to me that generic let would allow you to give a name to a constraint without erasing something important. Template let clearly should work here, though, and I didn't see a downside to using a template in this context, other than not having written the template doc yet.

How should I proceed from here? For now my intent was simply to say "you can write let template to give a name to a constraint expression as can be seen in these examples"

Copy link
Contributor

Choose a reason for hiding this comment

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

As it stands, I think this proposal is introducing new let template syntax that didn't exist in the design before. If we want to add that let template syntax in addition to let syntax, I'd like to be clear on what the difference between the two is, semantically, and why we want to have both. My inference from the appearance of template here is that lets won't be fully referentially transparent, in particular for certain kinds of compile-time evaluation -- that there will be cases in which factoring out a constant into a named (non-template) let will change the meaning of the program in some ways -- but I'm not sure in which ways, or why.

There may be good reasons we want to distinguish between a non-referentially-transparent let and a referentially-transparent let template, but -- absent a rationale for why this is a consequence of removing facet types -- I think that should be a separate discussion. I'd suggest we revert all the let -> let template changes in this proposal and consider the let template change separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of the conclusions of the talk with @chandlerc was that let here with auto probably should be referentially-transparent, so I've replaced let template with let.

docs/design/generics/details.md Show resolved Hide resolved
docs/design/generics/details.md Show resolved Hide resolved
docs/design/generics/details.md Outdated Show resolved Hide resolved
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.

Thanks, this looks good to me, modulo the let template piece that I think we should consider separately.

d.DrawRectangle(...);
...
ComparableFromDifferenceFn(IntWrapper, Difference)
as Comparable;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the as Comparable here? This seems a bit facet-type-y.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct I missed making this change.

docs/design/generics/details.md Outdated Show resolved Hide resolved

```
let Point2DInterface:! auto = NSpacePoint where .N = 2;
let template Point2DInterface:! auto = NSpacePoint where .N = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

As it stands, I think this proposal is introducing new let template syntax that didn't exist in the design before. If we want to add that let template syntax in addition to let syntax, I'd like to be clear on what the difference between the two is, semantically, and why we want to have both. My inference from the appearance of template here is that lets won't be fully referentially transparent, in particular for certain kinds of compile-time evaluation -- that there will be cases in which factoring out a constant into a named (non-template) let will change the meaning of the program in some ways -- but I'm not sure in which ways, or why.

There may be good reasons we want to distinguish between a non-referentially-transparent let and a referentially-transparent let template, but -- absent a rationale for why this is a consequence of removing facet types -- I think that should be a separate discussion. I'd suggest we revert all the let -> let template changes in this proposal and consider the let template change separately.

@chandlerc
Copy link
Contributor

FWIW, this LGTM as well. The whole generic let vs template let and related sty seems not super essential to this proposal. Mostly don't think it's worth slowing down, we can keep iterating there, either adding stuff if/when well motivated and understood or fixing it, either way.

Had an interesting chat with Josh about generic let's and more generally let's in impls, but again, I still think this is a good incremental step.

@josh11b josh11b merged commit 86dfaff into carbon-language:trunk Jan 6, 2022
Proposals automation moved this from RFC to Accepted Jan 6, 2022
@josh11b josh11b deleted the no-facet branch January 6, 2022 22:08
@github-actions github-actions bot added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Jan 6, 2022
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
There were some concerns about facet types leaking out of generic code in return types. Some initial fixes for this were done in [PR #900](#900), but there remain concerns, for example when associated types are involved.

In particular, given an interface method with return type using an associated type, as in:

```
interface Deref {
  let Result:! Type;
  fn DoDeref[me: Self]() -> Result;
}

class IntHandle {
  impl as Deref {
    let Result:! Type = i32;
    fn DoDeref[me: Self]() -> Result { ... }
  }
}
```

Since `Result` has type `Type`, we had the problem that `IntHandle.DoDeref` would have to return `i32 as Type`, instead of the desired `i32`.

We also think we can simplify the model by eliminating the facet type concept and syntax.

This proposal removes facet types, introduces archetypes in their place, clarifies how associated types work outside of a generic function, and specifies how a generic `let` statement in a function body works.

Co-authored-by: Wolff Dobson <[email protected]>
Co-authored-by: Richard Smith <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes PR meets CLA requirements according to bot. proposal accepted Decision made, proposal accepted proposal A proposal
Projects
No open projects
Proposals
Accepted
Development

Successfully merging this pull request may close these issues.

None yet

4 participants