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 #97388

Merged
merged 12 commits into from
May 25, 2022
Merged

Rollup of 5 pull requests #97388

merged 12 commits into from
May 25, 2022

Conversation

Dylan-DPC
Copy link
Member

Successful merges:

Failed merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

c410-f3r and others added 12 commits May 22, 2022 07:18
This commit:
- Counts some things that weren't being counted previously, and adds
  an assertion that ensure everything is counted.
- Reorders things so the `eprintln`s order matches the code order.
- Adds percentages, and makes clear that the zero bytes count is orthogonal to
  the other measurements.

Example of the new output:
```
55463779 metadata bytes, of which 18054531 bytes (32.6%) are zero
             preamble:       30 bytes ( 0.0%)
                  dep:        0 bytes ( 0.0%)
          lib feature:    17458 bytes ( 0.0%)
            lang item:      337 bytes ( 0.0%)
      diagnostic item:     1788 bytes ( 0.0%)
           native lib:        0 bytes ( 0.0%)
      foreign modules:     5113 bytes ( 0.0%)
       def-path table:   720180 bytes ( 1.3%)
               traits:      359 bytes ( 0.0%)
                impls:    64624 bytes ( 0.1%)
     incoherent_impls:      130 bytes ( 0.0%)
                  mir: 16137354 bytes (29.1%)
                 item: 23773099 bytes (42.9%)
interpret_alloc_index:      599 bytes ( 0.0%)
      proc-macro-data:        0 bytes ( 0.0%)
               tables: 10081135 bytes (18.2%)
 debugger visualizers:        0 bytes ( 0.0%)
     exported symbols:     5666 bytes ( 0.0%)
              hygiene:  1539390 bytes ( 2.8%)
      def-path hashes:  2752564 bytes ( 5.0%)
           source_map:   363540 bytes ( 0.7%)
                final:      413 bytes ( 0.0%)
```
Modify MIR building to drop repeat expressions with length zero

Closes rust-lang#74836 .

Previously, when a user wrote `[foo; 0]` we used to simply leak `foo`. The goal is to fix that. This PR changes MIR building to make `[foo; 0]` equivalent to `{ drop(foo); [] }` in all cases. Of course, this is a breaking change (see below). A crater run did not indicate any regressions though, and given that the previous behavior was almost definitely not what any user wanted, it seems unlikely that anyone was relying on this.

Note that const generics are in general unaffected by this. Inserting the extra `drop` is only meaningful/necessary when `foo` is of a non-`Copy` type, and array repeat expressions with const generic repetition count must always be `Copy`.

Besides the obvious change to behavior associated with the additional drop, there are three categories of examples where this also changes observable behavior. In all of these cases, the new behavior is consistent with what you would get by replacing `[foo; 0]` with `{ drop(foo); [] }`. As such, none of these give the user new powers to express more things.

**No longer allowed in const (breaking)**:

```rust
const _: [String; 0] = [String::new(); 0];
```

This compiles on stable today. Because we now introduce the drop of `String`, this no longer compiles as `String` may not be dropped in a const context.

**Reduced dataflow (non-breaking)**:

```rust
let mut x: i32 = 0;
let r = &x;
let a = [r; 0];
x = 5;
let _b = a;
```

Borrowck rejects this code on stable because it believes there is dataflow between `a` and `r`, and so the lifetime of `r` has to extend to the last statement. This change removes the dataflow and the above code is allowed to compile.

**More const promotion (non-breaking)**:

```rust
let _v: &'static [String; 0] = &[String::new(); 0];
```

This does not compile today because `String` having drop glue keeps it from being const promoted (despite that drop glue never being executed). After this change, this is allowed to compile.

### Alternatives

A previous attempt at this tried to reduce breakage by various tricks. This is still a possibility, but given that crater showed no regressions it seems unclear why we would want to introduce this complexity.

Disallowing `[foo; 0]` completely is also an option, but obviously this is more of a breaking change. I do not know how often this is actually used though.

r? `@oli-obk`
RFC3239: Implement `cfg(target)` - Part 2

This pull-request implements the compact `cfg(target(..))` part of [RFC 3239](rust-lang#96901).

I recommend reviewing this PR on a per commit basics, because of some moving parts.

cc `@GuillaumeGomez`
r? `@petrochenkov`
[RFC 2011] Library code

CC rust-lang#96496

Based on https://github.com/dtolnay/case-studies/tree/master/autoref-specialization.

Basically creates two traits with the same method name. One trait is generic over any `T` and the other is specialized to any `T: Printable`.

The compiler will then call the corresponding trait method through auto reference.

```rust
fn main() {
    let mut a = Capture::new();
    let mut b = Capture::new();

    (&Wrapper(&1i32)).try_capture(&mut a); // `try_capture` from `TryCapturePrintable`
    (&Wrapper(&vec![1i32])).try_capture(&mut b); // `try_capture` from `TryCaptureGeneric`

    assert_eq!(format!("{:?}", a), "1");
    assert_eq!(format!("{:?}", b), "N/A");
}
```

r? `@scottmcm`
…an-DPC

Minor improvement on else-no-if diagnostic

Don't suggest wrapping in block since it's highly likely to be a missing `if` after `else`. Also rework message a bit (open to further suggestions).

cc: rust-lang#97298 (comment)

r? `@estebank`
…jorn3

Fix metadata stats.

This commit:
- Counts some things that weren't being counted previously, and adds
  an assertion that ensure everything is counted.
- Reorders things so the `eprintln`s order matches the code order.
- Adds percentages, and makes clear that the zero bytes count is orthogonal to
  the other measurements.

Example of the new output:
```
55463779 metadata bytes, of which 18054531 bytes (32.6%) are zero
             preamble:       30 bytes ( 0.0%)
                  dep:        0 bytes ( 0.0%)
          lib feature:    17458 bytes ( 0.0%)
            lang item:      337 bytes ( 0.0%)
      diagnostic item:     1788 bytes ( 0.0%)
           native lib:        0 bytes ( 0.0%)
      foreign modules:     5113 bytes ( 0.0%)
       def-path table:   720180 bytes ( 1.3%)
               traits:      359 bytes ( 0.0%)
                impls:    64624 bytes ( 0.1%)
     incoherent_impls:      130 bytes ( 0.0%)
                  mir: 16137354 bytes (29.1%)
                 item: 23773099 bytes (42.9%)
interpret_alloc_index:      599 bytes ( 0.0%)
      proc-macro-data:        0 bytes ( 0.0%)
               tables: 10081135 bytes (18.2%)
 debugger visualizers:        0 bytes ( 0.0%)
     exported symbols:     5666 bytes ( 0.0%)
              hygiene:  1539390 bytes ( 2.8%)
      def-path hashes:  2752564 bytes ( 5.0%)
           source_map:   363540 bytes ( 0.7%)
                final:      413 bytes ( 0.0%)
```
r? `@bjorn3`
@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. rollup A PR which is a rollup labels May 25, 2022
@Dylan-DPC
Copy link
Member Author

@bors r rollup=never p=5

@bors
Copy link
Contributor

bors commented May 25, 2022

📌 Commit 3c11bf3 has been approved by Dylan-DPC

@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 May 25, 2022
@bors
Copy link
Contributor

bors commented May 25, 2022

⌛ Testing commit 3c11bf3 with merge fe9c64d...

@bors
Copy link
Contributor

bors commented May 25, 2022

☀️ Test successful - checks-actions
Approved by: Dylan-DPC
Pushing fe9c64d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 25, 2022
@bors bors merged commit fe9c64d into rust-lang:master May 25, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 25, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (fe9c64d): comparison url.

Instruction count

  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)
0.2% 0.2% 1
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) 0.2% 0.2% 1

Max RSS (memory usage)

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)
1.8% 1.8% 2
Regressions 😿
(secondary)
2.9% 3.5% 2
Improvements 🎉
(primary)
-3.0% -3.0% 1
Improvements 🎉
(secondary)
-1.9% -2.8% 2
All 😿🎉 (primary) 0.2% -3.0% 3

Cycles

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
2.4% 2.4% 1
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

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

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants