Rollup of 4 pull requests
Successful merges:
- #91008 (Adds IEEE 754-2019 minimun and maximum functions for f32/f64)
- #91070 (Make `LLVMRustGetOrInsertGlobal` always return a `GlobalVariable`)
- #91097 (Add spaces in opaque `impl Trait` with more than one trait)
- #91098 (Don't suggest certain fixups (`.field`, `.await`, etc) when reporting errors while matching on arrays )
Failed merges:
r? `@ghost`
`@rustbot` modify labels: rollup
Don't suggest certain fixups (`.field`, `.await`, etc) when reporting errors while matching on arrays
When we have a type mismatch with a `cause.code` that is an `ObligationCauseCode::Pattern`, skip suggesting fixes like adding `.await` or accessing a struct's `.field` if the pattern's `root_ty` differs from the `expected` ty. This occurs in situations like this:
```rust
struct S(());
fn main() {
let array = [S(())];
match array {
[()] => {}
_ => {}
}
}
```
I think what's happening here is a layer of `[_; N]` is peeled off of both types and we end up seeing the mismatch between just `S` and `()`, but when we suggest a fixup, that applies to the expression with type `root_ty`.
---
Questions:
1. Should this check live here, above all of the suggestions, or should I push this down into every suggestion when we match `ObligationCauseCode`?
2. Any other `ObligationCauseCode`s to check here?
3. Am I overlooking an easier way to get to this same conclusion without pattern matching on `ObligationCauseCode` and comparing `root_ty`?
Fixes#91058
Make `LLVMRustGetOrInsertGlobal` always return a `GlobalVariable`
`Module::getOrInsertGlobal` returns a `Constant*`, which is a super
class of `GlobalVariable`, but if the given type doesn't match an
existing declaration, it returns a bitcast of that global instead.
This causes UB when we pass that to `LLVMGetVisibility` which
unconditionally casts the opaque argument to a `GlobalValue*`.
Instead, we can do our own get-or-insert without worrying whether
existing types match exactly. It's not relevant when we're just trying
to get/set the linkage and visibility, and if types are needed we can
bitcast or error nicely from `rustc_codegen_llvm` instead.
Fixes#91050, fixes#87933, fixes#87813.
Adds IEEE 754-2019 minimun and maximum functions for f32/f64
IEEE 754-2019 removed the `minNum` (`min` in Rust) and `maxNum` (`max` in Rust) operations in favor of the newly created `minimum` and `maximum` operations due to their [non-associativity](https://grouper.ieee.org/groups/msc/ANSI_IEEE-Std-754-2019/background/minNum_maxNum_Removal_Demotion_v3.pdf) that cannot be fix in a backwards compatible manner. This PR adds `fN::{minimun,maximum}` functions following the new rules.
### IEEE 754-2019 Rules
> **minimum(x, y)** is x if x < y, y if y < x, and a quiet NaN if either operand is a NaN, according to 6.2.
For this operation, −0 compares less than +0. Otherwise (i.e., when x = y and signs are the same)
it is either x or y.
> **maximum(x, y)** is x if x > y, y if y > x, and a quiet NaN if either operand is a NaN, according to 6.2.
For this operation, +0 compares greater than −0. Otherwise (i.e., when x = y and signs are the
same) it is either x or y.
"IEEE Standard for Floating-Point Arithmetic," in IEEE Std 754-2019 (Revision of IEEE 754-2008) , vol., no., pp.1-84, 22 July 2019, doi: 10.1109/IEEESTD.2019.8766229.
### Implementation
This implementation is inspired by the one in [`glibc` ](90f0ac10a7/math/s_fminimum_template.c) (it self derived from the C2X draft) expect that:
- it doesn't use `copysign` because it's not available in `core` and also because `copysign` is unnecessary (we only want to check the sign, no need to create a new float)
- it also prefer `other > self` instead of `self < other` like IEEE 754-2019 does
I originally tried to implement them [using intrinsics](1d8aa13bc3) but LLVM [error out](https://godbolt.org/z/7sMrxW49a) when trying to lower them to machine intructions, GCC doesn't yet have built-ins for them, only cranelift support them nativelly (as it doesn't support the nativelly the old sementics).
Helps with https://github.com/rust-lang/rust/issues/83984
Point at source of trait bound obligations in more places
Be more thorough in using `ItemObligation` and `BindingObligation` when
evaluating obligations so that we can point at trait bounds that
introduced unfulfilled obligations. We no longer incorrectly point at
unrelated trait bounds (`substs-ppaux.verbose.stderr`).
In particular, we now point at trait bounds on method calls.
We no longer point at "obvious" obligation sources (we no longer have a
note pointing at `Trait` saying "required by a bound in `Trait`", like
in `associated-types-no-suitable-supertrait*`).
We no longer point at associated items (`ImplObligation`), as they didn't
add any user actionable information, they just added noise.
Address part of #89418.
Rollup of 6 pull requests
Successful merges:
- #89741 (Mark `Arc::from_inner` / `Rc::from_inner` as unsafe)
- #90927 (Fix float ICE)
- #90994 (Fix ICE `#90993`: add missing call to cancel)
- #91018 (Adopt let_else in more places in rustc_mir_build)
- #91022 (Suggest `await` in more situations where infer types are involved)
- #91088 (Revert "require full validity when determining the discriminant of a value")
Failed merges:
r? `@ghost`
`@rustbot` modify labels: rollup
Suggest `await` in more situations where infer types are involved
Currently we use `TyS::same_type` in diagnostics that suggest adding `.await` to opaque future types.
This change makes the suggestion slightly more general, when we're comparing types like `Result<T, E>` and `Result<_, _>` which happens sometimes in places like `match` patterns or `let` statements with partially-elaborated types.
----
Question:
1. Is this change worthwhile? Totally fine if it doesn't make sense adding.
2. Should `same_type_modulo_infer` live in `rustc_infer::infer::error_reporting` or alongside the other method in `rustc_middle::ty::util`?
3. Should we generalize this change? I wanted to change all usages, but I don't want erroneous suggestions when adding `.field_name`...
Mark `Arc::from_inner` / `Rc::from_inner` as unsafe
While it's an internal function, it is easy to create invalid Arc/Rcs to
a dangling pointer with it.
Fixes https://github.com/rust-lang/rust/issues/89740
Be more thorough in using `ItemObligation` and `BindingObligation` when
evaluating obligations so that we can point at trait bounds that
introduced unfulfilled obligations. We no longer incorrectly point at
unrelated trait bounds (`substs-ppaux.verbose.stderr`).
In particular, we now point at trait bounds on method calls.
We no longer point at "obvious" obligation sources (we no longer have a
note pointing at `Trait` saying "required by a bound in `Trait`", like
in `associated-types-no-suitable-supertrait*`).
Address part of #89418.
Windows: Resolve `process::Command` program without using the current directory
Currently `std::process::Command` searches many directories for the executable to run, including the current directory. This has lead to a [CVE for `ripgrep`](https://cve.circl.lu/cve/CVE-2021-3013) but presumably other command line utilities could be similarly vulnerable if they run commands. This was [discussed on the internals forum](https://internals.rust-lang.org/t/std-command-resolve-to-avoid-security-issues-on-windows/14800). Also discussed was [which directories should be searched](https://internals.rust-lang.org/t/windows-where-should-command-new-look-for-executables/15015).
EDIT: This PR originally removed all implicit paths. They've now been added back as laid out in the rest of this comment.
## Old Search Strategy
The old search strategy is [documented here][1]. Additionally Rust adds searching the child's paths (see also #37519). So the full list of paths that were searched was:
1. The directories that are listed in the child's `PATH` environment variable.
2. The directory from which the application loaded.
3. The current directory for the parent process.
4. The 32-bit Windows system directory.
5. The 16-bit Windows system directory.
6. The Windows directory.
7. The directories that are listed in the PATH environment variable.
## New Search Strategy
The new strategy removes the current directory from the searched paths.
1. The directories that are listed in the child's PATH environment variable.
2. The directory from which the application loaded.
3. The 32-bit Windows system directory.
4. The Windows directory.
5. The directories that are listed in the parent's PATH environment variable.
Note that it also removes the 16-bit system directory, mostly because there isn't a function to get it. I do not anticipate this being an issue in modern Windows.
## Impact
Removing the current directory should fix CVE's like the one linked above. However, it's possible some Windows users of affected Rust CLI applications have come to expect the old behaviour.
This change could also affect small Windows-only script-like programs that assumed the current directory would be used. The user would need to use `.\file.exe` instead of the bare application name.
This PR could break tests, especially those that test the exact output of error messages (e.g. Cargo) as this does change the error messages is some cases.
[1]: https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa#parameters
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 = `?
Make scrollbar in the sidebar always visible for visual consistency
Fixes#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:
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``
Fix `non-constant value` ICE (#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#90878
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`.
```
