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

Rollup of 5 pull requests #91080

Merged
merged 19 commits into from
Nov 20, 2021
Merged

Rollup of 5 pull requests #91080

merged 19 commits into from
Nov 20, 2021

Conversation

matthiaskrgr
Copy link
Member

Successful merges:

Failed merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

Noratrieb and others added 19 commits November 15, 2021 20:40
This also fixes the same suggestion, which was kind of broken, because it just searched for the last occurence of `const` to replace with a `let`. This works great in some cases, but when there is no const and a leading space to the file, it doesn't work and panic with overflow because it thought that it had found a const.

I also changed the suggestion to only trigger if the `const` and the non-constant value are on the same line, because if they aren't, the suggestion is very likely to be wrong.

Also don't trigger the suggestion if the found `const` is on line 0, because that triggers the ICE.
…ewline

I cannot provide a test for that thanks to tidy.
…aused-by-reexporting-pub-crate-visibility-to-outside, r=oli-obk

Clarify error messages caused by re-exporting `pub(crate)` visibility to outside

This PR clarifies error messages and suggestions caused by re-exporting pub(crate) visibility outside the crate.

Here is a small example ([Rust Playground](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=e2cd0bd4422d4f20e6522dcbad167d3b)):

```rust
mod m {
    pub(crate) enum E {}
}
pub use m::E;

fn main() {}
```

This code is compiled to:

```
error[E0365]: `E` is private, and cannot be re-exported
 --> prog.rs:4:9
  |
4 | pub use m::E;
  |         ^^^^ re-export of private `E`
  |
  = note: consider declaring type or module `E` with `pub`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0365`.
```

However, enum `E` is actually public to the crate, not private totally—nevertheless, rustc treats `pub(crate)` and private visibility as the same on the error messages. They are not clear and should be segmented distinctly.

By applying changes in this PR, the error message below will be the following message that would be clearer:

```
error[E0365]: `E` is only public to inside of the crate, and cannot be re-exported outside
 --> prog.rs:4:9
  |
4 | pub use m::E;
  |         ^^^^ re-export of crate public `E`
  |
  = note: consider declaring type or module `E` with `pub`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0365`.
```
…r=estebank

Fix `non-constant value` ICE (rust-lang#90878)

This also fixes the same suggestion, which was kind of broken, because it just searched for the last occurence of `const` to replace with a `let`. This works great in some cases, but when there is no const and a leading space to the file, it doesn't work and panic with overflow because it thought that it had found a const.

I also changed the suggestion to only trigger if the `const` and the non-constant value are on the same line, because if they aren't, the suggestion is very likely to be wrong.

Also don't trigger the suggestion if the found `const` is on line 0, because that triggers the ICE.

Asking Esteban to review since he was the last one to change the relevant code.

r? ``@estebank``

Fixes rust-lang#90878
…=jsha

Make scrollbar in the sidebar always visible for visual consistency

Fixes rust-lang#90943.

I had to add a background in `dark` and `ayu` themes, otherwise it was looking strange (like an invisible margin). So it looks like this:

![Screenshot from 2021-11-17 14-45-49](https://user-images.githubusercontent.com/3050060/142212476-18892ae0-ba4b-48e3-8c0f-4ca1dd2f851d.png)
![Screenshot from 2021-11-17 14-45-53](https://user-images.githubusercontent.com/3050060/142212482-e97b2fad-68d2-439a-b62e-b56e6ded5345.png)

Sadly, I wasn't able to add a GUI test to ensure that the scrollbar was always displayed because it seems not possible in puppeteer for whatever reason... I used this method: on small pages (like `lib2/sub_mod/index.html`), comparing `.navbar`'s `clientWidth` with `offsetWidth` (the first doesn't include the sidebar in the computed amount). When checking in the browser, it works fine but in puppeteer it almost never works...

In case anyone want to try to solve the bug, here is the puppeteer code:

<details>
More information about this: I tried another approach which was to get the element in `evaluate` directly (by calling it from `page.evaluate(() => { .. });` directly instead of `parseAssertElemProp.evaluate(e => {...});`.

```js
const puppeteer = require('puppeteer');

(async () => {
    const browser = await puppeteer.launch();
    const page = await browser.newPage();
    await page.goto("file:///path/rust/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/lib2/sub_mod/index.html");
    await page.waitFor(".sidebar");
    let parseAssertElemProp = await page.$(".sidebar");
    if (parseAssertElemProp === null) { throw '".sidebar" not found'; }
    await parseAssertElemProp.evaluate(e => {
        const parseAssertElemPropDict = {"clientWidth": "192", "offsetWidth":"200"};
        for (const [parseAssertElemPropKey, parseAssertElemPropValue] of Object.entries(parseAssertElemPropDict)) {
            if (e[parseAssertElemPropKey] === undefined || String(e[parseAssertElemPropKey]) != parseAssertElemPropValue) {
                throw 'expected `' + parseAssertElemPropValue + '` for property `' + parseAssertElemPropKey + '` for selector `.sidebar`, found `' + e[parseAssertElemPropKey] + '`';
            }
        }
    }).catch(e => console.error(e));
    await browser.close();
})();
```

</details>

r? ``@jsha``
…, r=estebank

Elaborate `Future::Output` when printing opaque `impl Future` type

I would love to see the `Output =` type when printing type errors involving opaque `impl Future`.

[Test code](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=a800b481edd31575fbcaf5771a9c3678)

Before (cut relevant part of output):
```
note: while checking the return type of the `async fn`
 --> /home/michael/test.rs:5:19
  |
5 | async fn bar() -> usize {
  |                   ^^^^^ checked the `Output` of this `async fn`, found opaque type
  = note:     expected type `usize`
          found opaque type `impl Future`
```

After:
```
note: while checking the return type of the `async fn`
 --> /home/michael/test.rs:5:19
  |
5 | async fn bar() -> usize {
  |                   ^^^^^ checked the `Output` of this `async fn`, found opaque type
  = note:     expected type `usize`
          found opaque type `impl Future<Output = usize>`
```

Note the "found opaque type `impl Future<Output = usize>`" in the new output.

----

Questions:
1. We skip printing the output type when it's a projection, since I have been seeing some types like `impl Future<Output = <[static generator@/home/michael/test.rs:2:11: 2:21] as Generator<ResumeTy>>::Return>` which are not particularly helpful and leak implementation detail.
    * Am I able to normalize this type within `rustc_middle::ty::print::pretty`? Alternatively, can we normalize it when creating the diagnostic? Otherwise, I'm fine with skipping it and falling back to the old output.
    * Should I suppress any other types? I didn't encounter anything other than this generator projection type.
2. Not sure what the formatting of this should be. Do I include spaces in `Output = `?
@rustbot rustbot added the rollup A PR which is a rollup label Nov 20, 2021
@matthiaskrgr
Copy link
Member Author

@bors r+ rollup=never p=5

@bors
Copy link
Contributor

bors commented Nov 20, 2021

📌 Commit 3379721 has been approved by matthiaskrgr

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 20, 2021
@bors
Copy link
Contributor

bors commented Nov 20, 2021

⌛ Testing commit 3379721 with merge 3d78974...

@bors
Copy link
Contributor

bors commented Nov 20, 2021

☀️ Test successful - checks-actions
Approved by: matthiaskrgr
Pushing 3d78974 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 20, 2021
@bors bors merged commit 3d78974 into rust-lang:master Nov 20, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 20, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3d78974): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@matthiaskrgr matthiaskrgr deleted the rollup-znh88cy branch November 20, 2021 15:02
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 17, 2022
…nd-field-in-variant, r=davidtwco

Fix `try wrapping expression in variant` suggestion with struct field shorthand

Fixes a broken suggestion: [playground](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=83fe2dbfe1485f8cfca1aef2a6582e77)

before:
```
error[E0308]: mismatched types
 --> src/main.rs:7:19
  |
7 |     let x = Foo { bar };
  |                   ^^^ expected enum `Option`, found integer
  |
  = note: expected enum `Option<i32>`
             found type `{integer}`
help: try wrapping the expression in `Some`
  |
7 |     let x = Foo { Some(bar) };
  |                   +++++   +
```

after:
```
error[E0308]: mismatched types
 --> src/main.rs:7:19
  |
7 |     let x = Foo { bar };
  |                   ^^^ expected enum `Option`, found integer
  |
  = note: expected enum `Option<i32>`
             found type `{integer}`
help: try wrapping the expression in `Some`
  |
7 |     let x = Foo { bar: Some(bar) };
  |                   ~~~~~~~~~~~~~~
```

r? ``@m-ou-se``
since you touched the code last in rust-lang#91080
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. rollup A PR which is a rollup S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants