Remove a large amount of leb128-coded integers
This removes ~41%[^1] of the leb128-encoded integers serialized during libcore compilation by changing enum tags to opportunistically use `u8` where feasible instead of the leb128 coding via `usize`.
This should have effectively zero impact on metadata file sizes, since almost all or all enum tags fit into the 7 bits available in leb128 for single-byte encodings. Perf results indicate this is basically neutral across the board except for an improvement in bootstrap time.
[^1]: More than half the remaining integers still fit into <= 128, so the leb128 coding still makes sense. 32% are zero, and 46% are <= 4.
One consequence is that errors returned by
`maybe_new_parser_from_source_str` now must be consumed, so a bunch of
places that previously ignored those errors now cancel them. (Most of
them explicitly dropped the errors before. I guess that was to indicate
"we are explicitly ignoring these", though I'm not 100% sure.)
Stop mentioning internal lang items in no_std binary errors
When writing a no_std binary, you'll be greeted with nonsensical errors mentioning lang items like eh_personality and start. That's pretty bad because it makes you think that you need to define them somewhere! But oh no, now you're getting the `internal_features` lint telling you that you shouldn't use them! But you need a no_std binary! What now?
No problem! Writing a no_std binary is super easy. Just use panic=abort and supply your own platform specific entrypoint symbol (like `main`) and you're good to go. Would be nice if the compiler told you that, right?
This makes it so that it does do that.
I don't _love_ the new messages yet, but they're decent I think. They can probably be improved, please suggest improvements.
Two different lifetimes are conflated. This doesn't matter right now,
but needs to be fixed for the next commit to work. And the more
descriptive lifetime names make the code easier to read.
But we can't easily switch from `Vec<Diagnostic>` to
`Vec<DiagnosticBuilder<G>>` because there's a mix of errors and warnings
which result in different `G` types. So we must make
`DiagnosticBuilder::into_diagnostic` public, but that's ok, and it will
get more use in subsequent commits.
Each of these has a single call site: `source_file_to_parser`,
`try_file_to_source_file`, `file_to_source_file`. Having them separate
just makes the code longer and harder to read.
Also, `maybe_file_to_stream` doesn't need to be `pub`.
It seems very wrong to have a `-Ztreat-err-as-bug` check here before the
error is even emitted.
Once that's done:
- `into_diagnostic` is infallible, so its return type doesn't need the
`Option`;
- the `&'a DiagCtxt` also isn't needed, because only one callsite uses
it, and it already have access to it via `self.dcx`;
- the comments about dcx disabling buffering are no longer true, this is
unconditional now;
- and the `debug!` seems unnecessary... the comment greatly overstates
its importance because few diagnostics come through `into_diagnostic`,
and `-Ztrack-diagnostics` exists anyway.
Errors in `DiagCtxtInner::emit_diagnostic` are never set to
`Level::Bug`, because the condition never succeeds, because
`self.treat_err_as_bug()` is called *before* the error counts are
incremented.
This commit switches to `self.treat_next_err_as_bug()`, fixing the
problem. This changes the error message output to actually say "internal
compiler error".
This is less elegant in some ways, since we no longer visit a BCB's spans as a
batch, but will make it much easier to add support for other kinds of coverage
mapping regions (e.g. branch regions or gap regions).
Reorder early post-inlining passes.
`RemoveZsts`, `RemoveUnneededDrops` and `UninhabitedEnumBranching` only depend on types, so they should be executed together early after MIR inlining introduces those types.
This does not change the end-result, but this makes the pipeline a bit more consistent.
Rollup of 11 pull requests
Successful merges:
- #115046 (Use version-sorting for all sorting)
- #118915 (Add some comments, add `can_define_opaque_ty` check to `try_normalize_ty_recur`)
- #119006 (Fix is_global special address handling)
- #119637 (Pass LLVM error message back to pass wrapper.)
- #119715 (Exhaustiveness: abort on type error)
- #119763 (Cleanup things in and around `Diagnostic`)
- #119788 (change function name in comments)
- #119790 (Fix all_trait* methods to return all traits available in StableMIR)
- #119803 (Silence some follow-up errors [1/x])
- #119804 (Stabilize mutex_unpoison feature)
- #119832 (Meta: Add project const traits to triagebot config)
r? `@ghost`
`@rustbot` modify labels: rollup
Silence some follow-up errors [1/x]
this is one piece of the requested cleanups from https://github.com/rust-lang/rust/pull/117449
When we use `-> impl SomeTrait<_>` as a return type, we are both using the "infer return type suggestion" code path, and the infer opaque type code path within the same function. That can lead to confusing diagnostics, so silence all opaque type diagnostics in that case.
Cleanup things in and around `Diagnostic`
These changes all arose when I was looking closely at how to simplify `DiagCtxtInner::emit_diagnostic`.
r? `@compiler-errors`
Pass LLVM error message back to pass wrapper.
When rustc fails to load a plugin, it should provide more detailed error message. Before this PR, rustc prints:
```
error: failed to run LLVM passes: Failed to load pass pluginPLUGIN_NAME.so
```
This PR passes LLVM errors back to rustc. After this PR, rustc prints:
```
error: failed to run LLVM passes: Could not load library 'PLUGIN_NAME.so': PLUGIN_NAME.so: undefined symbol: _ZN4llvm9DebugFlagE
```
This PR only contains usability improvements and does not change any functionality. Thus, no new unit test is implemented.
Exhaustiveness: use an `Option` instead of allocating fictitious patterns
In the process of exhaustiveness checking, `Matrix` stores a 2D array of patterns. Those are subpatterns of the patterns we were provided as input, _except_ sometimes we allocate some extra wildcard patterns to fill a hole during specialization.
Morally though, we could store `Option<&'p DeconstructedPat>` in the matrix, where `None` signifies a wildcard. That way we'd only have "real" patterns in the matrix and we wouldn't need the arena to allocate these wildcards. This is what this PR does.
This is part of me splitting up https://github.com/rust-lang/rust/pull/119581 for ease of review.
r? `@compiler-errors`
Of the error levels satisfying `is_error`, `Level::Error` is the only
one that can be a lint, so there's no need to check for it.
(And even if it wasn't, it would make more sense to include
non-`Error`-but-`is_error` lints under `lint_err_count` than under
`err_count`.)
There are four functions that adjust error and warning counts:
- `stash_diagnostic` (increment)
- `steal_diagnostic` (decrement)
- `emit_stashed_diagnostics) (decrement)
- `emit_diagnostic` (increment)
The first three all behave similarly, and only update `warn_count` for
forced warnings. But the last one updates `warn_count` for both forced
and non-forced warnings.
Seems like a bug. How should it be fixed? Well, `warn_count` is only
used in one place: `DiagCtxtInner::drop`, where it's part of the
condition relating to the printing of `good_path_delayed_bugs`. The
intention of that condition seems to be "have any errors been printed?"
so this commit replaces `warn_count` with `has_printed`, which is set
when printing occurs. This is simpler than all the ahead-of-time
incrementing and decrementing.
`is_force_warn` is only possible for diagnostics with `Level::Warning`,
but it is currently stored in `Diagnostic::code`, which every diagnostic
has.
This commit:
- removes the boolean `DiagnosticId::Lint::is_force_warn` field;
- adds a `ForceWarning` variant to `Level`.
Benefits:
- The common `Level::Warning` case now has no arguments, replacing
lots of `Warning(None)` occurrences.
- `rustc_session::lint::Level` and `rustc_errors::Level` are more
similar, both having `ForceWarning` and `Warning`.
When writing a no_std binary, you'll be greeted with nonsensical errors
mentioning lang items like eh_personality and start. That's pretty bad
because it makes you think that you need to define them somewhere! But
oh no, now you're getting the `internal_features` lint telling you that
you shouldn't use them! But you need a no_std binary! What now?
No problem! Writing a no_std binary is super easy. Just use panic=abort
and supply your own platform specific entrypoint symbol (like `main`)
and you're good to go. Would be nice if the compiler told you that,
right?
This makes it so that it does do that.
Diagnostic API fixes
Some improvements to diagnostic APIs: improve some naming, use shortcuts in more places, and add a couple of missing methods.
r? `@compiler-errors`
This removes emit_enum_variant and the emit_usize calls that resulted
in. In libcore this eliminates 17% of leb128, taking us from 8964488 to
7383842 leb128's serialized.
100% of the serialized enums during libcore compilation fit into the
smaller tag, and this eliminates hitting the leb128 code for
coding/decoding when we can statically guarantee that's not required.
30% of all leb128 integers serialized in libcore (12981183 total) come
from the usize's removed here.
Avoid silencing relevant follow-up errors
r? `@matthewjasper`
This PR only adds new errors to tests that are already failing and fixes one ICE.
Several tests were changed to not emit new errors. I believe all of them were faulty tests, and not explicitly testing for the code that had new errors.
This lets us avoid the use of `DiagnosticBuilder::into_diagnostic` in
miri, when then means that `DiagnosticBuilder::into_diagnostic` can
become private, being now only used by `stash` and `buffer`.
In #119606 I added them and used a `_mv` suffix, but that wasn't great.
A `with_` prefix has three different existing uses.
- Constructors, e.g. `Vec::with_capacity`.
- Wrappers that provide an environment to execute some code, e.g.
`with_session_globals`.
- Consuming chaining methods, e.g. `Span::with_{lo,hi,ctxt}`.
The third case is exactly what we want, so this commit changes
`DiagnosticBuilder::foo_mv` to `DiagnosticBuilder::with_foo`.
Thanks to @compiler-errors for the suggestion.
We have `span_delayed_bug` and often pass it a `DUMMY_SP`. This commit
adds `delayed_bug`, which matches pairs like `err`/`span_err` and
`warn`/`span_warn`.
- `struct_foo` + `emit` -> `foo`
- `create_foo` + `emit` -> `emit_foo`
I have made recent commits in other PRs that have removed some of these
shortcuts for combinations with few uses, e.g.
`struct_span_err_with_code`. But for the remaining combinations that
have high levels of use, we might as well use them wherever possible.
Because it takes an error code after the span. This avoids the confusing
overlap with the `DiagCtxt::struct_span_err` method, which doesn't take
an error code.
`~const` trait and projection bounds do not imply their non-const counterparts
This PR removes the hack where we install a non-const trait and projection bound for every `const_trait` and `~const` projection bound we have in the AST. It ends up messing up more things than it fixes, see words below.
Fixes#119718
cc `@fmease` `@fee1-dead` `@oli-obk`
r? fee1-dead or one of y'all i don't care
---
My understanding is that this hack was added to support the following code:
```rust
pub trait Owo<X = <Self as Uwu>::T> {}
#[const_trait]
pub trait Uwu: Owo {}
```
Which is concretely lifted from in the `FromResidual` and `Try` traits. Since within the param-env of `trait Uwu`, we only know that `Self: ~const Uwu` and not `Self: Uwu`, the projection `<Self as Uwu>::T` is not satsifyable.
This causes problems such as #119718, since instantiations of `FnDef` types coming from `const fn` really do **only** implement one of `FnOnce` or `const FnOnce`!
---
In the long-term, I believe that such code should really look something more like:
```rust
#[const_trait]
pub trait Owo<X = <Self as ~const Uwu>::T> {}
#[const_trait]
pub trait Uwu: Owo {}
```
... and that we should introduce some sort of `<T as ~const Foo>::Bar` bound syntax, since due to the fact that `~const` bounds can be present in item bounds, e.g.
```rust
#[const_trait] trait Foo { type Bar: ~const Destruct; }
```
It's easy to see that `<T as Foo>::Bar` and `<T as ~const Foo>::Bar` (or `<T as const Foo>::Bar`) can be distinct types with distinct item bounds!
**Admission**: I know I've said before that I don't like `~const` projection syntax, I do at this point believe they're necessary to fully express bounds and types in a maybe-const world.
GNU/Hurd: unconditionally use inline stack probes
LLVM 11 has been unsupported since 45591408b1, so this doesn't need to be conditional on the LLVM version.
cc `@sthibaul`
Remove `-Zdont-buffer-diagnostics`.
It was added in #54232. It seems like it was aimed at NLL development, which is well in the past. Also, it looks like `-Ztreat-err-as-bug` can be used to achieve the same effect. So it doesn't seem necessary.
r? ``@pnkfelix``
Merge dead bb pruning and unreachable bb deduplication.
Both routines share the same basic structure: iterate on all bbs to identify work, and then renumber bbs.
We can do both at once.
unify query canonicalization mode
Exclude from canonicalization only the static lifetimes that appear in the param env because of #118965 . Any other occurrence can be canonicalized safely AFAICT.
r? `@lcnr`
Support async recursive calls (as long as they have indirection)
Before #101692, we stored coroutine witness types directly inside of the coroutine. That means that a coroutine could not contain itself (as a witness field) without creating a cycle in the type representation of the coroutine, which we detected with the `OpaqueTypeExpander`, which is used to detect cycles when expanding opaque types after that are inferred to contain themselves.
After `-Zdrop-tracking-mir` was stabilized, we no longer store these generator witness fields directly, but instead behind a def-id based query. That means there is no technical obstacle in the compiler preventing coroutines from containing themselves per se, other than the fact that for a coroutine to have a non-infinite layout, it must contain itself wrapped in a layer of allocation indirection (like a `Box`).
This means that it should be valid for this code to work:
```
async fn async_fibonacci(i: u32) -> u32 {
if i == 0 || i == 1 {
i
} else {
Box::pin(async_fibonacci(i - 1)).await
+ Box::pin(async_fibonacci(i - 2)).await
}
}
```
Whereas previously, you'd need to coerce the future to `Pin<Box<dyn Future<Output = ...>>` before `await`ing it, to prevent the async's desugared coroutine from containing itself across as await point.
This PR does two things:
1. Only report an error if an opaque expansion cycle is detected *not* through coroutine witness fields.
* Instead, if we find an opaque cycle through coroutine witness fields, we compute the layout of the coroutine. If that results in a cycle error, we report it as a recursive async fn.
4. Reworks the way we report layout errors having to do with coroutines, to make up for the diagnostic regressions introduced by (1.). We actually do even better now, pointing out the call sites of the recursion!
Adding alignment to the cases to test for specific error messages.
Adding alignment to the list of cases to test for specific error message. Covers `>`, `^` and `<`.
Pinging people who chimed in last time ( https://github.com/rust-lang/rust/pull/106805 ): ``@estebank`` , ``@compiler-errors`` and ``@Nilstrieb``
Add -Zuse-sync-unwind
Currently Rust uses async unwind by default, but async unwind will bring non-negligible size overhead. it would be nice to allow users to choose this.
In addition, async unwind currently prevents LLVM from generate compact unwind for MachO, if one wishes to generate compact unwind for MachO, then also needs this flag.
Remove crossbeam-channel
The standard library's std::sync::mpsc basically is a crossbeam channel, and for the use case here will definitely suffice. This drops this dependency from librustc_driver.
Add helper for when we want to know if an item has a host param
r? ````@fmease```` since you're a good reviewer and no good deed goes unpunished
This helper will see far more usages as built-in traits get constified.
coverage: `llvm-cov` expects column numbers to be bytes, not code points
Normally the compiler emits column numbers as a 1-based number of Unicode code points.
But when we embed coverage mappings for `-Cinstrument-coverage`, those mappings will ultimately be read by the `llvm-cov` tool. That tool assumes that column numbers are 1-based numbers of *bytes*, and relies on that assumption when slicing up source code to apply highlighting (in HTML reports, and in text-based reports with colour).
For the very common case of all-ASCII source code, bytes and code points are the same, so the difference isn't noticeable. But for code that contains non-ASCII characters, emitting column numbers as code points will result in `llvm-cov` slicing strings in the wrong places, producing mangled output or fatal errors.
(See https://github.com/taiki-e/cargo-llvm-cov/issues/275 as an example of what can go wrong.)
Improved support of collapse_debuginfo attribute for macros.
Added walk_chain_collapsed function to consider collapse_debuginfo attribute in parent macros in call chain.
Fixed collapse_debuginfo attribute processing for cranelift (there was if/else branches error swap).
cc https://github.com/rust-lang/rust/issues/100758
It was added in #54232. It seems like it was aimed at NLL development,
which is well in the past. Also, it looks like `-Ztreat-err-as-bug` can
be used to achieve the same effect. So it doesn't seem necessary.
Consuming `emit`
This PR makes `DiagnosticBuilder::emit` consuming, i.e. take `self` instead of `&mut self`. This is good because it doesn't make sense to emit a diagnostic twice.
This requires some changes to `DiagnosticBuilder` method changing -- every existing non-consuming chaining method gets a new consuming partner with a `_mv` suffix -- but permits a host of beneficial follow-up changes: more concise code through more chaining, removal of redundant diagnostic construction API methods, and removal of machinery to track the possibility of a diagnostic being emitted multiple times.
r? `@compiler-errors`
They are no longer used, because
`{DiagCtxt,DiagCtxtInner}::emit_diagnostic` are used everywhere instead.
This also means `track_diagnostic` can become consuming.
Currently it's used for two dynamic checks:
- When a diagnostic is emitted, has it been emitted before?
- When a diagnostic is dropped, has it been emitted/cancelled?
The first check is no longer need, because `emit` is consuming, so it's
impossible to emit a `DiagnosticBuilder` twice. The second check is
still needed.
This commit replaces `DiagnosticBuilderState` with a simpler
`Option<Box<Diagnostic>>`, which is enough for the second check:
functions like `emit` and `cancel` can take the `Diagnostic` and then
`drop` can check that the `Diagnostic` was taken.
The `DiagCtxt` reference from `DiagnosticBuilderState` is now stored as
its own field, removing the need for the `dcx` method.
As well as making the code shorter and simpler, the commit removes:
- One (deprecated) `ErrorGuaranteed::unchecked_claim_error_was_emitted`
call.
- Two `FIXME(eddyb)` comments that are no longer relevant.
- The use of a dummy `Diagnostic` in `into_diagnostic`.
Nice!
The existing uses are replaced in one of three ways.
- In a function that also has calls to `emit`, just rearrange the code
so that exactly one of `delay_as_bug` or `emit` is called on every
path.
- In a function returning a `DiagnosticBuilder`, use
`downgrade_to_delayed_bug`. That's good enough because it will get
emitted later anyway.
- In `unclosed_delim_err`, one set of errors is being replaced with
another set, so just cancel the original errors.
The old code was very hard to understand, involving an
`emit_without_consuming` call *and* a `delay_as_bug_without_consuming`
call.
With slight changes both calls can be avoided. Not creating the error
until later is crucial, as is the early return in the `if recovered`
block.
It took me some time to come up with this reworking -- it went through
intermediate states much further from the original code than this final
version -- and it's isn't obvious at a glance that it is equivalent. But
I think it is, and the unchanged test behaviour is good supporting
evidence.
The commit also changes `check_trailing_angle_brackets` to return
`Option<ErrorGuaranteed>`. This provides a stricter proof that it
emitted an error message than asserting `dcx.has_errors().is_some()`,
which would succeed if any error had previously been emitted anywhere.
It's not clear why this was here, because the created error is returned
as a normal error anyway.
Nor is it clear why removing the call works. The change doesn't affect
any tests; `tests/ui/parser/issues/issue-102182-impl-trait-recover.rs`
looks like the only test that could have been affected.
Instead of taking `seq` as a mutable reference,
`maybe_recover_struct_lit_bad_delims` now consumes `seq` on the recovery
path, and returns `seq` unchanged on the non-recovery path. The commit
also combines an `if` and a `match` to merge two identical paths.
Also change `recover_seq_parse_error` so it receives a `PErr` instead of
a `PResult`, because all the call sites now handle the `Ok`/`Err`
distinction themselves.
In this parsing recovery function, we only need to emit the previously
obtained error message and mark `expr` as erroneous in the case where we
actually recover.
This works for most of its call sites. This is nice, because `emit` very
much makes sense as a consuming operation -- indeed,
`DiagnosticBuilderState` exists to ensure no diagnostic is emitted
twice, but it uses runtime checks.
For the small number of call sites where a consuming emit doesn't work,
the commit adds `DiagnosticBuilder::emit_without_consuming`. (This will
be removed in subsequent commits.)
Likewise, `emit_unless` becomes consuming. And `delay_as_bug` becomes
consuming, while `delay_as_bug_without_consuming` is added (which will
also be removed in subsequent commits.)
All this requires significant changes to `DiagnosticBuilder`'s chaining
methods. Currently `DiagnosticBuilder` method chaining uses a
non-consuming `&mut self -> &mut Self` style, which allows chaining to
be used when the chain ends in `emit()`, like so:
```
struct_err(msg).span(span).emit();
```
But it doesn't work when producing a `DiagnosticBuilder` value,
requiring this:
```
let mut err = self.struct_err(msg);
err.span(span);
err
```
This style of chaining won't work with consuming `emit` though. For
that, we need to use to a `self -> Self` style. That also would allow
`DiagnosticBuilder` production to be chained, e.g.:
```
self.struct_err(msg).span(span)
```
However, removing the `&mut self -> &mut Self` style would require that
individual modifications of a `DiagnosticBuilder` go from this:
```
err.span(span);
```
to this:
```
err = err.span(span);
```
There are *many* such places. I have a high tolerance for tedious
refactorings, but even I gave up after a long time trying to convert
them all.
Instead, this commit has it both ways: the existing `&mut self -> Self`
chaining methods are kept, and new `self -> Self` chaining methods are
added, all of which have a `_mv` suffix (short for "move"). Changes to
the existing `forward!` macro lets this happen with very little
additional boilerplate code. I chose to add the suffix to the new
chaining methods rather than the existing ones, because the number of
changes required is much smaller that way.
This doubled chainging is a bit clumsy, but I think it is worthwhile
because it allows a *lot* of good things to subsequently happen. In this
commit, there are many `mut` qualifiers removed in places where
diagnostics are emitted without being modified. In subsequent commits:
- chaining can be used more, making the code more concise;
- more use of chaining also permits the removal of redundant diagnostic
APIs like `struct_err_with_code`, which can be replaced easily with
`struct_err` + `code_mv`;
- `emit_without_diagnostic` can be removed, which simplifies a lot of
machinery, removing the need for `DiagnosticBuilderState`.
macro_rules: Add an expansion-local cache to span marker
Most tokens in a macro body typically have the same syntax context.
So the cache should usually be hit.
This change can either be combined with https://github.com/rust-lang/rust/pull/119689, or serve as its alternative, depending on perf results.
The standard library's std::sync::mpsc basically is a crossbeam channel,
and for the use case here will definitely suffice. This drops this
dependency from librustc_driver.
Unions are not `PointerLike`
I introduced the `PointerLike` trait to enforce `dyn*` coercions only from types that share the same ABI as a pointer. On top of needing to be scalar, they also should not be unions, since CTFE chokes on scalar reads for union types.
Fixes#119695
Impl trait diagnostic tweaks
1. Tweak some names for `impl Trait` being used in the wrong position
2. Remove two helper functions that are no longer needed since RPITIT is stable, and which causes matches to be a bit obtuse.
3. Split and fix the part where the error notes that it's "only allowed in XX"
Fixes#119629
