Rework how we check calls to support deduced implicit parameters #4302
Conversation
generic classes and interfaces.
This allows generic classes and interfaces nested within another generic to work properly again.
diagnostic quality.
|
Let me know if you'd prefer that I try to split some smaller parts out of this for review purposes -- there's probably a few pieces here that could be landed separately. |
| @@ -829,6 +837,14 @@ class FormatterImpl { | |||
| } | |||
| } | |||
|
|
|||
| auto FormatInstRHS(GenericInterfaceType inst) -> void { | |||
| if (inst.enclosing_specific_id.is_valid()) { | |||
There was a problem hiding this comment.
Minor suggestion, comparing with FunctionType, ClassType, and InterfaceType, maybe add a helper:
// Shared formatting for entities that have an optional specific.
template<typename EntityId>
auto FormatInstRHSForEntity(EntityId entity_id, SpecificId specific_id) -> void {
if (specific_id.is_valid()) {
FormatArgs(entity_id, specific_id);
} else {
FormatArgs(entity_id);
}
}
There was a problem hiding this comment.
Yeah, we have a bunch of instructions like this. I'll send a follow-up PR for this.
toolchain/lower/file_context.cpp
Outdated
| if (!param.index.is_valid()) { | ||
| // Parameter is not passed at runtime. |
There was a problem hiding this comment.
Do you think the comment would still be helpful if the name were param.runtime_index?
Partly noting due to the identical comment on 316. It strikes me a related option might be, had you considered starting to add accessors to the typed instructions, e.g. param.is_runtime() or is_lowered?
Note, I'm also fine leaving this as-is if you prefer the comment, the code is clear.
There was a problem hiding this comment.
Renamed to runtime_index.
| SemIR::SpecificId specific_id; | ||
| }; | ||
|
|
||
| static auto ResolveCalleeInCall(Context& context, SemIR::LocId loc_id, |
There was a problem hiding this comment.
This looks like a pretty complex signature, is it worth a comment? What's the semantic of self_id?
toolchain/check/call.cpp
Outdated
| callee_info.implicit_param_refs_id, callee_info.param_refs_id, self_id, | ||
| arg_ids); | ||
| if (!specific_id.is_valid()) { | ||
| return {.is_valid = false, .specific_id = SemIR::SpecificId::Invalid}; |
There was a problem hiding this comment.
Had you considered a std::optional<SpecificId> return, with nullopt for invalid calls?
| if (!param_inst) { | ||
| // Once we support more generalized patterns we will need to diagnose | ||
| // parameters with unsupported patterns. | ||
| context.TODO(param_id, "unexpected syntax for parameter"); |
There was a problem hiding this comment.
There aren't any tests hitting this case, can there be, or is the code just not there yet?
There was a problem hiding this comment.
The code to parse more complex patterns isn't there yet; I've added a test so we should cover this once we get the parsing support.
| // This is the end of the first phase. Don't make a new class yet if | ||
| // we already have new work. |
There was a problem hiding this comment.
This is marked as the end of the first phase, but so is line 1289. Is this actually becoming four phases? Should the check on line 1288 just be removed, comparing with the interface version?
I'll be approving assuming 1288 should just be removed, please let me know if I'm misunderstanding.
(note, this inconsistency makes me wonder if there's a good way to factor towards more code sharing)
There was a problem hiding this comment.
Thanks, the second check is redundant and it looks like I forgot to delete it. Deleted now matching the other cases.
Yeah, I think it'd be good to look into refactoring function / class / interface handling.
Instead of the
callinstruction having a block with one argument per explicit argument, preceded optionally byselfand followed optionally by a return slot, change thecallto store only the runtime arguments. Store an index on the runtime parameters to make it easier to determine the correspondence between arguments and parameters in a call. Compile-time parameters, whether implicit or explicit, are no longer included in the call argument list. Instead, they're tracked only in thespecific_idon the callee.For calls to generic classes and generic interfaces, it no longer makes sense to form a
callinstruction, given that the entirety of the result is determined by thespecific_id, which is now formed when checking the call. Instead, thecallinstruction now only models function calls, and not calls to other kinds of parameterized entity names, and we create aclass_typeorinterface_typeinstead of acallinstruction to model these kinds of calls. Notionally the model here is that we're following the #3720 approach for calls, but for now we inline theCall.Opfunction when forming SemIR.We now also track the enclosing specific for a generic class or generic interface that appears within an enclosing generic. This is necessary in order for deduction of the inner generic parameters to not get confused by the outer generic parameters being absent.
In order to not regress diagnostics, the template argument deduction mechanism has been extended to specify the name of the parameter we're deducing against when possible, and call arity mismatch errors are now diagnosed before performing deduction rather than afterwards.