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

Merged
merged 34 commits into from
Apr 5, 2022
Merged

Rollup of 5 pull requests #95667

merged 34 commits into from
Apr 5, 2022

Conversation

Dylan-DPC
Copy link
Member

Successful merges:

Failed merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

ben0x539 and others added 30 commits March 23, 2022 13:00
Per https://www.freedesktop.org/software/systemd/man/os-release.html,

> Variable assignment values must be enclosed in double or single quotes
> if they include spaces, semicolons or other special characters outside
> of A–Z, a–z, 0–9. (Assignments that do not include these special
> characters may be enclosed in quotes too, but this is optional.)

So, past `ID=nixos`, let's also check for `ID='nixos'` and `ID="nixos"`.

One of these is necessary between NixOS/nixpkgs#162168 and
NixOS/nixpkgs#164068, but this seems more correct either way.
Eric figured out the fix to this almost 2 years ago, I just didn't read his comment carefully enough at the timme.
The issue was that fake rustc and fake rustdoc were inconsistent about when they passed `--sysroot` to the real compiler.
Change them to consistently only pass it when `--target` is present.
Introduce a `DiagnosticMessage` type that will enable diagnostic
messages to be simple strings or Fluent identifiers.
`DiagnosticMessage` is now used in the implementation of the standard
`DiagnosticBuilder` APIs.

Signed-off-by: David Wood <david.wood@huawei.com>
`MultiSpan` contains labels, which are more complicated with the
introduction of diagnostic translation and will use types from
`rustc_errors` - however, `rustc_errors` depends on `rustc_span` so
`rustc_span` cannot use types like `DiagnosticMessage` without
dependency cycles. Introduce a new `rustc_error_messages` crate that can
contain `DiagnosticMessage` and `MultiSpan`.

Signed-off-by: David Wood <david.wood@huawei.com>
This commit updates the signatures of all diagnostic functions to accept
types that can be converted into a `DiagnosticMessage`. This enables
existing diagnostic calls to continue to work as before and Fluent
identifiers to be provided. The `SessionDiagnostic` derive just
generates normal diagnostic calls, so these APIs had to be modified to
accept Fluent identifiers.

In addition, loading of the "fallback" Fluent bundle, which contains the
built-in English messages, has been implemented.

Each diagnostic now has "arguments" which correspond to variables in the
Fluent messages (necessary to render a Fluent message) but no API for
adding arguments has been added yet. Therefore, diagnostics (that do not
require interpolation) can be converted to use Fluent identifiers and
will be output as before.
Extend loading of Fluent bundles so that bundles can be loaded from the
sysroot based on the language requested by the user, or using a nightly
flag.

Sysroot bundles are loaded from `$sysroot/share/locale/$locale/*.ftl`.

Signed-off-by: David Wood <david.wood@huawei.com>
Various small changes to comments, like wrapping code in backticks,
changing comments to doc comments and adding newlines.

Signed-off-by: David Wood <david.wood@huawei.com>
Move the handling of `Span` or `(Span, Applicability)` types in
`#[suggestion]` attributes to its own function.

Signed-off-by: David Wood <david.wood@huawei.com>
Non-subdiagnostic fields (i.e. those that don't have `#[label]`
attributes or similar and are just additional context) have to be added
as arguments for Fluent messages to refer them. This commit extends the
`SessionDiagnostic` derive to do this for all fields that do not have
attributes and introduces an `IntoDiagnosticArg` trait that is
implemented on all types that can be converted to a argument for Fluent.

Signed-off-by: David Wood <david.wood@huawei.com>
Fluent diagnostics can insert directionality isolation markers around
interpolated variables indicating that there may be a shift from
right-to-left to left-to-right text (or vice-versa). These are disabled
because they are sometimes visible in the error output, but may be worth
investigating in future (for example: if type names are left-to-right
and the surrounding diagnostic messages are right-to-left, then these
might be helpful).

Signed-off-by: David Wood <david.wood@huawei.com>
Small commit adding backticks around types and annotations in the error
messages from the session diagnostic derive.

Signed-off-by: David Wood <david.wood@huawei.com>
Signed-off-by: David Wood <david.wood@huawei.com>
Small commit renaming `#[message]` to `#[primary_span]` as this more
accurately reflects what it does now.

Signed-off-by: David Wood <david.wood@huawei.com>
A call to `set_arg` is generated for every field of a
`SessionDiagnostic` struct without attributes, but not all types support
being an argument, so `#[no_arg]` is introduced to skip these fields.

Signed-off-by: David Wood <david.wood@huawei.com>
In an effort to make it easier to port diagnostics to
`SessionDiagnostic` (for translation) and since translation slugs could
replace error codes, make error codes optional in the
`SessionDiagnostic` derive.

Signed-off-by: David Wood <david.wood@huawei.com>
Extends support for generating `DiagnosticMessage::FluentIdentifier`
messages from `SessionDiagnostic` derive to `#[label]`.

Signed-off-by: David Wood <david.wood@huawei.com>
Signed-off-by: David Wood <david.wood@huawei.com>
Extends support for generating `DiagnosticMessage::FluentIdentifier`
messages from `SessionDiagnostic` derive to `#[suggestion]`.

