Merging forward declarations #3762

jonmeow · 2024-03-08T22:11:25Z

Add the extern keyword for forward declarations in libraries that don't
provide the definition.
Treat repeated forward declarations as redundant.
- Allow them when they prevent a dependence on an imported name.
Clarify rules for when modifier keywords should be on forward declarations
and definitions.

jonmeow · 2024-03-08T23:48:53Z

To note commits, the "Convert from Google doc" should pretty much be what's in Docs, which has edits to address comments that were made. "TODO cleanup" is cleaning up a couple TODOs that I'd left in the doc, including filling out some discord background that @josh11b highlighted.

zygoloid

What is our story for coherence in the presence of extern declarations, in particular extern type and interface declarations?

The best answer I can think of off the top of my head is that an impl lookup that involves an extern declaration in any way is invalid -- but that will have some serious implications if we define function calls in terms of impl lookup!

@josh11b Is this a problem that you've considered previously?

chandlerc

First pass of comments.

proposals/p3762.md

josh11b · 2024-03-11T15:18:14Z

What is our story for coherence in the presence of extern declarations, in particular extern type and interface declarations?

The best answer I can think of off the top of my head is that an impl lookup that involves an extern declaration in any way is invalid -- but that will have some serious implications if we define function calls in terms of impl lookup!

@zygoloid We can't use an extern type to satisfy the orphan rule in an impl definition, and extern types are incomplete, which limits them further.

There is an issue with impl lookup if you don't import the library with the definition (which is likely the point), but for one narrow case we can allow it: when we know all the implementations of a specific interface for the extern type. For this we need two ingredients:

an extern declaration saying what is implemented, and
a reason to understand that is complete.

We have these two ingredients in the case of an extern function declaration that is not an open extension point:

The extern declaration specifies what impl of the call operator exists for the type of the function in the other library.
Function overloading is closed, which says that the specified impls are a complete list.

This requires agreement of the whole overload set between the library that defines it and the extern declaration.

Co-authored-by: Chandler Carruth <[email protected]>

jonmeow

Thanks for the comments!

proposals/p3762.md

jonmeow · 2024-03-11T19:06:27Z

@josh11b @zygoloid Are you suggesting some change here, or is this just something to discuss?

josh11b · 2024-03-12T17:13:49Z

@josh11b @zygoloid Are you suggesting some change here, or is this just something to discuss?

@jonmeow I talked with @zygoloid yesterday, and I think we ended up in a position in between our original statements:

@zygoloid said impl lookup can't involve an extern type, but I think it is allowed to find extern impl declarations. It will be rare for the compiler to be able to succeed at resolving an impl lookup to a single result unless the found result is final, or it is a function overload. However, impl lookup can use extern impl declarations to prove that some impl exists, for example to be able to show that a call to a generic is allowed.
I said you might be able to use an extern type in an impl definition as long as it is not used to satisfy the orphan rule, and that is wrong. You may not use an extern type in the type structure of a non-extern impl. To see why, consider two libraries, one defining A and declaring B extern, and the other defining B and declaring A extern. Neither should be able to define an impl involving both A and B, otherwise both could.

Bottom line, I think we should include some statement about impls in this proposal, perhaps:

You may only use an extern type in the type structure of an extern impl.

It might be good to include the A and B example to justify this restriction.

jonmeow · 2024-03-12T20:48:50Z

@josh11b Added "Using extern declarations in extern impl" for this.

@zygoloid Added "impl files with a forward declaration must contain the definition" per discussion on #3763

chandlerc

I think I'm largely happy here, but would like @zygoloid to take a look as well before it goes in. Happy for it to merge when it LG to him as well.

zygoloid

Looks good, thanks. A few nuances, but nothing major.

proposals/p3762.md

zygoloid

I'm happy for this to go forward as-is. Please feel free to merge.

I'd also be happy to add a restriction on forward-declaring an entity in an impl file if it's also forward-declared in the api file. I don't see a need for that to be in this proposal, but I'm not hitting the merge button in case you would like to add that rule here.

jonmeow

Oops, forgot to publish responses

proposals/p3762.md

…ions. (#3843) Adds ImportIRId::ApiForImpl to reserve a specific slot for the `api` import, so that the code can trivially determine whether an import is from the same library. This is then used for merging function declarations, because the rules for redeclarations in the same library slightly differ as compared to other imports (note they're also not identical to same-file rules). The main thing this leaves from the recent #3762 is verifying that entities forward declared in the `impl` file are also defined, but that's not in-scope for merging; it's moreso post-checking validation. Note, a lot of our `invalid <entity> ID` comments in ids.h were incorrectly copy-pasted, so I've cut `<entity>`.

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 <[email protected]>

jonmeow added proposal A proposal proposal draft Proposal in draft, not ready for review labels Mar 8, 2024

Filling out template with PR 3762

75b5671

jonmeow force-pushed the proposal-merging-forward-decl branch from bf55cb9 to 946c340 Compare March 8, 2024 22:11

jonmeow added 2 commits March 8, 2024 15:12

Convert from Google doc

ff2d47d

TODO cleanup

19553ef

jonmeow marked this pull request as ready for review March 8, 2024 23:48

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 8, 2024

github-actions bot requested a review from zygoloid March 8, 2024 23:49

jonmeow force-pushed the proposal-merging-forward-decl branch from b70a45b to 98a6779 Compare March 8, 2024 23:50

Fix TOC

289337c

jonmeow force-pushed the proposal-merging-forward-decl branch from ae102c1 to e321f14 Compare March 8, 2024 23:53

zygoloid reviewed Mar 9, 2024

View reviewed changes

chandlerc reviewed Mar 9, 2024

View reviewed changes

jonmeow mentioned this pull request Mar 11, 2024

Matching redeclarations #3763

Merged

jonmeow force-pushed the proposal-merging-forward-decl branch from 507e7ff to ad5afce Compare March 11, 2024 17:59

jonmeow and others added 2 commits March 11, 2024 10:59

Apply suggestions from code review

fec567f

Co-authored-by: Chandler Carruth <[email protected]>

Addressing comments

94e8fc3

jonmeow commented Mar 11, 2024

View reviewed changes

Missed save

4650046

Addressing comments, redeclarations

4f0d82a

jonmeow force-pushed the proposal-merging-forward-decl branch from d5dc7fe to a6973f9 Compare March 12, 2024 20:47

Alternative syntax

59d4515

jonmeow force-pushed the proposal-merging-forward-decl branch from a6973f9 to 7808d5f Compare March 13, 2024 19:01

zygoloid added a commit to zygoloid/carbon-lang that referenced this pull request Mar 13, 2024

Remove and relocate content covered by carbon-language#3762.

0d37593

Migration notes

e368f83

jonmeow force-pushed the proposal-merging-forward-decl branch 3 times, most recently from 4cac1da to a5c0501 Compare March 14, 2024 21:18

chandlerc reviewed Mar 16, 2024

View reviewed changes

jonmeow force-pushed the proposal-merging-forward-decl branch 2 times, most recently from 0fed7ed to 6553478 Compare March 18, 2024 20:08

chandlerc requested a review from zygoloid March 18, 2024 22:22

zygoloid reviewed Mar 20, 2024

View reviewed changes

Addressing comments

453f673

jonmeow force-pushed the proposal-merging-forward-decl branch from 6553478 to f081368 Compare March 20, 2024 23:33

Remove unnecessary example code

b22749d

zygoloid approved these changes Mar 21, 2024

View reviewed changes

jonmeow commented Mar 21, 2024

View reviewed changes

jonmeow added 2 commits March 21, 2024 08:58

Decl example

4911951

appendix

d4a7cda

jonmeow force-pushed the proposal-merging-forward-decl branch from 5369fb9 to d4a7cda Compare March 21, 2024 21:25

example

4721a18

zygoloid added this pull request to the merge queue Mar 21, 2024

Merged via the queue into carbon-language:trunk with commit aacf0cd Mar 21, 2024
7 checks passed

jonmeow deleted the proposal-merging-forward-decl branch March 21, 2024 21:50

jonmeow mentioned this pull request Apr 2, 2024

Special-case api/impl implicit imports and verify relevant redeclarations. #3843

Merged

jonmeow mentioned this pull request Aug 19, 2024

Add documentation for entity declaration design work #4230

Merged

dwblaikie mentioned this pull request Oct 7, 2024

Disallow abstract or base on class declarations (that are not definitions) #4378

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merging forward declarations #3762

Merging forward declarations #3762

jonmeow commented Mar 8, 2024 •

edited

Loading

jonmeow commented Mar 8, 2024

zygoloid left a comment

chandlerc left a comment

josh11b commented Mar 11, 2024 •

edited

Loading

jonmeow left a comment

jonmeow commented Mar 11, 2024

josh11b commented Mar 12, 2024

jonmeow commented Mar 12, 2024

chandlerc left a comment

zygoloid left a comment

zygoloid left a comment

jonmeow left a comment

Merging forward declarations #3762

Merging forward declarations #3762

Conversation

jonmeow commented Mar 8, 2024 • edited Loading

jonmeow commented Mar 8, 2024

zygoloid left a comment

Choose a reason for hiding this comment

chandlerc left a comment

Choose a reason for hiding this comment

josh11b commented Mar 11, 2024 • edited Loading

jonmeow left a comment

Choose a reason for hiding this comment

jonmeow commented Mar 11, 2024

josh11b commented Mar 12, 2024

jonmeow commented Mar 12, 2024

chandlerc left a comment

Choose a reason for hiding this comment

zygoloid left a comment

Choose a reason for hiding this comment

zygoloid left a comment

Choose a reason for hiding this comment

jonmeow left a comment

Choose a reason for hiding this comment

jonmeow commented Mar 8, 2024 •

edited

Loading

josh11b commented Mar 11, 2024 •

edited

Loading