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 9 pull requests #72778

Merged
merged 23 commits into from
May 30, 2020
Merged

Rollup of 9 pull requests #72778

merged 23 commits into from
May 30, 2020

Conversation

RalfJung
Copy link
Member

Successful merges:

Failed merges:

r? @ghost

CAD97 and others added 23 commits May 19, 2020 22:31
This commit adjusts the condition used in the recursion limit check of
the monomorphization collector, from `>` to `>=`.

In rust-lang#67552, the test case had infinite indirect recursion, repeating a
handful of functions (from the perspective of the monomorphization
collector): `rec` -> `identity` -> `Iterator::count` -> `Iterator::fold`
-> `Iterator::next` -> `rec`.

During this process, `resolve_associated_item` was invoked for
`Iterator::fold` (during the construction of an `Instance`), and
ICE'd due to substitutions needing inference. However, previous
iterations of this recursion would have called this function for
`Iterator::fold` - and did! - and succeeded in doing so (trivially
checkable from debug logging, `()` is present where `_` is in the substs
of the failing execution).

The expected outcome of this test case would be a recursion limit error
(which is present when the `identity` fn indirection is removed), and
the recursion depth of `rec` is increasing (other functions finish
collecting their neighbours and thus have their recursion depths reset).

When the ICE occurs, the recursion depth of `rec` is 256 (which matches
the recursion limit), which suggests perhaps that a different part of
the compiler is using a `>=` comparison and returning a different result
on this recursion rather than what it returned in every previous
recursion, thus stopping the monomorphization collector from reporting
an error on the next recursion, where `recursion_depth_of_rec > 256`
would have been true.

With grep and some educated guesses, we can determine that
the recursion limit check at line 818 in
`src/librustc_trait_selection/traits/project.rs` is the other check that
is using a different comparison. Modifying either comparison to be `>` or
`>=` respectively will fix the error, but changing the monomorphization
collector produces the nicer error.

Signed-off-by: David Wood <[email protected]>
This commit introduces a `Limit` type which is used to ensure that all
comparisons against limits within the compiler are consistent (which can
result in ICEs if they aren't).

Signed-off-by: David Wood <[email protected]>
Resolve overflow behavior for RangeFrom

This specifies a documented unspecified implementation detail of `RangeFrom` and makes it consistently implement the specified behavior.

Specifically, `(u8::MAX).next()` is defined to cause an overflow, and resolve that overflow in the same manner as the `Step::forward` implementation.

The inconsistency that has existed is `<RangeFrom as Iterator>::nth`. The existing behavior should be plain to see after rust-lang#69659: the skipping part previously always panicked if it caused an overflow, but the final step (to set up the state for further iteration) has always been debug-checked.

The inconsistency, then, is that `RangeFrom::nth` does not implement the same behavior as the naive (and default) implementation of just calling `next` multiple times. This PR aligns `RangeFrom::nth` to have identical behavior to the naive implementation. It also lines up with the standard behavior of primitive math in Rust everywhere else in the language: debug checked overflow.

cc @Amanieu

---

Followup to rust-lang#69659. Closes rust-lang#25708 (by documenting the panic as intended).

The documentation wording is preliminary and can probably be improved.

This will probably need an FCP, as it changes observable stable behavior.
…nikomatsakis

Fix ICE with explicit late-bound lifetimes

Rather than returning an explicit late-bound lifetime as a generic argument count mismatch (which is not necessarily true), this PR propagates the presence of explicit late-bound lifetimes.

This avoids an ICE that can occur due to the presence of explicit late-bound lifetimes when building generic substitutions by explicitly ignoring them.

r? @varkor

cc @davidtwco (this removes a check you introduced in rust-lang#60892)

Resolves rust-lang#72278
Override Box::<[T]>::clone_from

Avoid dropping and reallocating when cloning to an existing box if the lengths are the same.

It would be nice if this could also be specialized for `Copy` but I don't know how that works since it's not on stable. Will gladly look into it if it's deemed as a good idea.

This is my first PR with code, hope I did everything right 😄
Properly handle InlineAsmOperand::SymFn when collecting monomorphized items

Fixes rust-lang#72484
…r-comparison, r=varkor

mir: adjust conditional in recursion limit check

Fixes rust-lang#67552.

This PR adjusts the condition used in the recursion limit check of
the monomorphization collector, from `>` to `>=`.

In rust-lang#67552, the test case had infinite indirect recursion, repeating a
handful of functions (from the perspective of the monomorphization
collector): `rec` -> `identity` -> `Iterator::count` -> `Iterator::fold`
-> `Iterator::next` -> `rec`.

During this process, `resolve_associated_item` was invoked for
`Iterator::fold` (during the construction of an `Instance`), and
ICE'd due to substitutions needing inference. However, previous
iterations of this recursion would have called this function for
`Iterator::fold` - and did! - and succeeded in doing so (trivially
checkable from debug logging, `()` is present where `_` is in the substs
of the failing execution).

The expected outcome of this test case would be a recursion limit error
(which is present when the `identity` fn indirection is removed), and
the recursion depth of `rec` is increasing (other functions finish
collecting their neighbours and thus have their recursion depths reset).

When the ICE occurs, the recursion depth of `rec` is 256 (which matches
the recursion limit), which suggests perhaps that a different part of
the compiler is using a `>=` comparison and returning a different result
on this recursion rather than what it returned in every previous
recursion, thus stopping the monomorphization collector from reporting
an error on the next recursion, where `recursion_depth_of_rec > 256`
would have been true.

With grep and some educated guesses, we can determine that
the recursion limit check at line 818 in
`src/librustc_trait_selection/traits/project.rs` is the other check that
is using a different comparison. Modifying either comparison to be `>` or
`>=` respectively will fix the error, but changing the monomorphization
collector produces the nicer error.
multiple Return terminators are possible

@ecstatic-morse mentioned in rust-lang#72515 that multiple `Return` terminators are possible. Update the docs accordingly.

Cc @rust-lang/wg-mir-opt
…r=petrochenkov

Only capture tokens for items with outer attributes

Suggested by @petrochenkov in rust-lang#43081 (comment)
Eagerly lower asm sub-expressions to HIR even if there is an error

Fixes rust-lang#72570

r? @oli-obk
@RalfJung
Copy link
Member Author

@rustbot modify labels: +rollup
@bors r+ rollup=never p=9

@bors
Copy link
Contributor

bors commented May 30, 2020

📌 Commit 69310de has been approved by RalfJung

@rustbot rustbot added the rollup A PR which is a rollup label May 30, 2020
@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 30, 2020
@bors
Copy link
Contributor

bors commented May 30, 2020

⌛ Testing commit 69310de with merge 74e8046...

@bors
Copy link
Contributor

bors commented May 30, 2020

☀️ Test successful - checks-azure
Approved by: RalfJung
Pushing 74e8046 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 30, 2020
@nnethercote
Copy link
Contributor

One of these PRs gave a small perf win on unused-warnings, and a tiny win on a few other benchmarks.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.