Skip to content
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

Stop allowing impl redeclarations to differ syntactically in where clause #4850

Merged
merged 10 commits into from
Jan 28, 2025

Conversation

josh11b
Copy link
Contributor

@josh11b josh11b commented Jan 27, 2025

Based on the lastest thinking on #4672 , require a full syntactic match for impl redeclaration, instead of excluding the where restriction. This means no updates to the impl witness on redeclaration, and no diagnostics that those updates are consistent.

Not included in this PR, but will need to be done in the future:

  • Support for assigning values to associated constants in the body of the impl definition. This will require moving the checking that non-function associated constants are set from the definition start to definition end.
  • Identify semantic redeclarations that are not syntactic matches to give a failed redeclaration diagnostic. This should be done once we are already identifying impl declarations with the same type structure in order to require they be identified in an impl_priority/match_first block.
  • Merging of the functions in check/impl.cpp that are now always called together.

Also add some test coverage of where parsing I developed in PR I've now abandoned because of this new simplification of the impl redeclaration semantics.

@github-actions github-actions bot requested a review from danakj January 27, 2025 23:20
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LG, just some minor style nits.

last_param_iter);
} while (where_operands_to_skip > 0);
}
Parse::NodeId last_param_node_id(end_of_decl_node_id.index - 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to keep treating NodeId::index as opaque, keeping the PostorderIterator use? As a reminder, #4762 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched back to using PostorderIterator.

toolchain/check/handle_impl.cpp Outdated Show resolved Hide resolved
toolchain/check/impl.cpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@josh11b josh11b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm holding off on merging since it looks like there are some regressions after I synced it to head.

@josh11b
Copy link
Contributor Author

josh11b commented Jan 28, 2025

I'm holding off on merging since it looks like there are some regressions after I synced it to head.

I fixed the bad merge, tests now pass.

@josh11b josh11b enabled auto-merge January 28, 2025 04:39
@josh11b josh11b added this pull request to the merge queue Jan 28, 2025
Merged via the queue into carbon-language:trunk with commit 5abe5a3 Jan 28, 2025
8 checks passed
@josh11b josh11b deleted the simplify branch January 28, 2025 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants