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

Reword trait-object compatibility in rustdoc #126554

Closed

Conversation

kornelski
Copy link
Contributor

"Object Safety" is a difficult to understand jargon. I'm proposing a wording that makes it clearer it's about trait objects, and mentions dyn Trait which is the syntax that users are going to be more familiar with.

Related PR: rust-lang/reference#1512

Longer explanation: https://internals.rust-lang.org/t/object-safety-is-a-terrible-term/21025

@rustbot
Copy link
Collaborator

rustbot commented Jun 16, 2024

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jun 16, 2024
@fmease fmease added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Jun 18, 2024
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

Thanks for your initiative! I do agree with all of the points raised.

However, I'd like to see some official T-lang-blessed proposal. Basically what Josh has proposed here: https://internals.rust-lang.org/t/object-safety-is-a-terrible-term/21025/13 to ensure that we update the terminology everywhere consistently and "all at once" (docs, diagnostics, rustdoc, …).

Edit: I think Josh suggested to open a proposal over at https://github.com/rust-lang/lang-team.

@fmease fmease removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 18, 2024
@bors
Copy link
Contributor

bors commented Jun 24, 2024

☔ The latest upstream changes (presumably #126788) made this pull request unmergeable. Please resolve the merge conflicts.

@joshtriplett joshtriplett added the I-lang-nominated Nominated for discussion during a lang team meeting. label Aug 4, 2024
@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

When this ongoing FCP completes, we will have decided to call traits that are compatible with dyn "dyn compatible" rather than "object safe".

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Sep 25, 2024
@traviscross traviscross added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Sep 25, 2024
@fmease fmease added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Sep 27, 2024
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

The new nomenclature is "dyn-compatible", "dyn-compatibility". PR needs adjusting. See also rust-lang/lang-team#286

@@ -954,13 +954,13 @@ fn item_trait(w: &mut Buffer, cx: &mut Context<'_>, it: &clean::Item, t: &clean:
if !t.is_object_safe(cx.tcx()) {
write_section_heading(
w,
"Object Safety",
"Trait-Object Safety",
"object-safety",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"object-safety",
"dyn-compatibility"

might need to update other references of this ID

Copy link
Contributor

@traviscross traviscross Oct 3, 2024

Choose a reason for hiding this comment

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

As a nit, one would probably say that "a trait is dyn compatible" (but one might "have a dyn-compatible trait"), in the same way that we're more likely to hyphenate "a forward-compatible change" than "a change that is forward compatible".

Similarly, I'd probably say "dyn compatibility" unhyphenated, in the same way "forward compatibility" most often is.

Copy link
Member

@fmease fmease Oct 3, 2024

Choose a reason for hiding this comment

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

Yeah, that makes sense (attributive vs. predicative usage). I went for unconditional hyphenation here and in my other "object safe → dyn compatible" PRs for legibility (well, that's slightly subjective I guess).

Apart from personal preference, my argument goes as follows: If you skim source code comments and documentation, a "stray" dyn has a good chance of tripping up your brain (well, that's my assumption) especially since it's jargon, not everyday language and since brains are prone to overlooking small words.

Anyway, I'm fine with conditional hyphenation here as you've suggested (since it probably looks better in the rendered HTML and my "source code skimming" argument doesn't really apply).

Copy link
Member

Choose a reason for hiding this comment

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

Tho note that this specific string should stay hyphenated for unrelated reasons – it represents the element ID.

@@ -954,13 +954,13 @@ fn item_trait(w: &mut Buffer, cx: &mut Context<'_>, it: &clean::Item, t: &clean:
if !t.is_object_safe(cx.tcx()) {
write_section_heading(
w,
"Object Safety",
"Trait-Object Safety",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Trait-Object Safety",
"Dyn-Compatibility",

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Trait-Object Safety",
"Dyn Compatibility",

"object-safety",
None,
&format!(
"<div class=\"object-safety-info\">This trait is <b>not</b> \
"<div class=\"object-safety-info\">This trait is <strong>not</strong> compatible with \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"<div class=\"object-safety-info\">This trait is <strong>not</strong> compatible with \
"<div class=\"dyn-compat-info\">This trait is <strong>not</strong> compatible with \

and updating its references

"object-safety",
None,
&format!(
"<div class=\"object-safety-info\">This trait is <b>not</b> \
"<div class=\"object-safety-info\">This trait is <strong>not</strong> compatible with \
<a href=\"{base}/reference/items/traits.html#object-safety\">\
Copy link
Member

@fmease fmease Oct 3, 2024

Choose a reason for hiding this comment

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

You should leave a FIXME(dyn_compat_renaming): Update the URL fragment once the changes in <https://github.com/rust-lang/reference/pull/1512> reach stable.

<a href=\"{base}/reference/items/traits.html#object-safety\">\
object safe</a>.</div>",
<code>dyn Trait</code> types</a>.</div>",
Copy link
Member

@fmease fmease Oct 3, 2024

Choose a reason for hiding this comment

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

Right, I guess "dyn Trait types" is fine and easy to understand. The official term is "trait object types" but that might be too jargon-y, it's hard to tell for me ^^' I use the latter all the time.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2024
@fmease
Copy link
Member

fmease commented Oct 12, 2024

@kornelski I've opened a "competing" PR, namely #131594, which is in line with T-lang's latest resolution and a bit more comprehensive. I hope that's not too brash ^^'. Thing is, I'm having a good run updating various places that need updating and I want to land the renaming "simultaneously" (well, within the same release and ignoring the URL staging issue).

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 16, 2024
…pat, r=notriddle

rustdoc: Rename "object safe" to "dyn compatible"

Supersedes rust-lang#126554:

1. In line with [T-lang's latest resolution](rust-lang/lang-team#286 (comment)).
2. More comprehensive: Not only updates user-facing text but also source code.

Part of rust-lang#130852.

Doesn't update rustdoc-JSON (will be filed separately).

r? `@notriddle` (rust-lang/lang-team#286) `@GuillaumeGomez` (for visibility)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 16, 2024
Rollup merge of rust-lang#131594 - fmease:rustdoc-mv-obj-safe-dyn-compat, r=notriddle

rustdoc: Rename "object safe" to "dyn compatible"

Supersedes rust-lang#126554:

1. In line with [T-lang's latest resolution](rust-lang/lang-team#286 (comment)).
2. More comprehensive: Not only updates user-facing text but also source code.

Part of rust-lang#130852.

Doesn't update rustdoc-JSON (will be filed separately).

r? `@notriddle` (rust-lang/lang-team#286) `@GuillaumeGomez` (for visibility)
@fmease
Copy link
Member

fmease commented Oct 16, 2024

Closing in favor of the now merged #131594. kornelski, thanks for your initial PR and for bringing up this topic in the first place!

@fmease fmease closed this Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants