Automatic Rustup #3362
Closed
Automatic Rustup #3362
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
change equate for binders to not rely on subtyping
*summary by `@spastorino` and `@lcnr*`
### Context
The following code:
```rust
type One = for<'a> fn(&'a (), &'a ());
type Two = for<'a, 'b> fn(&'a (), &'b ());
mod my_api {
use std::any::Any;
use std::marker::PhantomData;
pub struct Foo<T: 'static> {
a: &'static dyn Any,
_p: PhantomData<*mut T>, // invariant, the type of the `dyn Any`
}
impl<T: 'static> Foo<T> {
pub fn deref(&self) -> &'static T {
match self.a.downcast_ref::<T>() {
None => unsafe { std::hint::unreachable_unchecked() },
Some(a) => a,
}
}
pub fn new(a: T) -> Foo<T> {
Foo::<T> {
a: Box::leak(Box::new(a)),
_p: PhantomData,
}
}
}
}
use my_api::*;
fn main() {
let foo = Foo::<One>::new((|_, _| ()) as One);
foo.deref();
let foo: Foo<Two> = foo;
foo.deref();
}
```
has UB from hitting the `unreachable_unchecked`. This happens because `TypeId::of::<One>()` is not the same as `TypeId::of::<Two>()` despite them being considered the same types by the type checker.
Currently the type checker considers binders to be equal if subtyping succeeds in both directions: `for<'a> T<'a> eq for<'b> U<'b>` holds if `for<'a> exists<'b> T<'b> <: T'<a> AND for<'b> exists<'a> T<'a> <: T<'b>` holds. This results in `for<'a> fn(&'a (), &'a ())` and `for<'a, 'b> fn(&'a (), &'b ())` being equal in the type system.
`TypeId` is computed by looking at the *structure* of a type. Even though these types are semantically equal, they have a different *structure* resulting in them having different `TypeId`. This can break invariants of unsafe code at runtime and is unsound when happening at compile time, e.g. when using const generics.
So as seen in `main`, we can assign a value of type `Foo::<One>` to a binding of type `Foo<Two>` given those are considered the same type but then when we call `deref`, it calls `downcast_ref` that relies on `TypeId` and we would hit the `None` arm as these have different `TypeId`s.
As stated in rust-lang/rust#97156 (comment), this causes the API of existing crates to be unsound.
## What should we do about this
The same type resulting in different `TypeId`s is a significant footgun, breaking a very reasonable assumptions by authors of unsafe code. It will also be unsound by itself once they are usable in generic contexts with const generics.
There are two options going forward here:
- change how the *structure* of a type is computed before relying on it. i.e. continue considering `for<'a> fn(&'a (), &'a ())` and `for<'a, 'b> fn(&'a (), &'b ())` to be equal, but normalize them to a common representation so that their `TypeId` are also the same.
- change how the semantic equality of binders to match the way we compute the structure of types. i.e. `for<'a> fn(&'a (), &'a ())` and `for<'a, 'b> fn(&'a (), &'b ())` still have different `TypeId`s but are now also considered to not be semantically equal.
---
Advantages of the first approach:
- with the second approach some higher ranked types stop being equal, even though they are subtypes of each other
General thoughts:
- changing the approach in the future will be breaking
- going from first to second may break ordinary type checking, as types which were previously equal are now distinct
- going from second to first may break coherence, because previously disjoint impls overlap as the used types are now equal
- both of these are quite unlikely. This PR did not result in any crater failures, so this should not matter too much
Advantages of the second approach:
- the soundness of the first approach requires more non-local reasoning. We have to make sure that changes to subtyping do not cause the representative computation to diverge from semantic equality
- e.g. we intend to consider higher ranked implied bounds when subtyping to [fix] rust-lang/rust#25860, I don't know how this will interact and don't feel confident making any prediction here.
- computing a representative type is non-trivial and soundness critical, therefore adding complexity to the "core type system"
---
This PR goes with the second approach. A crater run did not result in any regressions. I am personally very hesitant about trying the first approach due to the above reasons. It feels like there are more unknowns when going that route.
### Changing the way we equate binders
Relating bound variables from different depths already results in a universe error in equate. We therefore only need to make sure that there is 1-to-1 correspondence between bound variables when relating binders. This results in concrete types being structurally equal after anonymizing their bound variables.
We implement this by instantiating one of the binder with placeholders and the other with inference variables and then equating the instantiated types. We do so in both directions.
More formally, we change the typing rules as follows:
```
for<'r0, .., 'rn> exists<'l0, .., 'ln> LHS<'l0, .., 'ln> <: RHS<'r0, .., 'rn>
for<'l0, .., 'ln> exists<'r0, .., 'rn> RHS<'r0, .., 'rn> <: LHS<'l0, .., 'ln>
--------------------------------------------------------------------------
for<'l0, .., 'ln> LHS<'l0, .., 'ln> eq for<'r0, .., 'rn> RHS<'r0, .., 'rn>
```
to
```
for<'r0, .., 'rn> exists<'l0, .., 'ln> LHS<'l0, .., 'ln> eq RHS<'r0, .., 'rn>
for<'l0, .., 'ln> exists<'r0, .., 'rn> RHS<'r0, .., 'rn> eq LHS<'l0, .., 'ln>
--------------------------------------------------------------------------
for<'l0, .., 'ln> LHS<'l0, .., 'ln> eq for<'r0, .., 'rn> RHS<'r0, .., 'rn>
```
---
Fixes #97156
r? `@lcnr`
Delete architecture-specific memchr code in std::sys Currently all architecture-specific memchr code is only used in `std::io`. Most of the actual `memchr` capacity exposed to the user through the slice API is instead implemented in `core::slice::memchr`. Hence this commit deletes `memchr` from `std::sys[_common]` and replace calls to it by calls to `core::slice::memchr` functions. This deletes `(r)memchr` from the list of symbols linked to libc. The interest of putting architecture specific code back in core is linked to the discussion to be had in #113654
…r-errors Add stubs in IR and ABI for `f16` and `f128` This is the very first step toward the changes in rust-lang/rust#114607 and the [`f16` and `f128` RFC](https://rust-lang.github.io/rfcs/3453-f16-and-f128.html). It adds the types to `rustc_type_ir::FloatTy` and `rustc_abi::Primitive`, and just propagates those out as `unimplemented!` stubs where necessary. These types do not parse yet so there is no feature gate, and it should be okay to use `unimplemented!`. The next steps will probably be AST support with parsing and the feature gate. r? `@compiler-errors` cc `@Nilstrieb` suggested breaking the PR up in rust-lang/rust#120645 (comment)
Combine `Sub` and `Equate` Combine `Sub` and `Equate` into a new relation called `TypeRelating` (that name sounds familiar...) Tracks the difference between `Sub` and `Equate` via `ambient_variance: ty::Variance` much like the `NllTypeRelating` relation, but implemented slightly jankier because it's a more general purpose relation. r? lcnr
…ister test: enable `unpacked-lto` tests This enables the correct `unpacked-lto` tests. Not sure whether `.o` should be removed. They are bitcode for linker-plugin-lto, though there might be some `.o` for `#[no_builtins]`?
Introduce `run-make` V2 infrastructure, a `run_make_support` library and port over 2 tests as example ## Preface See [issue #40713: Switch run-make tests from Makefiles to rust](rust-lang/rust#40713) for more context. ## Basic Description of `run-make` V2 `run-make` V2 aims to eliminate the dependency on `make` and `Makefile`s for building `run-make`-style tests. Makefiles are replaced by *recipes* (`rmake.rs`). The current implementation runs `run-make` V2 tests in 3 steps: 1. We build the support library `run_make_support` which the `rmake.rs` recipes depend on as a tool lib. 2. We build the recipe `rmake.rs` and link in the support library. 3. We run the recipe to build and run the tests. `rmake.rs` is basically a replacement for `Makefile`, and allows running arbitrary Rust code. The support library is built using cargo, and so can depend on external crates if desired. The infrastructure implemented by this PR is very barebones, and is the minimally required infrastructure needed to build, run and pass the two example `run-make` tests ported over to the new infrastructure. ### Example `run-make` V2 test ```rs // ignore-tidy-linelength extern crate run_make_support; use std::path::PathBuf; use run_make_support::{aux_build, rustc}; fn main() { aux_build() .arg("--emit=metadata") .arg("stable.rs") .run(); let mut stable_path = PathBuf::from(env!("TMPDIR")); stable_path.push("libstable.rmeta"); let output = rustc() .arg("--emit=metadata") .arg("--extern") .arg(&format!("stable={}", &stable_path.to_string_lossy())) .arg("main.rs") .run(); let stderr = String::from_utf8_lossy(&output.stderr); let version = include_str!(concat!(env!("S"), "/src/version")); let expected_string = format!("stable since {}", version.trim()); assert!(stderr.contains(&expected_string)); } ``` ## Follow Up Work - [ ] Adjust rustc-dev-guide docs
Detect more cases of `=` to `:` typo
When a `Local` is fully parsed, but not followed by a `;`, keep the `:` span arround and mention it. If the type could continue being parsed as an expression, suggest replacing the `:` with a `=`.
```
error: expected one of `!`, `+`, `->`, `::`, `;`, or `=`, found `.`
--> file.rs:2:32
|
2 | let _: std::env::temp_dir().join("foo");
| - ^ expected one of `!`, `+`, `->`, `::`, `;`, or `=`
| |
| while parsing the type for `_`
| help: use `=` if you meant to assign
```
Fix #119665.
Cleanup windows `abort_internal` As the comments on the functions say, we define abort in both in panic_abort and in libstd. This PR makes the two implementation (mostly) the same. Additionally it: * uses `options(noreturn)` on the asm instead of using `core::intrinsics::unreachable`. * removed unnecessary allow lints * added `FAST_FAIL_FATAL_APP_EXIT` to our generated Windows API bindings instead of defining it manually (std only)
Miri subtree update r? `@ghost`
Update outdated LLVM comment The first path no longer exists, but the second does.
Always generate GEP i8 / ptradd for struct offsets This implements #98615, and goes a bit further to remove `struct_gep` entirely. Upstream LLVM is in the beginning stages of [migrating to `ptradd`](https://discourse.llvm.org/t/rfc-replacing-getelementptr-with-ptradd/68699). LLVM 19 will [canonicalize](llvm/llvm-project#68882) all constant-offset GEPs to i8, which has roughly the same effect as this change. Fixes #121719. Split out from #121577. r? `@nikic`
Promote OpenHarmony targets to tier 2 MCP: rust-lang/compiler-team#719
… r=compiler-errors Fix misleading message in struct repr alignment and packed Fixes #121425 By the way, fix the spans for the argument in the second commit.
…eywiser Add a new `wasm32-wasip1` target to rustc This commit adds a new target called `wasm32-wasip1` to rustc. This new target is explained in these two MCPs: * rust-lang/compiler-team#607 * rust-lang/compiler-team#695 In short, the previous `wasm32-wasi` target is going to be renamed to `wasm32-wasip1` to better live alongside the [new `wasm32-wasip2` target](rust-lang/rust#119616). This new target is added alongside the `wasm32-wasi` target and has the exact same definition as the previous target. This PR is effectively a rename of `wasm32-wasi` to `wasm32-wasip1`. Note, however, that as explained in rust-lang/compiler-team#695 the previous `wasm32-wasi` target is not being removed at this time. This change will reach stable Rust before even a warning about the rename will be printed. At this time this change is just the start where a new target is introduced and users can start migrating if they support only Nightly for example.
Diagnostic renaming 2 A sequel to #121489. r? `@davidtwco`
perf: improve write_fmt to handle simple strings In case format string has no arguments, simplify its implementation with a direct call to `output.write_str(value)`. This builds on `@dtolnay` original [suggestion](serde-rs/serde#2697 (comment)). This does not change any expectations because the original `fn write()` implementation calls `write_str` for parts of the format string. ```rust write!(f, "text") -> f.write_str("text") ``` ```diff /// [`write!`]: crate::write! +#[inline] #[stable(feature = "rust1", since = "1.0.0")] pub fn write(output: &mut dyn Write, args: Arguments<'_>) -> Result { + if let Some(s) = args.as_str() { output.write_str(s) } else { write_internal(output, args) } +} + +/// Actual implementation of the [`write`], but without the simple string optimization. +fn write_internal(output: &mut dyn Write, args: Arguments<'_>) -> Result { let mut formatter = Formatter::new(output); let mut idx = 0; ``` * Hopefully it will improve the simple case for the rust-lang/rust#99012 * Another related (original?) issues #10761 * Previous similar attempt to fix it by by `@Kobzol` #100700 CC: `@m-ou-se` as probably the biggest expert in everything `format!`
Add ASCII fast-path for `char::is_grapheme_extended` I discovered that `impl Debug for str` is quite slow because it ends up doing a `unicode_data::grapheme_extend::lookup` for each char, which ends up doing a binary search. This introduces a fast-path for ASCII chars which do not have this property. The `lookup` is thus completely gone from profiles. --- As a followup, maybe it’s worth implementing this fast path directly in `unicode_data` so that it can check for the lower bound directly before going to a potentially expensive binary search.
…zkan tidy: split dots in filename not the entire path when checking for stray stdout/stderr files I committed a path crime by splitting the entire path on `.`, when I meant to split on the filename. This means that any parent folders which contain `.` will cause tidy failure. Added a regression test so that doesn't happen again. ### Follow-up - [ ] Adjust rustc-dev-guide to document assert on test name not containing dots. rust-lang/rustc-dev-guide#1927 Fixes #121986.
net: Don't use checked arithmetic when parsing numbers with known max digits Add a branch to `Parser::read_number` that determines whether checked or regular arithmetic is used. - If `max_digits.is_some()`, then we know we are parsing a `u8` or `u16` because `read_number` is only called with `Some(3)` or `Some(4)`. Both types fit within a `u32` without risk of overflow. Thus, we can use plain arithmetic to avoid extra instructions from `checked_mul` and `checked_add`. Add benches for `IpAddr`, `Ipv4Addr`, `Ipv6Addr`, `SocketAddr`, `SocketAddrV4`, and `SocketAddrV6` parsing
|
Still blocked on rust-lang/rust#121889 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.