-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Identification of a named constraint during definition #6902
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
Open
danakj
wants to merge
7
commits into
carbon-language:trunk
Choose a base branch
from
danakj:proposal-identify-in-defn
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+182
−0
Open
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
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
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or 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,136 @@ | ||
| # Identification of a named constraint during definition | ||
|
|
||
| <!-- | ||
| 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/6902) | ||
|
|
||
| <!-- toc --> | ||
|
|
||
| ## Table of contents | ||
|
|
||
| - [Abstract](#abstract) | ||
| - [Problem](#problem) | ||
| - [Background](#background) | ||
| - [Proposal](#proposal) | ||
| - [Details](#details) | ||
| - [Rationale](#rationale) | ||
| - [Alternatives considered](#alternatives-considered) | ||
|
|
||
| <!-- tocstop --> | ||
|
|
||
| ## Abstract | ||
|
|
||
| This proposal updates the criteria for when a facet type is considered | ||
| "identified." Specifically, it relaxes the requirement for named constraints, | ||
| allowing them to be considered identified if they have begun being defined, | ||
| rather than requiring them to be fully complete. This change enables `Self` | ||
| lookups within a constraint's definition to correctly resolve witnesses based on | ||
| prior `require impls` statements in the same scope. | ||
|
|
||
| ## Problem | ||
|
|
||
| Under the rules established in [Proposal #5168](/proposals/p5168.md), a facet | ||
| type can be identified only if all its referenced interfaces were declared and | ||
| all its named constraints were complete. | ||
|
|
||
| This definition creates a circularity problem during the definition of a named | ||
| constraint. If a `require impls` statement inside a named constraint definition | ||
| relies on an impl lookup that involves `Self`, that lookup will fail because the | ||
| the facet type of `Self` is not identified before the named constraint is | ||
| complete. This prevents requirements in a named constraints from depending on | ||
| earlier ones. | ||
|
|
||
| ## Background | ||
|
|
||
| - [Proposal #5168](/proposals/p5168.md): Introduced rules for facet type | ||
| identification and completion. | ||
|
|
||
| ## Proposal | ||
|
|
||
| We propose redefining the identification criteria for facet types to say that a | ||
| facet type is identified if all the interfaces it references are declared and | ||
| all of its named constraints are complete **or have begun being defined**. | ||
|
|
||
| But we also propose to disallow using a named constraint by name inside its own | ||
| definition. This way we can use `Self` to find a witness, but we can not use the | ||
| named constraint as a constraint for a `require impls` statement. | ||
|
|
||
| ## Details | ||
|
|
||
| This change allows the compiler to treat a named constraint as identified for | ||
| the purposes of impl lookup while it is still being typechecked. | ||
|
|
||
| As the compiler processes a series of `require impls` statements within a named | ||
| constraint, the identified facet type of `Self` is built up incrementally. The | ||
| identified facet type of `Self` will contain interfaces provided by | ||
| `require impls` statements written prior to that use of `Self`. Thus later | ||
| `require impls` statements can use the partially-built identified facet type of | ||
| `Self` to find witnesses provided by earlier `require impls` statements during | ||
| impl lookup. | ||
|
|
||
| The following example demonstrates a scenario that is currently invalid but | ||
| would be enabled by this proposal: | ||
|
|
||
| ```carbon | ||
| interface Y {} | ||
|
|
||
| interface NeedsY(T:! Y) {} | ||
|
|
||
| constraint W { | ||
| require impls Y; | ||
|
|
||
| // This requires an impl lookup where the query self value is `Self` | ||
| // (which is of type `W`) and the query interface is `Y`. The lookup | ||
| // requires identifying the facet type of `Self` to find a witness. | ||
| // After this proposal, W can be identified because it has begun being | ||
| // defined. The lookup for `Self as Y` can now succeed due to the previous | ||
| // `require impls` statement. | ||
| require impls NeedsY(Self); | ||
| } | ||
| ``` | ||
|
|
||
| In this example, identifying `W` while it is being defined allows the lookup for | ||
| `Self as Y` in order to form a facet value for `NeedsY` to succeed because the | ||
| compiler knows `Self` (of type `W`) implements `Y` from the previous | ||
| `require impls` statement. | ||
|
|
||
| However, without disallowing the use of the named constraint by name, the | ||
| following becomes possible: | ||
|
|
||
| ```carbon | ||
| constraint W { | ||
| require C impls W; | ||
| require impls Z; | ||
| } | ||
| ``` | ||
|
|
||
| This says that `C` must implement `W`, yet `W` is not fully defined. At that | ||
| line `W` is still empty, so it places no requirements on `C`. The next line | ||
| requires that anything implementing `W` must implement `Z`. | ||
|
|
||
| To prevent this, we disallow the use of _the name_ `W` inside `W`, while still | ||
| allowing the use of `Self`. | ||
|
|
||
| Together these changes allow a named constraint to _provide_ witnesses for facet | ||
| values, without allowing the named constraint to become a _requirement_ in a | ||
| facet type. To understand the requirements of a named constraint, it must be | ||
| complete. | ||
|
|
||
| ## Rationale | ||
|
|
||
| This change aligns with Carbon's goal of | ||
| [Code that is easy to read, understand, and write](/docs/project/goals.md#code-that-is-easy-to-read-understand-and-write), | ||
| by allowing expressive generics as they have been designed. | ||
|
|
||
| This follows the | ||
| [Information accumulation](/docs/project/principles/information_accumulation.md) | ||
| principle by increasing the information available to the program with each | ||
| statement. | ||
|
|
||
| ## Alternatives considered | ||
|
|
||
| None. | ||
Oops, something went wrong.
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.
This restriction seems possible to circumvent. Given a
TypeOffunction:it seems we could circumvent the restriction by using of
TypeOf(Self)instead of the name of the constraint, or similarly by using an alias or other indirection:Is that OK? (Are we trying to create a speed bump or a hard barrier here?)
If that is a problem, I wonder if we could allow only
Self as Nto be identified (whereSelfis the self ofN) while the named constraintNis being defined, and not any other identification ofN. I think that may be the same as what you're proposing except in these corner cases.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.
Yeah that sounds like a better way to say it, thanks.
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.
We do want a hard barrier, because other uses of the constraint lead to what could be seen as incorrect output, where a constraint on
Adoesn't actually include all the requirements thatAdoes.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 am struggling with how to make this a rule around the facet type of
Selfbeing identified. If it's the facet type, then TypeOf(Self) seems like it must return a facet type that is identified, but we don't want that.As such I think the rule has to be instead that impl lookup does not require an identified facet type for
Selfin a named constraint? But that's much further from what the toolchain is doing - which is that it is identifyingSelfand accepting that inside the definition.Maybe TypeOf(Self) should be okay, since it's related to Self it's more clear that it changes over the definition, unlike using the name of the constraint.
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 guess it may be because the previous rule around identifying a facet type is written in terms of the type only. Whereas what you're thinking about here is the self+type pair.
Nonetheless, I think I am happy with a rule that says the type of
Selfcan be identified (generally), since you've gone throughSelfyou are acknowledging the incremental/incomplete nature.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.
The alias case already worked, but added a test in #6922