Refactor Impl construction to clarify and reduce extraneous work #6420
Conversation
|
This is based on #6385, the first commit is |
Rebased to trunk. |
danakj
force-pushed
the
less-completing-impl-as
branch
from
c713931 to
181c03c
Compare
josh11b
left a comment
josh11b
left a comment
There was a problem hiding this comment.
Sorry for the delay in reviewing.
This PR seems a bit large, it ended up being pretty difficult to sort through all the changes. It would have helped a lot to break this into smaller changes.
| // If the explicit self type is the default, suggest removing it with a | ||
| // diagnostic, but continue as if no error occurred since the self-type | ||
| // is semantically valid. |
There was a problem hiding this comment.
FYI, since this is a recoverable error, we might want to make this into a warning, following https://docs.google.com/document/d/1-H-LwSllXydml49t-dHsUTOt8geY8ocskRMMRTtLfw0/edit?tab=t.0 .
There was a problem hiding this comment.
I thought about that but it's not recoverable in the sense that the code is correct but unintended. The code is syntactically wrong but we're able to fish for more errors by not inserting ErrorInst. I didn't think that sounded like a warning. Anyhow - if it is, let's address it separately.
| bool need_witness_table = | ||
| is_definition || !rewrites_into_interface_to_witness.empty(); | ||
| if (!need_witness_table) { | ||
| if (rewrites_into_interface_to_witness.empty()) { |
There was a problem hiding this comment.
Seems we lost && !is_definition in the most recent changes. I'm having difficulty following what is happening, there are too many changes happening together in a single PR.
There was a problem hiding this comment.
Yes, that is how we now have a single call to RequireCompleteType. I had attempted to explain the change in here: #6420 (comment)
We only need a non-placeholder witness table here if there is a rewrite into .Self. In that case we know .Self ensured the interface is already complete. For a declaration that is a definition the rule is the same.
Then we unconditionally RequireCompleteType of the facet type in the definition - inside the generic. This avoids the coordination between these two files to avoid doing RequireCompleteType twice.
| bool need_witness_table = | ||
| is_definition || !rewrites_into_interface_to_witness.empty(); | ||
| if (!need_witness_table) { | ||
| if (rewrites_into_interface_to_witness.empty()) { |
There was a problem hiding this comment.
Yes, that is how we now have a single call to RequireCompleteType. I had attempted to explain the change in here: #6420 (comment)
We only need a non-placeholder witness table here if there is a rewrite into .Self. In that case we know .Self ensured the interface is already complete. For a declaration that is a definition the rule is the same.
Then we unconditionally RequireCompleteType of the facet type in the definition - inside the generic. This avoids the coordination between these two files to avoid doing RequireCompleteType twice.
| // If the explicit self type is the default, suggest removing it with a | ||
| // diagnostic, but continue as if no error occurred since the self-type | ||
| // is semantically valid. |
There was a problem hiding this comment.
I thought about that but it's not recoverable in the sense that the code is correct but unintended. The code is syntactically wrong but we're able to fish for more errors by not inserting ErrorInst. I didn't think that sounded like a warning. Anyhow - if it is, let's address it separately.
2 participants
We were requiring the facet type to be complete in two different places, but this was not necessary. Now we only require it to be complete in a single place when checking the definition. If it was required earlier for a rewrite constraint, it would have been already checked and diagnosed in member lookup for the rewrite.
In the process reorganize and rename code to help clarify the relationships and behaviour between handle_impl.cpp and impl.cpp.
Move the diagnosing of
extend impl T asto the handler of theT, similar to where it's diagnosed forrequiredecls. This helps simplify the code, avoiding a bunch of looking through parse nodes for specific types. Make them both handle errors in the self type in a similar manner, avoiding a second diagnostic.