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

impl-everywhere.rs has a weird test #55201

Closed
Munksgaard opened this issue Oct 19, 2018 · 5 comments · Fixed by #78727
Closed

impl-everywhere.rs has a weird test #55201

Munksgaard opened this issue Oct 19, 2018 · 5 comments · Fixed by #78727
Assignees
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@Munksgaard
Copy link
Contributor

Munksgaard commented Oct 19, 2018

From #54824 (comment):

[ ] impl-everywhere.rs:// @!has foo/fn.foo.html '//section[@id="main"]//pre' "x: &'x impl Foo"

This test looks strange. The generated documentation looks like this (wrapped in pre):

pub fn foo<'x>(
    x: &'x impl Foo
) -> &'x impl Foo

so it seems like the backslash in the test is taken literally which means that it doesn't match. Is that on purpose? @GuillaumeGomez it seems like you created the test, so perhaps you can enlighten me?

Edit: To be clear, removing the backslash from the test causes the test to fail.

My understanding is that test is trying to assert that fn.foo.html doesn't contain the phrase x: &' impl Foo, but because it is erroneously escaping a quote it fails to make that assertion. Furthermore, the generated fn.foo.html file actually does contain the text being checked for, so there is a bug in rustdoc as well.

@Munksgaard
Copy link
Contributor Author

This issue should be assigned to @GuillaumeGomez, as per his request in #54824 (comment)

@GuillaumeGomez GuillaumeGomez self-assigned this Oct 19, 2018
@GuillaumeGomez GuillaumeGomez added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Oct 19, 2018
@jonas-schievink jonas-schievink added A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. labels Jul 29, 2019
@liketechnik
Copy link
Contributor

@rustbot claim

@liketechnik
Copy link
Contributor

I've made the following observations:

  • The current output of rustdoc for the test is the same as the output with the initial implementation used with the test (e67bba8) and also seems to match with the initial goal of the implementation in Fix missing impl trait display as ret type #53541. (I don't think the actual output of rustdoc is necessary here, but can provide that if needed).
  • The check has not been modified since its creation, I haven't been able to compare the implementations of the relevant parts in rustdoc though, due to my unfamiliarity with rustdocs code.

My interpretation is that there is no bug in rustdoc (since the output stayed the same), but the test simply never validated the output correctly, because

  • all checks validate the absence (all checks start with @!has) of an impl Type in parameters or return values, while the original goal of the implementation (see Fix missing impl trait display as ret type #53541) was to add support for impl Type in parameters and return values. Example:
    // @!has foo/fn.foo.html '//section[@id="main"]//pre' "x: &\'x impl Foo"
    // @!has foo/fn.foo.html '//section[@id="main"]//pre' "-> &\'x impl Foo {"
    pub fn foo<'x>(x: &'x impl Foo) -> &'x impl Foo {
    which would generate pub fn foo<'x>(x: &'x impl Foo) -> &'x impl Foo as documentation
  • all checks expect an opening brace { after the return type, which is not generated by rustdoc. This makes the checks work, because now the 'expected' (see point 1) mismatch occurs.
  • all checks with a lifetime were erroneously escaping a quote. Again, this makes the checks work, because now the 'expected' mismatch occurs.

If my observations are correct, I would go forward by opening a pr fixing the issues listed above for the test (e. g. making the test fail for possibly incorrect output by rustdoc in the future).

@jyn514
Copy link
Member

jyn514 commented Nov 4, 2020

@GuillaumeGomez can you confirm whether the summary above is correct?

@GuillaumeGomez
Copy link
Member

Yes, it seems to be correct. Thanks for the summary @liketechnik ! Please feel free to open the PR. :)

JohnTitor added a commit to JohnTitor/rust that referenced this issue Nov 5, 2020
…eGomez

(rustdoc) fix test for trait impl display

The test checks that parameters and return values with `impl Trait` types are correctly generated in rustdoc's output.

In essence, the previous version of the test checked the absence of values that would never be generated by rustdoc, so it could basically never fail. These values were adjusted to the expected output and are now required to exist in rustdoc's output. See rust-lang#55201 (comment) for a detailed explanation of the reasoning behind the changes.

Note that the output of rustdoc for `impl Trait`s in parameters and return values did not change since the inital test creation, so this PR only modifies the test.

Closes rust-lang#55201
JohnTitor added a commit to JohnTitor/rust that referenced this issue Nov 5, 2020
…eGomez

(rustdoc) fix test for trait impl display

The test checks that parameters and return values with `impl Trait` types are correctly generated in rustdoc's output.

In essence, the previous version of the test checked the absence of values that would never be generated by rustdoc, so it could basically never fail. These values were adjusted to the expected output and are now required to exist in rustdoc's output. See rust-lang#55201 (comment) for a detailed explanation of the reasoning behind the changes.

Note that the output of rustdoc for `impl Trait`s in parameters and return values did not change since the inital test creation, so this PR only modifies the test.

Closes rust-lang#55201
m-ou-se added a commit to m-ou-se/rust that referenced this issue Nov 5, 2020
…eGomez

(rustdoc) fix test for trait impl display

The test checks that parameters and return values with `impl Trait` types are correctly generated in rustdoc's output.

In essence, the previous version of the test checked the absence of values that would never be generated by rustdoc, so it could basically never fail. These values were adjusted to the expected output and are now required to exist in rustdoc's output. See rust-lang#55201 (comment) for a detailed explanation of the reasoning behind the changes.

Note that the output of rustdoc for `impl Trait`s in parameters and return values did not change since the inital test creation, so this PR only modifies the test.

Closes rust-lang#55201
@bors bors closed this as completed in 9bbb052 Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants