Fix syntactic match of impl decl to definition #4709
Conversation
toolchain/check/handle_impl.cpp
Outdated
| // Subtracting 1 since we don't want to include the final `{` or `;` of the | ||
| // declaration when performing syntactic match. | ||
| Parse::NodeId last_param_node_id(end_of_decl_node_id.index - 1); |
There was a problem hiding this comment.
Optional: what do you think about a CHECK that the final token we're excluding is in fact { or ;?
There was a problem hiding this comment.
Added, along with a TODO comment.
| // CHECK:STDOUT: %Interface.ref: type = name_ref Interface, file.%Interface.decl [template = constants.%Interface.type] | ||
| // CHECK:STDOUT: %T.param: %I.type = value_param runtime_param<invalid> | ||
| // CHECK:STDOUT: %T.loc11_14.1: %I.type = bind_symbolic_name T, 0, %T.param [symbolic = %T.loc11_14.2 (constants.%T.1)] | ||
| // CHECK:STDOUT: %I.ref.loc11: type = name_ref I, file.%I.decl [template = constants.%I.type] |
There was a problem hiding this comment.
This is a surprising change for this PR -- is there some other change that was included when generating this?
There was a problem hiding this comment.
The reason this name changes is because before we had a separate definition with another %I.ref and after this change that definition instead reopens this impl declaration, so now there are two %I.refs in one scope.
| // CHECK:STDOUT: %int_32: Core.IntLiteral = int_value 32 [template = constants.%int_32] | ||
| // CHECK:STDOUT: %i32: type = class_type @Int, @Int(constants.%int_32) [template = constants.%i32] | ||
| // CHECK:STDOUT: %I.ref: type = name_ref I, file.%I.decl [template = constants.%I.type] | ||
| // CHECK:STDOUT: impl_decl @impl [template] {} { |
There was a problem hiding this comment.
My understanding is this is the result of no longer considering ;and { sufficient to differentiate two impls?
Is it possible yet to write a test that fundamentally fails without this fix rather than relying on catching it in the SemIR?
There was a problem hiding this comment.
I have a future PR that detects these errors. The fact that the tests before were passing is because it was ignoring the declaration, making it hard to create a failure when matching fails.
There was a problem hiding this comment.
Future PR is #4719, which is currently failing but should pass once this is merged.
This PR detects the failures that #4709 fixes. --------- Co-authored-by: Josh L <josh11b@users.noreply.github.com>
No description provided.