Rollup of 5 pull requests #149923
Closed
Rollup of 5 pull requests #149923
+998
−2,083
Conversation
This file contains hidden or 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
This solves the "can't find the upvar" ICEs that resulted from `maybe_read_scrutinee` being unfit for purpose.
The split between walk_pat and maybe_read_scrutinee has now become redundant. Due to this change, one testcase within the testsuite has become similar enough to a known ICE to also break. I am leaving this as future work, as it requires feature(type_alias_impl_trait)
As per code review, it is preferred to not use derives in tests that aren't about them.
This aims to make each major part responsible for modifying the precision be visible in the logs.
As Nadrieril remarked, the previous comment was misleadingly framed.
See clippy issue 16086 for context
This flag was an artifact of compiletest's old libtest-based test executor, and currently doesn't influence compiletest's output at all.
This commit introduces two new constants to SystemTime: `MIN` and `MAX`, whose value represent the maximum values for the respective data type, depending upon the platform. Technically, this value is already obtainable during runtime with the following algorithm: Use `SystemTime::UNIX_EPOCH` and call `checked_add` (or `checked_sub`) repeatedly with `Duration::new(0, 1)` on it, until it returns None. Mathematically speaking, this algorithm will terminate after a finite amount of steps, yet it is impractical to run it, as it takes practically forever. Besides, this commit also adds a unit test. Concrete implementation depending upon the platform is done in later commits. In the future, the hope of the authors lies within the creation of a `SystemTime::saturating_add` and `SystemTime::saturating_sub`, similar to the functions already present in `std::time::Duration`. However, for those, these constants are crucially required, thereby this should be seen as the initial step towards this direction. Below are platform specifc notes: # Hermit The HermitOS implementation is more or less identitcal to the Unix one. # sgx The implementation uses a `Duration` to store the Unix time, thereby implying `Duration::ZERO` and `Duration::MAX` as the limits. # solid The implementation uses a `time_t` to store the system time within a single value (i.e. no dual secs/nanosecs handling), thereby implying its `::MIN` and `::MAX` values as the respective boundaries. # UEFI UEFI has a weird way to store times, i.e. a very complicated struct. The standard proclaims "1900-01-01T00:00:00+0000" to be the lowest possible value and `MAX_UEFI_TIME` is already present for the upper limit. # Windows Windows is weird. The Win32 documentation makes no statement on a maximum value here. Next to this, there are two conflicting types: `SYSTEMTIME` and `FILETIME`. Rust's Standard Library uses `FILETIME`, whose limit will (probably) be `i64::MAX` packed into two integers. However, `SYSTEMTIME` has a lower-limit. # xous It is similar to sgx in the sense of using a `Duration`. # unsupported Unsupported platforms store a `SystemTime` in a `Duration`, just like sgx, thereby implying `Duration::ZERO` and `Duration::MAX` as the respective limits.
This logic is cargo-specific anyway, so there is no need for it to be a generally-available helper method.
…drieril,traviscross Make closure capturing have consistent and correct behaviour around patterns Reference PR: - rust-lang/reference#1837 This PR has two goals: - firstly, it fixes rust-lang#137467. In order to do so, it needs to introduce a small breaking change surrounding the interaction of closure captures with matching against enums with uninhabited variants. Yes – to fix an ICE! - this also fixes rust-lang#138973, a slightly different case with the same root cause. - likewise, fixes rust-lang#140011. - secondly, it fixes rust-lang#137553, making the closure capturing rules consistent between `let` patterns and `match` patterns. This is new insta-stable behavior. ## Background This change concerns how precise closure captures interact with patterns. As a little known feature, patterns that require inspecting only part of a value will only cause that part of the value to get captured: ```rust fn main() { let mut a = (21, 37); // only captures a.0, writing to a.1 does not invalidate the closure let mut f = || { let (ref mut x, _) = a; *x = 42; }; a.1 = 69; f(); } ``` I was not able to find any discussion of this behavior being introduced, or discussion of its edge-cases, but it is [documented in the Rust reference](https://doc.rust-lang.org/reference/types/closure.html#r-type.closure.capture.precision.wildcard). The currently stable behavior is as follows: - if any pattern contains a binding, the place it binds gets captured (implemented in current `walk_pat`) - patterns in refutable positions (`match`, `if let`, `let ... else`, but not destructuring `let` or destructuring function parameters) get processed as follows (`maybe_read_scrutinee`): - if matching against the pattern will at any point require inspecting a discriminant, or it includes a variable binding not followed by an ``@`-pattern,` capture *the entire scrutinee* by reference You will note that this behavior is quite weird and it's hard to imagine a sensible rationale for at least some of its aspects. It has the following issues: - firstly, it assumes that matching against an irrefutable pattern cannot possibly require inspecting any discriminants. With or-patterns, this isn't true, and it is the cause of the rust-lang#137467 ICE. - secondly, the presence of an ``@`-pattern` doesn't really have any semantics by itself. This is the weird behavior tracked as rust-lang#137553. - thirdly, the behavior is different between pattern-matching done through `let` and pattern-matching done through `match` – which is a superficial syntactic difference This PR aims to address all of the above issues. The new behavior is as follows: - like before, if a pattern contains a binding, the place it binds gets captured as required by the binding mode - if matching against the pattern requires inspecting a disciminant, the place whose discriminant needs to be inspected gets captured by reference "requires inspecting a discriminant" is also used here to mean "compare something with a constant" and other such decisions. For types other than ADTs, the details are not interesting and aren't changing. ## The breaking change During closure capture analysis, matching an `enum` against a constructor is considered to require inspecting a discriminant if the `enum` has more than one variant. Notably, this is the case even if all the other variants happen to be uninhabited. This is motivated by implementation difficulties involved in querying whether types are inhabited before we're done with type inference – without moving mountains to make it happen, you hit this assert: https://github.com/rust-lang/rust/blob/43f0014ef0f242418674f49052ed39b70f73bc1c/compiler/rustc_middle/src/ty/inhabitedness/mod.rs#L121 Now, because the previous implementation did not concern itself with capturing the discriminants for irrefutable patterns at all, this is a breaking change – the following example, adapted from the testsuite, compiles on current stable, but will not compile with this PR: ```rust #[derive(Clone, Copy, PartialEq, Eq, Debug)] enum Void {} pub fn main() { let mut r = Result::<Void, (u32, u32)>::Err((0, 0)); let mut f = || { let Err((ref mut a, _)) = r; *a = 1; }; let mut g = || { //~^ ERROR: cannot borrow `r` as mutable more than once at a time let Err((_, ref mut b)) = r; *b = 2; }; f(); g(); assert_eq!(r, Err((1, 2))); } ``` ## Is the breaking change necessary? One other option would be to double down, and introduce a set of syntactic rules for determining whether a sub-pattern is in an irrefutable position, instead of querying the types and checking how many variants there are. **This would not eliminate the breaking change,** but it would limit it to more contrived examples, such as ```rust let ((true, Err((ref mut a, _, _))) | (false, Err((_, ref mut a, _)))) = x; ``` In this example, the `Err`s would not be considered in an irrefutable position, because they are part of an or-pattern. However, current stable would treat this just like a tuple `(bool, (T, U, _))`. While introducing such a distinction would limit the impact, I would say that the added complexity would not be commensurate with the benefit it introduces. ## The new insta-stable behavior If a pattern in a `match` expression or similar has parts it will never read, this part will not be captured anymore: ```rust fn main() { let mut a = (21, 37); // now only captures a.0, instead of the whole a let mut f = || { match a { (ref mut x, _) => *x = 42, } }; a.1 = 69; f(); } ``` Note that this behavior was pretty much already present, but only accessible with this One Weird Trick™: ```rust fn main() { let mut a = (21, 37); // both stable and this PR only capture a.0, because of the no-op `@-pattern` let mut f = || { match a { (ref mut x @ _, _) => *x = 42, } }; a.1 = 69; f(); } ``` ## The second, more practically-relevant breaking change After running crater, we have discovered that the aforementioned insta-stable behavior, where sometimes closures will now capture less, can also manifest as a breaking change. This is because it is possible that previously a closure would capture an entire struct by-move, and now it'll start capturing only part of it – some by move, and some by reference. This then causes the closure to have a more restrictive lifetime than it did previously. See: - rust-lang#138961 (comment) - EC-labs/cec-assignment#1 - tryandromeda/andromeda#43 ## Implementation notes The PR has two main commits: - "ExprUseVisitor: properly report discriminant reads" makes `walk_pat` perform all necessary capturing. This is the part that fixes rust-lang#137467. - "ExprUseVisitor: remove maybe_read_scrutinee" removes the unnecessary "capture the entire scrutinee" behavior, fixing rust-lang#137553. The new logic stops making the distinction between one particular example that used to work, and another ICE, tracked as rust-lang#119786. As this requires an unstable feature, I am leaving this as future work.
…=ChrisDenton
Add SystemTime::{MIN, MAX}
Accepted ACP: <rust-lang/libs-team#692>
Tracking Issue: <rust-lang#149067>
---
This merge request introduces two new constants to `SystemTime`: `MIN` and `MAX`, whose values represent the maximum values for the respective data type, depending upon the platform.
Technically, this value is already obtainable during runtime with the following algorithm:
Use `SystemTime::UNIX_EPOCH` and call `checked_add` (or `checked_sub`) repeatedly with `Duration::new(0, 1)` on it, until it returns None.
Mathematically speaking, this algorithm will terminate after a finite amount of steps, yet it is impractical to run it, as it takes practically forever.
Besides, this commit also adds a unit test to verify those values represent the respective minimum and maximum, by letting a `checked_add` and `checked_sub` on it fail.
In the future, the hope of the authors lies within the creation of a `SystemTime::saturating_add` and `SystemTime::saturating_sub`, similar to the functions already present in `std::time::Duration`.
However, for those, these constants are crucially required, thereby this should be seen as the initial step towards this direction.
With this change, implementing these functions oneself outside the standard library becomes feasible in a portable manner for the first time.
This feature (and a related saturating version of `checked_{add, sub}` has been requested multiple times over the course of the past few years, most notably:
* rust-lang#100141
* rust-lang#133525
* rust-lang#105762
* rust-lang#71224
* rust-lang#45448
* rust-lang#52555
…=jdonszelmann Remove unused code in `cfg_old` r? `@jdonszelmann` Fixes one of the todos from rust-lang#149865
bootstrap: Don't pass an unused `--color` to compiletest - Follow-up to rust-lang#149850 --- This flag was an artifact of compiletest's old libtest-based test executor, and currently doesn't influence compiletest's output at all. A follow-up commit also inlines `force_coloring_in_ci` into its only remaining caller, and updates its comment.
…-suggestions, r=JonathanBrouwer If there are too many suggestions for malformed attribute, do not suggest them Part of rust-lang#149865. This not only covers for doc attributes but for all attributes, so don't hesitate to tell me if you want it to be limited to only doc attributes (although I think it's actually a nice improvement overall). Also, I picked 3 as the maximum number of suggestions before it becomes noise, but it's very much open to debate. r? `@JonathanBrouwer`
rustbot
added
A-attributes
rustbot
added
O-hermit
Member
Author
|
@bors r+ p=5 rollup=never |
Collaborator
bors
added
S-waiting-on-bors
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Member
Author
|
@bors r- |
bors
added
S-waiting-on-author
rustbot
removed
the
S-waiting-on-author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
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.
Successful merges:
cfg_old#149903 (Remove unused code incfg_old)--colorto compiletest #149911 (bootstrap: Don't pass an unused--colorto compiletest)r? @ghost
@rustbot modify labels: rollup
Create a similar rollup