-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Type completion of facet types is separate from Identifying #6385
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
27
commits into
carbon-language:trunk
Choose a base branch
from
danakj:complete-not-identify
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.
+1,828
−534
Open
Changes from all commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
98a72b9
complete-not-identify
danakj 1a64cdc
touchup
danakj e271440
MakeCopyOfSpecificAndAppendSelf
danakj ac0469b
bit-simpler
danakj acd9333
whitespace
danakj b8e5c89
complete-at-require-decl
danakj b92f9b1
no-recurse-needed
danakj 8219586
recurse-on-facets
danakj c183562
autoupdate
danakj 97c2a3c
comment
danakj bb93fd5
rewrites-complete
danakj 92e0bc1
autoupdate
danakj 47b1728
avoid-uaf
danakj c312a16
test-and-fix-multi-level-specific-resolution
danakj fe9a842
one-less-malloc
danakj aa2fcc0
add-comment
danakj 4fe0365
extends
danakj a35442d
no-k
danakj 56886b7
fix-test
danakj cc27be4
Revert "avoid-uaf"
danakj 9305cca
move-tests
danakj 05af854
try-move-requirecompletetype
danakj a18bbae
works-but-import-crash
danakj 3939132
wip
danakj ec8e2d4
works
danakj e1fc972
yay
danakj 00b9dc0
remove-todo-diagnostic
danakj 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
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
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 | ||
|---|---|---|---|---|
|
|
@@ -154,6 +154,7 @@ static auto TypeStructureReferencesSelf( | |||
|
|
||||
| struct ValidateRequireResult { | ||||
| SemIR::FacetType facet_type; | ||||
| SemIR::TypeId facet_type_type_id; | ||||
| const SemIR::IdentifiedFacetType* identified; | ||||
| }; | ||||
|
|
||||
|
|
@@ -164,12 +165,14 @@ static auto ValidateRequire(Context& context, SemIR::LocId loc_id, | |||
| SemIR::InstId constraint_inst_id, | ||||
| SemIR::InstId scope_inst_id) | ||||
| -> std::optional<ValidateRequireResult> { | ||||
| auto constraint_constant_value_inst_id = | ||||
| context.constant_values().GetConstantInstId(constraint_inst_id); | ||||
| auto constraint_facet_type = context.insts().TryGetAs<SemIR::FacetType>( | ||||
| constraint_constant_value_inst_id); | ||||
| auto constraint_constant_value_id = | ||||
| context.constant_values().Get(constraint_inst_id); | ||||
| auto constraint_type_id = | ||||
| SemIR::TypeId::ForTypeConstant(constraint_constant_value_id); | ||||
| auto constraint_facet_type = | ||||
| context.types().TryGetAs<SemIR::FacetType>(constraint_type_id); | ||||
| if (!constraint_facet_type) { | ||||
| if (constraint_constant_value_inst_id != SemIR::ErrorInst::InstId) { | ||||
| if (constraint_constant_value_id != SemIR::ErrorInst::ConstantId) { | ||||
| CARBON_DIAGNOSTIC( | ||||
| RequireImplsMissingFacetType, Error, | ||||
| "`require` declaration constrained by a non-facet type; " | ||||
|
|
@@ -218,6 +221,7 @@ static auto ValidateRequire(Context& context, SemIR::LocId loc_id, | |||
| } | ||||
|
|
||||
| return ValidateRequireResult{.facet_type = *constraint_facet_type, | ||||
| .facet_type_type_id = constraint_type_id, | ||||
| .identified = &identified}; | ||||
| } | ||||
|
|
||||
|
|
@@ -244,7 +248,7 @@ auto HandleParseNode(Context& context, Parse::RequireDeclId node_id) -> bool { | |||
| return true; | ||||
| } | ||||
|
|
||||
| auto [constraint_facet_type, identified] = *validated; | ||||
| auto [constraint_facet_type, constraint_type_id, identified] = *validated; | ||||
| if (identified->required_interfaces().empty()) { | ||||
| // A `require T impls type` adds no actual constraints, so nothing to do. | ||||
| DiscardGenericDecl(context); | ||||
|
|
@@ -271,8 +275,28 @@ auto HandleParseNode(Context& context, Parse::RequireDeclId node_id) -> bool { | |||
| require_impls_decl.require_impls_id = require_impls_id; | ||||
| ReplaceInstBeforeConstantUse(context, decl_id, require_impls_decl); | ||||
|
|
||||
| context.require_impls_stack().AppendToTop(require_impls_id); | ||||
| // We look for a complete type after BuildGenericDecl, so that the resulting | ||||
| // RequireCompleteType instruction is part of the enclosing interface or named | ||||
| // constraint generic definition. Then requiring enclosing entity to be | ||||
| // complete will resolve that definition (via ResolveSpecificDefinition()) and | ||||
| // also construct a specific for the `constraint_inst_id`, finding any | ||||
| // monomorphization errors that result. | ||||
| if (extend) { | ||||
| if (!RequireCompleteType( | ||||
| context, constraint_type_id, SemIR::LocId(constraint_inst_id), [&] { | ||||
| CARBON_DIAGNOSTIC(RequireImplsIncompleteFacetType, Error, | ||||
| "`extend require` of incomplete facet type {0}", | ||||
| InstIdAsType); | ||||
| return context.emitter().Build(constraint_inst_id, | ||||
| RequireImplsIncompleteFacetType, | ||||
| constraint_inst_id); | ||||
| })) { | ||||
| // DiscardGenericDecl(context); | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
| return true; | ||||
| } | ||||
| } | ||||
|
|
||||
| context.require_impls_stack().AppendToTop(require_impls_id); | ||||
| return true; | ||||
| } | ||||
|
|
||||
|
|
||||
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
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
139 changes: 139 additions & 0 deletions
139
toolchain/check/testdata/generic/extend_type_completion.carbon
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,139 @@ | ||
| // 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 | ||
| // | ||
| // INCLUDE-FILE: toolchain/testing/testdata/min_prelude/int.carbon | ||
| // | ||
| // AUTOUPDATE | ||
| // TIP: To test this file alone, run: | ||
| // TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/generic/extend_type_completion.carbon | ||
| // TIP: To dump output, run: | ||
| // TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/generic/extend_type_completion.carbon | ||
|
|
||
| // --- class_impl_doesnt_need_complete_interface.carbon | ||
| library "[[@TEST_NAME]]"; | ||
|
|
||
| interface K(T:! type) {} | ||
|
|
||
| class C(N:! i32) { | ||
| impl as K(array(i32, N)) {} | ||
| } | ||
|
|
||
| // C does not extend K so the type of K is not completed. No error. | ||
| var v: C(-1); | ||
|
|
||
| // --- fail_class_extend_impl_does_need_complete_interface.carbon | ||
| library "[[@TEST_NAME]]"; | ||
|
|
||
| interface K(T:! type) {} | ||
|
|
||
| class C(N:! i32) { | ||
| // CHECK:STDERR: fail_class_extend_impl_does_need_complete_interface.carbon:[[@LINE+3]]:18: error: array bound of -1 is negative [ArrayBoundNegative] | ||
| // CHECK:STDERR: extend impl as K(array(i32, N)) {} | ||
| // CHECK:STDERR: ^~~~~~~~~~~~~~~~ | ||
| extend impl as K(array(i32, N)) {} | ||
| } | ||
|
|
||
| // C extends K so the type of K is completed, but is invalid. | ||
| // CHECK:STDERR: fail_class_extend_impl_does_need_complete_interface.carbon:[[@LINE+4]]:8: note: in `C(-1)` used here [ResolvingSpecificHere] | ||
| // CHECK:STDERR: var v: C(-1); | ||
| // CHECK:STDERR: ^~~~~ | ||
| // CHECK:STDERR: | ||
| var v: C(-1); | ||
|
|
||
| // --- interface_require_impls_doesnt_need_complete_interface.carbon | ||
| library "[[@TEST_NAME]]"; | ||
|
|
||
| interface K(T:! type) {} | ||
| interface J(N:! i32) { | ||
| require impls K(array(i32, N)); | ||
| } | ||
|
|
||
| // J does not extend K so the type of K is not completed. No error. | ||
| var v: J(-1); | ||
|
|
||
| // --- fail_interface_extend_require_impls_does_need_complete_interface.carbon | ||
| library "[[@TEST_NAME]]"; | ||
|
|
||
| interface K(T:! type) {} | ||
| interface J(N:! i32) { | ||
| // CHECK:STDERR: fail_interface_extend_require_impls_does_need_complete_interface.carbon:[[@LINE+3]]:24: error: array bound of -1 is negative [ArrayBoundNegative] | ||
| // CHECK:STDERR: extend require impls K(array(i32, N)); | ||
| // CHECK:STDERR: ^~~~~~~~~~~~~~~~ | ||
| extend require impls K(array(i32, N)); | ||
| } | ||
| interface I(N:! i32) { | ||
| // CHECK:STDERR: fail_interface_extend_require_impls_does_need_complete_interface.carbon:[[@LINE+3]]:24: note: in `J(-1)` used here [ResolvingSpecificHere] | ||
| // CHECK:STDERR: extend require impls J(N); | ||
| // CHECK:STDERR: ^~~~ | ||
| extend require impls J(N); | ||
| } | ||
|
|
||
| // I extends J extends K so the type of K is completed, but is invalid. | ||
| // | ||
| // TODO: The error location should be the type, like in the class case above. We | ||
| // need a location for the type in context.bind_name_map() to use as the | ||
| // location to Convert(). | ||
| // | ||
| // CHECK:STDERR: fail_interface_extend_require_impls_does_need_complete_interface.carbon:[[@LINE+4]]:1: note: in `I(-1)` used here [ResolvingSpecificHere] | ||
| // CHECK:STDERR: var v: I(-1); | ||
| // CHECK:STDERR: ^~~~~~~~~~~~ | ||
| // CHECK:STDERR: | ||
| var v: I(-1); | ||
|
|
||
| // --- constraint_require_impls_doesnt_need_complete_interface.carbon | ||
| library "[[@TEST_NAME]]"; | ||
|
|
||
| interface K(T:! type) {} | ||
| constraint J(N:! i32) { | ||
| require impls K(array(i32, N)); | ||
| } | ||
|
|
||
| // J does not extend K so the type of K is not completed. No error. | ||
| var v: J(-1); | ||
|
|
||
| // --- fail_constraint_extend_require_impls_does_need_complete_interface.carbon | ||
| library "[[@TEST_NAME]]"; | ||
|
|
||
| interface K(T:! type) {} | ||
| constraint J(N:! i32) { | ||
| // CHECK:STDERR: fail_constraint_extend_require_impls_does_need_complete_interface.carbon:[[@LINE+3]]:24: error: array bound of -1 is negative [ArrayBoundNegative] | ||
| // CHECK:STDERR: extend require impls K(array(i32, N)); | ||
| // CHECK:STDERR: ^~~~~~~~~~~~~~~~ | ||
| extend require impls K(array(i32, N)); | ||
| } | ||
| constraint I(N:! i32) { | ||
| // CHECK:STDERR: fail_constraint_extend_require_impls_does_need_complete_interface.carbon:[[@LINE+3]]:24: note: in `{}` used here [ResolvingSpecificHere] | ||
| // CHECK:STDERR: extend require impls J(N); | ||
| // CHECK:STDERR: ^~~~ | ||
| extend require impls J(N); | ||
| } | ||
|
|
||
| // I extends J extends K so the type of K is completed, but is invalid. | ||
| // CHECK:STDERR: fail_constraint_extend_require_impls_does_need_complete_interface.carbon:[[@LINE+4]]:1: note: in `{}` used here [ResolvingSpecificHere] | ||
| // CHECK:STDERR: var v: I(-1); | ||
| // CHECK:STDERR: ^~~~~~~~~~~~ | ||
| // CHECK:STDERR: | ||
| var v: I(-1); | ||
|
|
||
| // --- interface_require_impls_doesnt_need_complete_self.carbon | ||
| library "[[@TEST_NAME]]"; | ||
|
|
||
| interface K {} | ||
| interface J(N:! i32) { | ||
| require array(Self, N) impls K; | ||
| } | ||
|
|
||
| // J does not extend K so the type of Self impling K is not completed. No error. | ||
| var v: J(-1); | ||
|
|
||
| // --- constraint_require_impls_doesnt_need_complete_self.carbon | ||
| library "[[@TEST_NAME]]"; | ||
|
|
||
| interface K {} | ||
| constraint J(N:! i32) { | ||
| require array(Self, N) impls K; | ||
| } | ||
|
|
||
| // J does not extend K so the type of Self impling K is not completed. No error. | ||
| var v: J(-1); |
Oops, something went wrong.
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.