Signed-off-by: David Wood <david.wood@huawei.com>
If the user requests a diagnostic locale of "en-US" then it doesn't make
sense to try and load that from the `$sysroot` because it is just the
default built-in locale.

Signed-off-by: David Wood <david.wood@huawei.com>
Removes `expected_pluralize` parameter from diagnostic struct which is
no longer necessary as the Fluent message can determine the correct
pluralization.

Signed-off-by: David Wood <david.wood@huawei.com>
Add some links to the Fluent documentation to
`DiagnosticMessage::FluentIdentifier` which explain what a Fluent
message and attribute are.

Signed-off-by: David Wood <david.wood@huawei.com>
Add an option for enabling and disabling Fluent's directionality
isolation markers in output. Disabled by default as these can render in
some terminals and applications.

Signed-off-by: David Wood <david.wood@huawei.com>
`FluentId` is the type alias that is used everywhere else so it should
be used here too so that this doesn't need updated if the alias changes.

Signed-off-by: David Wood <david.wood@huawei.com>
Unfortunately, fluent comes with a lot of dependencies and these need to
be added to the whitelist.

Signed-off-by: David Wood <david.wood@huawei.com>
Conditional on the parallel compiler being enabled, use a different
`IntlLangMemoizer` which supports being sent between threads in
`FluentBundle`.

Signed-off-by: David Wood <david.wood@huawei.com>
bootstrap.py: nixos check in /etc/os-release with quotes

Per https://www.freedesktop.org/software/systemd/man/os-release.html,

> Variable assignment values must be enclosed in double or single quotes
> if they include spaces, semicolons or other special characters outside
> of A–Z, a–z, 0–9. (Assignments that do not include these special
> characters may be enclosed in quotes too, but this is optional.)

So, past `ID=nixos`, let's also check for `ID='nixos'` and `ID="nixos"`.

One of these is necessary between NixOS/nixpkgs#162168 and
NixOS/nixpkgs#164068, but this seems more correct either way.
@bors
Copy link
Contributor

bors commented Apr 5, 2022

📌 Commit bf44a87 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 Apr 5, 2022
@bors
Copy link
Contributor

bors commented Apr 5, 2022

⌛ Testing commit bf44a87 with merge 634770c...

@bors
Copy link
Contributor

bors commented Apr 5, 2022

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

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (634770c): comparison url.

Summary:

  • Primary benchmarks: mixed results
  • Secondary benchmarks: 😿 relevant regressions found
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 17 39 14 0 31
mean2 6.2% 7.9% -0.8% N/A 3.1%
max 39.4% 38.9% -1.6% N/A 39.4%

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

  1. number of relevant changes

  2. the arithmetic mean of the percent change

@nnethercote
Copy link
Contributor

Terrible perf results for doc builds here, and bootstrap time jumped from 675 to 691 seconds. @davidtwco #95512 seems the most likely cause.

@rylev
Copy link
Member

rylev commented Apr 8, 2022

Going to try to run a perf run on the revert of #95512. We'll see if that turns anything up.

@davidtwco
Copy link
Member

davidtwco commented Apr 8, 2022

Unfortunately, I don't think this is too surprising. #95512 adds new dependencies which will all be adding to our bootstrap time. I don't think there's any easy way to avoid these dependencies though, maybe there's some feature flags I didn't notice for disabling some.

@rylev
Copy link
Member

rylev commented Apr 11, 2022

Unfortunately, reverting the PR is going to be difficult (it touches a lot of code and quickly becomes unmergeable). I think it's pretty clear that is the cause of the perf regression. For future note, we should really be avoiding rolling up PRs like this. Still, the regressions are pretty bad. We probably want to prioritize looking into ways to gain back some of the performance lost.

@davidtwco
Copy link
Member

Unfortunately, reverting the PR is going to be difficult (it touches a lot of code and quickly becomes unmergeable). I think it's pretty clear that is the cause of the perf regression. For future note, we should really be avoiding rolling up PRs like this. Still, the regressions are pretty bad. We probably want to prioritize looking into ways to gain back some of the performance lost.

I've got some ideas for how to claw back some of the lost performance - I load the translations unconditionally right now but I could probably delay that until we know we're going to be emitting an error, that's probably where the performance loss comes from.

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 13, 2022
…r=oli-obk

errors: lazily load fallback fluent bundle

Addresses (hopefully) rust-lang#95667 (comment).

Loading the fallback bundle in compilation sessions that won't go on to emit any errors unnecessarily degrades compile time performance, so lazily create the Fluent bundle when it is first required.

r? `@ghost` (just for perf initially)
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Apr 21, 2022
errors: lazily load fallback fluent bundle

Addresses (hopefully) rust-lang/rust#95667 (comment).

Loading the fallback bundle in compilation sessions that won't go on to emit any errors unnecessarily degrades compile time performance, so lazily create the Fluent bundle when it is first required.

r? `@ghost` (just for perf initially)
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. perf-regression Performance regression. 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-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.