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

rustdoc: Fix ICE with doc(hidden) on tuple variant fields #88639

Merged
merged 1 commit into from
Sep 11, 2021

Conversation

Emilgardis
Copy link
Contributor

@Emilgardis Emilgardis commented Sep 4, 2021

Fixes #88600.

pub struct H;
pub struct S;

pub enum FooEnum {
    HiddenTupleItem(#[doc(hidden)] H),
    MultipleHidden(#[doc(hidden)] H, #[doc(hidden)] H),
    MixedHiddenFirst(#[doc(hidden)] H, S),
    MixedHiddenLast(S, #[doc(hidden)] H),
    HiddenStruct {
        #[doc(hidden)]
        h: H,
        s: S,
    },
}

Generates
image

@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 4, 2021
@Emilgardis
Copy link
Contributor Author

Emilgardis commented Sep 4, 2021

Generates from

pub enum FooEnum {
    HiddenTupleItem(#[doc(hidden)] ()),
    MultipleHidden(#[doc(hidden)] (), #[doc(hidden)] ()),
    MixedHiddenTupleItemLast((), #[doc(hidden)] ()),
    MixedHiddenTupleItemFirst(#[doc(hidden)] (), ()),
    NotHiddenTupleItem1(()),
    NotHiddenTupleItem2((), ()),
}

image

I don't like how the Variants are shown, preferably I'd have it as 0: /* omitted */ or something. This would probably be done in https://github.com/Emilgardis/rust/blob/838493dd0809720d9ec95680852e61a120d89838/src/librustdoc/html/render/print_item.rs#L1074-L1091 with an else clause

@@ -944,13 +944,17 @@ fn item_union(w: &mut Buffer, cx: &Context<'_>, it: &clean::Item, s: &clean::Uni
fn print_tuple_struct_fields(w: &mut Buffer, cx: &Context<'_>, s: &[clean::Item]) {
for (i, ty) in s
.iter()
.map(|f| if let clean::StructFieldItem(ref ty) = *f.kind { ty } else { unreachable!() })
.map(|f| if let clean::StructFieldItem(ref ty) = *f.kind { Some(ty) } else { None })
Copy link
Member

Choose a reason for hiding this comment

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

It should be possible at this stage, it means that the clean::Item was badly created... Can you find out where a non-StructFieldItem was added please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean it shouldn't be possible to create a clean::Item with StructFieldItem kind?

Copy link
Member

Choose a reason for hiding this comment

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

I meant that a tuple item (when creating a new one) should only have StructFieldItem and discard everything else. (In clean/mod.rs iirc, sorry, not on computer at the moment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The kind we end up with is

StrippedItem(StructFieldItem(Tuple([])))

my brief look into how the render works suggests to me that this is correct, and that it just needs to be handled.

Copy link
Member

@GuillaumeGomez GuillaumeGomez Sep 4, 2021

Choose a reason for hiding this comment

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

Isn't there the same issue with:

pub enum Foo {
    Bar {
        x: u32,
        #[doc(hidden)]
        y: u8,
    }
}

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That does not panic

image

pub struct Foo(#[doc(hidden)] ());

does not panic either, and outputs

image

Which gives me the idea to do the same _ for stripped/hidden tuple field

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Sep 4, 2021

Also, please add a regression test to ensure that this doesn't panic anymore in the future. But otherwise it looks like a great start, thanks!

EDIT: In src/test/rustdoc (also, please check that the output looks like what it's supposed to).

@rust-log-analyzer

This comment has been minimized.

@Emilgardis
Copy link
Contributor Author

Emilgardis commented Sep 4, 2021

I'm trying to figure out how I'd do HTML checking with @!has or @!matches. What absence should be checked?

would

// @!has issue_88600/enum.FooEnum.html '//*[@id="variant.HiddenTupleItem.field.0"]'

etc. be enough?

@GuillaumeGomez
Copy link
Member

Also check that the other fields are present, but otherwise seems good.

@Emilgardis
Copy link
Contributor Author

Emilgardis commented Sep 4, 2021

Fixed :D

Seems like

// @!has issue_88600/enum.FooEnum.html '//*[@id="variant.HiddenTupleItem.field.0"]'

doesn't actually work, need to specify a pattern to not match.

@Emilgardis
Copy link
Contributor Author

Emilgardis commented Sep 4, 2021

looks like this now:

image

vs how it looks on current stable
image

@jyn514
Copy link
Member

jyn514 commented Sep 5, 2021

I don't think this is a good idea. Enum fields are always public, they shouldn't be hidden from documentation. This should only fix the ICE, but not change the behavior (and actually I would suggest warning that doc(hidden) does nothing in that position as well).

@jyn514 jyn514 added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Sep 5, 2021
@jyn514 jyn514 changed the title remove ICE in rustdoc on hidden tuple fields Rustdoc: hide tuple fields marked with doc(hidden) Sep 5, 2021
@jyn514
Copy link
Member

jyn514 commented Sep 5, 2021

Also, @GuillaumeGomez please remember to check with the team before making changes in behavior like this, this is a feature we'd have to support into the future.

@Emilgardis
Copy link
Contributor Author

Emilgardis commented Sep 5, 2021

My previous CI run with nightly toolchain from where I encountered this ICE generate this output:

$ cargo +nightly-2021-08-21 -V
cargo 1.56.0-nightly (e96bdb0c3 2021-08-17)

image

so this particular change/pr is not really a change stable->nightly.

@jyn514
Copy link
Member

jyn514 commented Sep 5, 2021

Well that seems like a misfeature ... But I guess if it already existed that's fine.

Have you tested this respects --document-hidden-items?

@jyn514 jyn514 removed the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Sep 5, 2021
@Emilgardis
Copy link
Contributor Author

Well that seems like a misfeature ... But I guess if it already existed that's fine.

Have you tested this respects --document-hidden-items?

That works just fine, I think that's because strip-hidden is not ran so no StrippedItems are made with that option.

@GuillaumeGomez
Copy link
Member

Also, @GuillaumeGomez please remember to check with the team before making changes in behavior like this, this is a feature we'd have to support into the future.

My bad, I thought it was something already supported. What should we do then? Opening an FCP or just discard it and do what I suggested here?

@jyn514
Copy link
Member

jyn514 commented Sep 5, 2021

@GuillaumeGomez oh no, I was just confused - I thought this was a no-op before.

@GuillaumeGomez
Copy link
Member

Perfect, back to reviewing then. ;)

@GuillaumeGomez
Copy link
Member

Looks good to me, thanks! r=me once CI pass.

@camelid camelid changed the title Rustdoc: hide tuple fields marked with doc(hidden) rustdoc: Fix ICE with doc(hidden) on tuple variant fields Sep 5, 2021
@camelid
Copy link
Member

camelid commented Sep 5, 2021

It actually looks like rustdoc's behavior for this changed between stable and 2021-08-21. I don't think that change was intentional, so I'm wondering if we should revert the behavior change in this PR before it hits stable?

@camelid
Copy link
Member

camelid commented Sep 5, 2021

It looks likely that the behavior changed in #87451.

@Emilgardis
Copy link
Contributor Author

Emilgardis commented Sep 6, 2021

They all hide the field

Ok, then I think we should be consistent between tuple fields and struct variant fields and hide both if they have doc(hidden). (I still think it's weird rustdoc allowed this at all, but since it already exists we should support it and have it be consistent.) That means the warning won't be necessary, sorry to waste your time.

No worries! Ok, I'll fix the behaviour. I spent a good time trying to find out how to get the variant, but it's not wasted time, I now know more about the internals. Thanks for the help :)

So in summary.

  • #[doc(hidden)] should apply to both fields in tuple variants and struct variants.
    • fields in tuple variants are hidden with _.
    • fields in struct variants are hidden like in normal structs. /* (some) fields omitted */
  • These hidden fields should be hidden from the summary.
  • No warning should be emitted.

Ill add a doc-test to make sure the behaviour is consistent for both variants, I'll do this in a test called issue-88600.rs

For documentation, the lint code I ended up with was
    fn check_doc_hidden(&self, meta: &NestedMetaItem, hir_id: HirId, target: Target) -> bool {
        match target {
            Target::Field => {
                let parent_node_hir_id = self.tcx.hir().get_parent_node(hir_id);
                let parent_node = self.tcx.hir().find(parent_node_hir_id);
                if let Some(hir::Node::Variant(hir::Variant {
                    data: hir::VariantData::Tuple(..),
                    ..
                })) = parent_node
                {
                    self.tcx.struct_span_lint_hir(
                        INVALID_DOC_ATTRIBUTES,
                        hir_id,
                        meta.span(),
                        |lint| {
                            lint.build("#[doc(hidden)] does nothing on enum variant fields").emit()
                        },
                    );
                    return false;
                }
            }
            _ => {}
        }
        true
    }

@Emilgardis
Copy link
Contributor Author

image

This is how it looks now

@Emilgardis
Copy link
Contributor Author

Emilgardis commented Sep 6, 2021

I'd want to make sure the <code> block is checked as well, but I'm not sure how I would do that

this also renders them as `_`, which rustdoc previously did not.
@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 7, 2021

📌 Commit 4a915ac has been approved by GuillaumeGomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 7, 2021
@camelid
Copy link
Member

camelid commented Sep 7, 2021

Accepted for backport to 1.56 beta.

@rustbot label: +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 7, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 9, 2021
…aumeGomez

rustdoc: Fix ICE with `doc(hidden)` on tuple variant fields

Fixes rust-lang#88600.

```rust
pub struct H;
pub struct S;

pub enum FooEnum {
    HiddenTupleItem(#[doc(hidden)] H),
    MultipleHidden(#[doc(hidden)] H, #[doc(hidden)] H),
    MixedHiddenFirst(#[doc(hidden)] H, S),
    MixedHiddenLast(S, #[doc(hidden)] H),
    HiddenStruct {
        #[doc(hidden)]
        h: H,
        s: S,
    },
}
```

Generates
![image](https://user-images.githubusercontent.com/1502855/132259152-382f9517-c2a0-41d8-acd0-64e5993931fc.png)
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2021
…arth

Rollup of 15 pull requests

Successful merges:

 - rust-lang#85200 (Ignore derived Clone and Debug implementations during dead code analysis)
 - rust-lang#86165 (Add proc_macro::Span::{before, after}.)
 - rust-lang#87088 (Fix stray notes when the source code is not available)
 - rust-lang#87441 (Emit suggestion when passing byte literal to format macro)
 - rust-lang#88546 (Emit proper errors when on missing closure braces)
 - rust-lang#88578 (fix(rustc): suggest `items` be borrowed in `for i in items[x..]`)
 - rust-lang#88632 (Fix issues with Markdown summary options)
 - rust-lang#88639 (rustdoc: Fix ICE with `doc(hidden)` on tuple variant fields)
 - rust-lang#88667 (Tweak `write_fmt` doc.)
 - rust-lang#88720 (Rustdoc coverage fields count)
 - rust-lang#88732 (RustWrapper: avoid deleted unclear attribute methods)
 - rust-lang#88742 (Fix table in docblocks)
 - rust-lang#88776 (Workaround blink/chromium grid layout limitation of 1000 rows)
 - rust-lang#88807 (Fix typo in docs for iterators)
 - rust-lang#88812 (Fix typo `option` -> `options`.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0438048 into rust-lang:master Sep 11, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 11, 2021
@Emilgardis Emilgardis deleted the fix-issue-88600 branch September 11, 2021 08:15
@cuviper cuviper mentioned this pull request Sep 14, 2021
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 14, 2021
@cuviper cuviper modified the milestones: 1.57.0, 1.56.0 Sep 14, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2021
[beta] backports

- rustdoc: Fix ICE with `doc(hidden)` on tuple variant fields rust-lang#88639
- Fix 2021 `dyn` suggestion that used code as label rust-lang#88657
- Workaround blink/chromium grid layout limitation of 1000 rows rust-lang#88776
- Change scope of temporaries in match guards rust-lang#88678
- Add a regression test for rust-lang#88649 rust-lang#88691
- Revert anon union parsing rust-lang#88775
- Disable validate_maintainers. rust-lang#88977

Also drop stage0 rustfmt, because that's only supposed to be used on master.

r? `@Mark-Simulacrum`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

rustdoc ICE: entered unreachable code on hidden tuple item
10 participants