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

Destructor syntax #5017

Merged
merged 6 commits into from
Mar 5, 2025
Merged

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Feb 25, 2025

Fix destructor syntax ambiguity by switching to fn destroy mirroring standard function syntax. This is a purely syntactic change, maintaining destructor semantics.

This comes from leads question #4999

@jonmeow jonmeow added proposal A proposal proposal draft Proposal in draft, not ready for review labels Feb 25, 2025
@jonmeow jonmeow force-pushed the proposal-destructor-syntax branch from a2b707f to 7be4a5f Compare February 25, 2025 22:11
@jonmeow jonmeow force-pushed the proposal-destructor-syntax branch 7 times, most recently from f78897a to ef0f225 Compare February 25, 2025 23:07
@jonmeow jonmeow marked this pull request as ready for review February 25, 2025 23:08
@github-actions github-actions bot added proposal rfc Proposal with request-for-comment sent out documentation An issue or proposed change to our documentation and removed proposal draft Proposal in draft, not ready for review labels Feb 25, 2025
@github-actions github-actions bot requested a review from KateGregory February 25, 2025 23:08
@jonmeow jonmeow force-pushed the proposal-destructor-syntax branch from ef0f225 to 0db7620 Compare February 25, 2025 23:11

Note that non-keyword names were considered as part of proposal
[#1154: Destructors](https://github.com/carbon-language/carbon-lang/pull/1154),
and
Copy link
Contributor

Choose a reason for hiding this comment

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

incomplete sentence

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completed, thanks!

In particular, we are discussing destruction as possibly similar to copy and
move syntax, and trying to create a consistency between the functions.

## Proposal
Copy link
Contributor

Choose a reason for hiding this comment

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

I heard the leads say they would like an explicit syntax to say that the destructor is trivial. How would that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to what leads approved in #1154:

Types satisfy the
[`TrivialDestructor`](/docs/design/generics/details.md#destructor-constraints)
type-of-type if:

-   the class declaration does not define a destructor or the class defines the
    destructor with an empty body `{ }`,
-   all data members implement `TrivialDestructor`, and
-   all base classes implement `TrivialDestructor`.

For example, a [struct type](#struct-types) implements `TrivialDestructor` if
all its members do.

`TrivialDestructor` implies that their destructor does nothing, which may be
used to generate optimized specializations.

This proposal is not suggesting to change that at this time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note too, per minutes at https://docs.google.com/document/d/1Iut5f2TQBrtBNIduF4vJYOKfw7MbS8xH_J01_Q4e6Rk/edit?resourcekey=0-mc_vh5UzrzXfU4kO-3tOjA&tab=t.0, I believe the discussion about trivial destructors was constrained to the discussion about how destructors work behind the scenes -- which in this proposal, is the desugaring "future work".

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, my memory is that we did discuss wanting some explicit way to mark trivial cases, essentially forcing an error if it is mistaken.

I don't think that desire was especially connected to the desire for the semantics of these to map trivially into some set of impls of interfaces -- I think these are largely orthogonal.

Not sure why the minutes missed that or wore confusing here...

That said, this all still seems like future work. I've made a suggestion below to maybe document this in the future work without needing to dig into it further.

github-merge-queue bot pushed a commit that referenced this pull request Mar 3, 2025
Syntax is proposed in #5017, but has already been discussed with leads.
Semantics is left as a TODO.

---------

Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.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.

Generally looks pretty good to me. A suggestion for another future work note inline.

In particular, we are discussing destruction as possibly similar to copy and
move syntax, and trying to create a consistency between the functions.

## Proposal
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, my memory is that we did discuss wanting some explicit way to mark trivial cases, essentially forcing an error if it is mistaken.

I don't think that desire was especially connected to the desire for the semantics of these to map trivially into some set of impls of interfaces -- I think these are largely orthogonal.

Not sure why the minutes missed that or wore confusing here...

That said, this all still seems like future work. I've made a suggestion below to maybe document this in the future work without needing to dig into it further.

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.

Approving or leads with the suggested future work. Thanks!

jonmeow and others added 3 commits March 4, 2025 10:00
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
@jonmeow
Copy link
Contributor Author

jonmeow commented Mar 4, 2025

Minor note, committed a small typo fix where a sentence from the prior paragraph was repeated.

@jonmeow
Copy link
Contributor Author

jonmeow commented Mar 4, 2025

Also added back-references to classes.md, seemed better to do that here than a separate PR. Going to give some time in case someone wants to comment on these final cleanups.

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.

Still LG!

@jonmeow jonmeow added this pull request to the merge queue Mar 5, 2025
Merged via the queue into carbon-language:trunk with commit 10a87c0 Mar 5, 2025
8 checks passed
@jonmeow jonmeow deleted the proposal-destructor-syntax branch March 5, 2025 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation An issue or proposed change to our documentation 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