-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Generic impls access (details 4) #931
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
f018da7
Filling out template with PR 931
josh11b a363ba5
Proposal text
josh11b ec30018
Details changes
josh11b 50c911c
Merge remote-tracking branch 'upstream/trunk' into access
josh11b bde8483
Add an alternative considered
josh11b 9022f50
Merge remote-tracking branch 'upstream/trunk' into access
josh11b 0c057e3
Address suggestions from code review
josh11b bcc8221
Include suggestion for proposals/p0931.md
josh11b 8fcebb4
Clarify "private to a library"
josh11b 01ae9a3
Merge remote-tracking branch 'upstream/trunk' into access
josh11b 04e8cee
Merge branch 'trunk' into access
josh11b File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,143 @@ | ||
# Generic impls access (details 4) | ||
|
||
<!-- | ||
Part of the Carbon Language project, under the Apache License v2.0 with LLVM | ||
Exceptions. See /LICENSE for license information. | ||
SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
--> | ||
|
||
[Pull request](https://github.com/carbon-language/carbon-lang/pull/931) | ||
|
||
<!-- toc --> | ||
|
||
## Table of contents | ||
|
||
- [Problem](#problem) | ||
- [Background](#background) | ||
- [Proposal](#proposal) | ||
- [Rationale based on Carbon's goals](#rationale-based-on-carbons-goals) | ||
- [Alternatives considered](#alternatives-considered) | ||
- [Private impls for public types](#private-impls-for-public-types) | ||
- [Private interfaces in public API files](#private-interfaces-in-public-api-files) | ||
|
||
<!-- tocstop --> | ||
|
||
## Problem | ||
|
||
Should a type be able to declare an `impl` to be private, like it can for its | ||
data and methods? There are some use cases for this feature, but those use cases | ||
generally could also be addressed by implementing the interface on a private | ||
adapter type instead. | ||
|
||
## Background | ||
|
||
Private impls have been considered but not implemented for Swift in the | ||
[scoped conformances pitch](https://forums.swift.org/t/scoped-conformances/37159). | ||
|
||
## Proposal | ||
|
||
This is a proposal to add to | ||
[this design document on generics details](/docs/design/generics/details.md). | ||
The additions are: | ||
|
||
- to say impls do not allow access control modifiers and are always as public | ||
as the names used in the signature of the impl; and | ||
- to document how to use private adapter types instead. | ||
|
||
## Rationale based on Carbon's goals | ||
|
||
We decided to make generics coherent as part of accepting proposal | ||
[#24: generics goals](https://github.com/carbon-language/carbon-lang/pull/24). | ||
This approach is consistent with broader Carbon goals that Carbon have | ||
[code that is easy to read, understand, and write](/docs/project/goals.md#code-that-is-easy-to-read-understand-and-write). | ||
In particular, we favor making code | ||
[less context sensitive](/docs/project/principles/low_context_sensitivity.md). | ||
|
||
## Alternatives considered | ||
|
||
### Private impls for public types | ||
|
||
We considered supporting private impls for public types. This was motivated by | ||
using the conversion of a `struct` type to a `class` type to construct class | ||
values. In the case that the class had private data members, we wanted to | ||
restrict that conversion so it was only available in the bodies of class members | ||
or friends of the class. We considered achieving that goal by saying that the | ||
implementation of the conversion would be private in that case. Allowing impls | ||
to generally be private, though, led to a number of coherence concerns that the | ||
same code would behave differently in different files. Our solution was to | ||
address these conversions using a different approach that only addressed the | ||
conversion use case: | ||
|
||
- Users could not implement `struct` to `class` conversions themselves. | ||
- The compiler would generate `struct` to `class` conversions for constructing | ||
class values itself. | ||
- Those conversion impls will always be as visible as the class type, and will | ||
still be selected using the same rules as other impls. | ||
- When one of these compiler-generated conversion impls is selected, the | ||
compiler would perform an access control check to see if it was allowed. | ||
|
||
We believe that other use cases for restricting access to an impl are better | ||
accomplished by using a private adapter type than supporting private impls. | ||
|
||
### Private interfaces in public API files | ||
|
||
A private interface may only be implemented by a single library, which gives the | ||
library full control. We considered and rejected the idea that developers could | ||
put that interface declaration in an API file to allow it to be referenced in | ||
named constraints available to users. All impls for that interface would also | ||
have to be declared in the API file, unless they were for a private type | ||
declared in that library. | ||
|
||
This would allow you to express things like: | ||
|
||
- A named constraint that is satisfied for any type **not** implementing | ||
interface `Foo`: | ||
|
||
``` | ||
class TrueType {} | ||
class FalseType {} | ||
private interface NotFooImpl { | ||
let IsFoo:! Type; | ||
} | ||
impl [T:! Foo] T as NotFooImpl { | ||
let IsFoo:! Type = TrueType; | ||
} | ||
impl [T:! Type] T as NotFooImpl { | ||
let IsFoo:! Type = FalseType; | ||
} | ||
constraint NotFoo { | ||
extends NotFooImpl where .IsFoo == FalseType; | ||
} | ||
``` | ||
|
||
- A named constraint that ensures `CommonType` is implemented symmetrically: | ||
|
||
``` | ||
// For users to implement for types | ||
interface CommonTypeWith(U:! Type) { | ||
let Result:! Type where ...; | ||
} | ||
|
||
// internal library detail | ||
private interface AutoCommonTypeWith(U:! Type) { | ||
let Result:! Type where ...; | ||
} | ||
|
||
// To use as the requirement for parameters | ||
constraint CommonType(U:! AutoCommonTypeWith(Self)) { | ||
extends AutoCommonTypeWith(U) where .Result == U.Result; | ||
} | ||
|
||
match_first { | ||
impl [T:! Type, U:! ManualCommonTypeWith(T)] | ||
T as AutoCommonTypeWith(U) { | ||
let Result:! auto = U.Result; | ||
} | ||
impl [T:! Type, U:! ManualCommonTypeWith(T)] | ||
U as AutoCommonTypeWith(T) { | ||
let Result:! auto = U.Result; | ||
} | ||
} | ||
``` | ||
|
||
This feature is something we might consider adding in the future. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the rejection of this reflected in existing design documents somewhere? This seems like something that would be allowed by default unless we expressly prohibit it, and at least this proposal's design updates don't seem to do so. I'm OK with this rejected alternative being the only record of this for now, especially given that this is listed as something we might want to revisit, but please file an issue to track this so we don't forget entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created #971 with this question.