Require a definition in the same file as an impl declaration
#4719
Conversation
toolchain/check/check_unit.cpp
Outdated
| auto& impl = context_.impls().Get(impl_decl.impl_id); | ||
| if (!impl.is_defined()) { | ||
| CARBON_DIAGNOSTIC(MissingImplDefinition, Error, | ||
| "no definition found for declaration of impl"); |
There was a problem hiding this comment.
Maybe "impl declared but not defined"? I think we have previously said that we prefer diagnostics that state facts about the program rather than actions of the toolchain (eg, say the thing doesn't exist, not that we couldn't find it).
toolchain/check/impl.cpp
Outdated
| } | ||
| auto facet_type = context.types().TryGetAs<SemIR::FacetType>(facet_type_id); | ||
| if (!facet_type) { | ||
| CARBON_DIAGNOSTIC(ImplAsNonFacetType, Error, "impl as non-facet-type"); |
There was a problem hiding this comment.
| CARBON_DIAGNOSTIC(ImplAsNonFacetType, Error, "impl as non-facet-type"); | |
| CARBON_DIAGNOSTIC(ImplAsNonFacetType, Error, "impl as non-facet type"); |
Do you think it'd be useful to include the type facet_type_id in this diagnostic?
There was a problem hiding this comment.
Used impl.constraint_id instead of facet_type_id so we can eventually use the source's spelling of that type.
|
I've synced and the four failing tests have been updated:
I'll look at your suggestions tomorrow. |
| valid_redeclaration = IsValidImplRedecl(context, impl_info, prev_impl_id); | ||
| if (valid_redeclaration) { | ||
| impl_decl.impl_id = prev_impl_id; | ||
| } | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Create a new impl if this isn't a valid redeclaration. |
There was a problem hiding this comment.
FWIW I am finding it hard to follow the logic in this change. Specifically:
- There's now
valid_redeclarationas a bool. - The comment here shows we check if we set the
impl_decl.impl_idto something valid to see if it's a valid redeclaration. - The bool can be true without ever going into the point where we set
impl_decl.impl_id. So the bool means something similar, but different. - Then later we check the bool instead of
impl_decl.impl_idto see if it's a "valid redeclaration".
There was a problem hiding this comment.
You are right, this was the result of iterating until it behaved the way I wanted. The purpose of this bool was to plumb the information about whether we've already issued a particular error to the point where we can avoid producing another error. I think it would make sense to rename this !invalid_redeclaration, but I don't know if that goes far enough. Does #4738 help?
There was a problem hiding this comment.
yes it helps a lot with the intent of the bool.
I do think there's still a bit of collision with "valid redeclaration" meaning that impl_decl.impl_id.is_valid(). Maybe invalid_redeclaration would be better using a different name than "valid" and IsValidImplRedecl() could be renamed to match. I am not sure but maybe "legal" or something for that, since it's checking rules, whereas "valid" is used to refer to the id value.
Edit: clarifying grammar
Follow up to #4179, specifically re: #4719 (comment) . Co-authored-by: Josh L <[email protected]>
This PR detects the failures that #4709 fixes.