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

Matching redeclarations #3763

Merged
merged 13 commits into from
May 20, 2024

Conversation

zygoloid
Copy link
Contributor

@zygoloid zygoloid commented Mar 8, 2024

Require exact syntactic matching in redeclarations. Provide new terminology for
redeclaration matching and agreement. Specify non-redeclaration rules for the
other contexts where we require multiple declarations to match, such as impls
of interfaces, impls of virtual fns.

@zygoloid zygoloid added proposal A proposal proposal draft Proposal in draft, not ready for review labels Mar 8, 2024
@zygoloid zygoloid force-pushed the proposal-redeclarations branch from 9df4a27 to ef12d93 Compare March 8, 2024 23:50
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.

I had a couple questions about extern handling as mentioned here. I skimmed through everything else and 👍

proposals/p3763.md Show resolved Hide resolved
proposals/p3763.md Outdated Show resolved Hide resolved
proposals/p3763.md Outdated Show resolved Hide resolved
proposals/p3763.md Outdated Show resolved Hide resolved
proposals/p3763.md Outdated Show resolved Hide resolved
@zygoloid zygoloid changed the title Redeclarations Matching redeclarations Mar 13, 2024
@zygoloid zygoloid marked this pull request as ready for review March 13, 2024 22:31
@github-actions github-actions bot requested a review from chandlerc March 13, 2024 22:32
@github-actions github-actions bot added proposal rfc Proposal with request-for-comment sent out and removed proposal draft Proposal in draft, not ready for review labels Mar 13, 2024
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

Largely looks good. Mostly cosmetic comments on my end except for the thunk part for impl functions. But even that is really just trying to find a somewhat cleaner model, not suggesting shifting direction.

proposals/p3763.md Outdated Show resolved Hide resolved
proposals/p3763.md Outdated Show resolved Hide resolved
proposals/p3763.md Show resolved Hide resolved
proposals/p3763.md Outdated Show resolved Hide resolved
proposals/p3763.md Outdated Show resolved Hide resolved
@zygoloid zygoloid requested a review from chandlerc March 27, 2024 20:47
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

Sorry this somewhat fell off my radar, I think there's mostly just a wording/nuance tweak that I'd like to try to get working before landing still...

proposals/p3763.md Outdated Show resolved Hide resolved
proposals/p3763.md Outdated Show resolved Hide resolved
proposals/p3763.md Show resolved Hide resolved
proposals/p3763.md Show resolved Hide resolved
@chandlerc chandlerc requested a review from jonmeow May 14, 2024 01:56
proposals/p3763.md Show resolved Hide resolved
proposals/p3763.md Show resolved Hide resolved
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
proposals/p3763.md Outdated Show resolved Hide resolved
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

LG, shipping it!

@chandlerc chandlerc added this pull request to the merge queue May 20, 2024
Merged via the queue into carbon-language:trunk with commit 7fc69c0 May 20, 2024
7 checks passed
CJ-Johnson pushed a commit to CJ-Johnson/carbon-lang that referenced this pull request May 23, 2024
Require exact syntactic matching in redeclarations. Provide new
terminology for
redeclaration matching and agreement. Specify non-redeclaration rules
for the
other contexts where we require multiple declarations to match, such as
`impl`s
of `interfaces`, `impl`s of `virtual fn`s.

---------

Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
@zygoloid zygoloid deleted the proposal-redeclarations branch May 24, 2024 21:01
github-merge-queue bot pushed a commit that referenced this pull request Jun 28, 2024
This refactors how the implicit import is handled in order to retain
more name scope information. As a consequence, private access control
works better between api files and implementation files. Note though
that this will also be essential for name poisoning between the API and
implementation, as discussed in #3763.

In implementing this, I ran into a couple issues with namespaces that I
think point to flaws in their handling. I've fixed some and added a TODO
for the biggest issue (in check.cpp line 281-288), which relates to the
handling of namespaces of direct imports which are first evaluated
indirectly.
github-merge-queue bot pushed a commit that referenced this pull request Aug 21, 2024
Trying to pull in key elements of #3762, #3763, and #3980 (decl matching
and `extern`, essentially). These aren't specific to any particular
declaration type, but are common to entities, so suggesting a new doc
oriented on that.

There's probably more that could be said here, I'm just focused on
getting the recent formal discussion mirrored into the design.

---------

Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
github-merge-queue bot pushed a commit that referenced this pull request Sep 27, 2024
Update impl handling to more closely match other kinds of declaration,
including support for declaring and defining gneeric `impl`s.

When of performing redeclaration lookup for impls by looking for the
self and constraint type, produce a list of impls rather than a single
impl because it's possible for there to be multiple impls with different
deduced parameters but the same self type and constraint. Following
#3763, only consider impls to be redeclarations if they're spelled the
same, not just if they have the same self and constraint types.

No support yet for impl selection to deduce the arguments of a generic
impl.

---------

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
github-merge-queue bot pushed a commit that referenced this pull request Jan 7, 2025
* Implement ignoring the difference between `Self as` and `as`, as well
as `where` clauses at the end of an `impl` declaration when checking
whether `impl` declarations match, from #3763.
* Allow impl declarations with different constraint ids to match, as
long as the facet type of the constraint has the same interface_id and
specific_id.
* Add some TODOs reflecting future facet type resolution.

---------

Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal rfc Proposal with request-for-comment sent out proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants