Use tuple inference for closures #21
Closed
Conversation
This file contains 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
|
As expected, many tests are failing currently. |
|
@roxelo can u rebase this on rust-lang/master? |
arora-aman
requested changes
Jul 22, 2020
roxelo
force-pushed
the
use_tuple_inference_for_closures
branch
from
3c19b06 to
349f23e
Compare
arora-aman
reviewed
Jul 22, 2020
arora-aman
requested changes
Jul 22, 2020
There was a problem hiding this comment.
@roxelo did we see any changes in number of tests passing?
Also can you look in rust/build/x86_64-unknown-linux-gnu/test/ui and figure out what is the general theme for the tests failing?
You should be able to compare the expected results in https://github.com/sexxi-goose/rust/tree/master/src/test/ui
The test folder in build aggregates files with name x.rs and x.err in one folder called x and compares x.err agiainst x.stderr in the src/test
| for upvar_ty in substs.as_generator().upvar_tys() { | ||
| upvar_ty.visit_with(self); | ||
| } | ||
| let ty = self.infcx.shallow_resolve(substs.as_generator().upvar_tuple_ty()); |
There was a problem hiding this comment.
I think we are calling this peice of code a lot now,
can you add something like
upvar_types(cx: &InferCtxt) -> Option<&[Ty]> as niko suggested on zulip?
So if the ty is resolved it return Some(ty), else None
There was a problem hiding this comment.
As discussed, can't work because infer depends on middle
There was a problem hiding this comment.
in that case we could just add a method to the inference context, so that you wrote self.infcx.upvar_tys(substs.as_generator()) (and make sure to note that the method exists from upvar_tuple_ty)
|
After the following two changes. Nine of the previous failing tests now pass: The following tests still fail: Once I have finished my investigation on why the above test fails, I'll add another comment |
sty.rs was changed in this PR files: [ui] ui/async-await/await-keyword/incorrect-syntax-suggestions.rs (in this error, the error type + note at the bottom is the same, the rest includes the actual file names etc) files: files:
|
|
There are also a few other files not included in this overview that actually have a different stderr than expected, but they are mostly unique per file, see build logs |
| // All we care here is if any variable is being captured and not the exact paths, | ||
| // so we check `upvars_mentioned` for root variables being captured. | ||
| ty::FnPtr(fn_ty) | ||
| if self.tcx.upvars_mentioned(closure_def_id_a.expect_local()).is_none() => |
There was a problem hiding this comment.
suggestion from aman:try size ==1 instead of is_none()
There was a problem hiding this comment.
or maybe self.tcx.upvars_mentioned(closure_def_id_a.expect_local().map_or(true, |u| u.is_empty())
arora-aman
reviewed
Jul 28, 2020
| for ty in substs.as_closure().upvar_tys() { | ||
| dtorck_constraint_for_ty(tcx, span, for_ty, depth + 1, ty, constraints)?; | ||
| ty::Closure(_, substs) => { | ||
| if !substs.as_closure().is_valid() { |
There was a problem hiding this comment.
Add comment here
// By the time this code runs, all type variables ought to // By the time this code runs, all type variables ought to
// be fully resolved.
Same as the ty::Infer case
| @@ -241,6 +247,10 @@ fn dtorck_constraint_for_ty<'tcx>( | |||
| // derived from lifetimes attached to the upvars and resume | |||
| // argument, and we *do* incorporate those here. | |||
|
|
|||
| if !substs.as_generator().is_valid() { | |||
There was a problem hiding this comment.
Add comment here
// By the time this code runs, all type variables ought to // By the time this code runs, all type variables ought to
// be fully resolved.
Same as the ty::Infer case
There was a problem hiding this comment.
hmmm well im already deleting my folder so i can use ssh
so that it a later issue
| let ty = self.infcx.shallow_resolve(substs.as_closure().upvar_tuple_ty()); | ||
| if let ty::Infer(ty::TyVar(_)) = ty.kind { | ||
| // Not yet resolved. | ||
| warn!("asked to assemble constituent types of unexpected type: {:?}", t); |
There was a problem hiding this comment.
this should be bug!, like the calls above
| for upvar_ty in substs.as_closure().upvar_tys() { | ||
| // FIXME(eddyb) add the type to `walker` instead of recursing. | ||
| self.compute(upvar_ty.into()); | ||
| let ty = self.infcx.shallow_resolve(substs.as_closure().upvar_tuple_ty()); |
There was a problem hiding this comment.
this can just be self.compute(substs.as_closure().upvar_tuple_ty()), I believe, as inference variables will already be resolved in that recursive call
There was a problem hiding this comment.
@roxelo check https://rust-lang.zulipchat.com/#narrow/stream/189812-t-compiler.2Fwg-rfc-2229/topic/meeting.202020-07-31/near/205631567
There are some details here, that might be helpful
| dtorck_constraint_for_ty(tcx, span, for_ty, depth + 1, ty, constraints)?; | ||
| ty::Closure(_, substs) => { | ||
| if !substs.as_closure().is_valid() { | ||
| return Err(NoSolution); |
There was a problem hiding this comment.
I think that this code can also just recursive on the upvar_tys, because the inference variable should be replaced by this point with a tuple
arora-aman
reviewed
Aug 7, 2020
| for upvar_ty in substs.as_closure().upvar_tys() { | ||
| // FIXME(eddyb) add the type to `walker` instead of recursing. | ||
| self.compute(upvar_ty.into()); | ||
| let ty = self.infcx.shallow_resolve(substs.as_closure().upvar_tuple_ty()); |
There was a problem hiding this comment.
@roxelo check https://rust-lang.zulipchat.com/#narrow/stream/189812-t-compiler.2Fwg-rfc-2229/topic/meeting.202020-07-31/near/205631567
There are some details here, that might be helpful
…oli-obk
Add derive macro for specifying diagnostics using attributes.
Introduces `#[derive(SessionDiagnostic)]`, a derive macro for specifying structs that can be converted to Diagnostics using directions given by attributes on the struct and its fields. Currently, the following attributes have been implemented:
- `#[code = "..."]` -- this sets the Diagnostic's error code, and must be provided on the struct iself (ie, not on a field). Equivalent to calling `code`.
- `#[message = "..."]` -- this sets the Diagnostic's primary error message.
- `#[label = "..."]` -- this must be applied to fields of type `Span`, and is equivalent to `span_label`
- `#[suggestion(..)]` -- this allows a suggestion message to be supplied. This attribute must be applied to a field of type `Span` or `(Span, Applicability)`, and is equivalent to calling `span_suggestion`. Valid arguments are:
- `message = "..."` -- this sets the suggestion message.
- (Optional) `code = "..."` -- this suggests code for the suggestion. Defaults to empty.
`suggestion`also comes with other variants: `#[suggestion_short(..)]`, `#[suggestion_hidden(..)]` and `#[suggestion_verbose(..)]` which all take the same keys.
Within the strings passed to each attribute, fields can be referenced without needing to be passed explicitly into the format string -- eg, `#[error = "{ident} already declared"] ` will set the error message to `format!("{} already declared", &self.ident)`. Any fields on the struct can be referenced in this way.
Additionally, for any of these attributes, Option fields can be used to only optionally apply the decoration -- for example:
```rust
#[derive(SessionDiagnostic)]
#[code = "E0123"]
struct SomeKindOfError {
...
#[suggestion(message = "informative error message")]
opt_sugg: Option<(Span, Applicability)>
...
}
```
will not emit a suggestion if `opt_sugg` is `None`.
We plan on iterating on this macro further; this PR is a start.
Closes rust-lang#61132.
r? `@oli-obk`
Co-authored-by: Peter Todd <pete@petertodd.org>
…-obk Tracing update This does not bring the more significant changes that are coming down the pipeline, but since I've already prepared the PR leaving it up :) See rust-lang#76210 (comment): > Unfortunately, tracing 0.1.20 — which contained the change to reduce the amount of code generated by the tracing macros — had to be yanked, as it broke previously-compiling code for some downstream crates. I've not yet had the chance to fix this and release a new patch. So, in order to benefit from the changes to reduce generated code, you'll need to wait until there's a new version of tracing as well as tracing-attributes and tracing-core.
Do not promote &mut of a non-ZST ever
Since ~pre-1.0~ 1.36, we have accepted code like this:
```rust
static mut TEST: &'static mut [i32] = {
let x = &mut [1,2,3];
x
};
```
I tracked it back to rust-lang#21744, but unfortunately could not find any discussion or RFC that would explain why we thought this was a good idea. And it's not, it breaks all sorts of things -- see rust-lang#75556.
To fix rust-lang#75556, we have to stop promoting non-ZST mutable references no matter the context, which is what this PR does. It's a breaking change.
Notice that this still works, since it does not rely on promotion:
```rust
static mut TEST: &'static mut [i32] = &mut [0,1,2];
```
Cc `@rust-lang/wg-const-eval`
…yn514 Make bootstrap build on beta This is generally a good idea, and will help with being able to build bootstrap without Python over time as it means we can "just" build with cargo +beta build rather than needing the user to set environment variables. This is a minor step, but a necessary one on that road. r? `@jyn514`
…=lcnr Finish off revisions for const generics UI tests. This time it really does fix it. 😅 The only ones left are `min-and-full-same-time.rs`, which doesn't need it, and `array-impls/` which check the feature indirectly. Fixes rust-lang#75279. r? @lcnr
…ary-ops, r=jyn514 Add missing examples on binary core traits r? @jyn514
Rollup of 7 pull requests Successful merges: - rust-lang#76114 (Add saturating methods for `Duration`) - rust-lang#76297 (rustdoc: fix min_const_generics with ty::Param) - rust-lang#76484 (Add MaybeUninit::assume_init_drop.) - rust-lang#76530 (Eliminate mut reference UB in Drop impl for Rc<T>) - rust-lang#76583 (Update `std::os` module documentation.) - rust-lang#76599 (Finish off revisions for const generics UI tests.) - rust-lang#76615 (Add missing examples on binary core traits) Failed merges: r? `@ghost`
…Amanieu Use IOV_MAX and UIO_MAXIOV constants in limit vectored I/O Also updates the libc dependency to 0.2.77 (from 0.2.74) as the constants were only recently added. Related rust-lang#68042, rust-lang#75005 r? `@Amanieu` (also reviewed rust-lang#75005)
update the version of itertools and parking_lot this is to avoid compiling multiple version of the crates in rustc speeding up compilation of rustc an old version of parking_lot is still used in measureme but new version will not be released for some time see [zulip chat](https://rust-lang.zulipchat.com/#narrow/stream/187831-t-compiler.2Fwg-self-profile/topic/new.20release.20of.20measureme)
It is really painful to inspect differences in what was built in CI if things are appearing and disappearing randomly as they hover around the 100ms mark. No matter what we choose there's always going to be quite a bit of variability on CI in timing, so we're going to see things appear and vanish.
Add host triples to CI builders This is a follow-up to rust-lang#76415, which changed how x.py interprets cross-compilation target/host flags. This should fix the known cases, but I'm still working through CI logs before/after that PR to identify if anything else is missing.
…s-unstable-trait-impl, r=lcnr Warn for #[unstable] on trait impls when it has no effect. Earlier today I sent a PR with an `#[unstable]` attribute on a trait `impl`, but was informed that this attribute has no effect there. (comment: rust-lang#76525 (comment), issue: rust-lang#55436) This PR adds a warning for this situation. Trait `impl` blocks with `#[unstable]` where both the type and the trait are stable will result in a warning: ``` warning: An `#[unstable]` annotation here has no effect. See issue rust-lang#55436 <rust-lang#55436> for more information. --> library/std/src/panic.rs:235:1 | 235 | #[unstable(feature = "integer_atomics", issue = "32976")] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ``` --- It detects three problems in the existing code: 1. A few `RefUnwindSafe` implementations for the atomic integer types in `library/std/src/panic.rs`. Example: https://github.com/rust-lang/rust/blob/d92155bf6ae0b7d79fc83cbeeb0cc0c765353471/library/std/src/panic.rs#L235-L236 2. An implementation of `Error` for `LayoutErr` in `library/std/srd/error.rs`: https://github.com/rust-lang/rust/blob/d92155bf6ae0b7d79fc83cbeeb0cc0c765353471/library/std/src/error.rs#L392-L397 3. `From` implementations for `Waker` and `RawWaker` in `library/alloc/src/task.rs`. Example: https://github.com/rust-lang/rust/blob/d92155bf6ae0b7d79fc83cbeeb0cc0c765353471/library/alloc/src/task.rs#L36-L37 Case 3 interesting: It has a bound with an `#[unstable]` trait (`W: Wake`), so appears to have much effect on stable code. It does however break similar blanket implementations. It would also have immediate effect if `Wake` was implemented for any stable type. (Which is not the case right now, but there are no warnings in place to prevent it.) Whether this case is a problem or not is not clear to me. If it isn't, adding a simple `c.visit_generics(..);` to this PR will stop the warning for this case.
This avoids missing a shared build when uplifting LLVM artifacts into the sysroot. We were already producing a shared link anyway, though, so this is not a visible change from the end user's perspective.
…ement, r=matthewjasper Validate built-in attribute placement Closes rust-lang#54584, closes rust-lang#47725, closes rust-lang#54044. I've changed silently ignoring some incorrectly placed attributes to errors. I'm not sure what the policy is since this can theoretically break code (should they be warnings instead? does it warrant a crater run?).
NRVO: Allow occurrences of the return place in var debug info
The non-use occurrence of the return place in var debug info does not
currently inhibit NRVO optimization, but it will fail assertion in
`visit_place` when optimization is performed.
Relax assertion check to allow the return place in var debug info.
This case might be impossible to hit in optimization pipelines as of
now, but can be encountered in customized mir-opt-level=2 pipeline with
copy propagation disabled. For example in:
```rust
pub fn b(s: String) -> String {
a(s)
}
#[inline]
pub fn a(s: String) -> String {
let x = s;
let y = x;
y
}
```
Download LLVM from CI to bootstrap (linux-only to start) This follows rust-lang#76332, adding support for using CI-built LLVM rather than building it locally. This should essentially "just work," but is left off by default in this PR. While we can support downloading LLVM for multiple host triples, this currently only downloads it for the build triple. That said, it should be possible to expand this relatively easily should multiple host triples be desired. Most people shouldn't be adjusting host/target triples though, so this should cover most use cases. Currently this downloads LLVM for the last bors-authored commit in the `git log`. This is a bit suboptimal -- we want the last bors-authored commit that touched the llvm-project submodule in basically all cases. But for now this just adds an extra ~20 MB download when rebasing atop latest master. Once we have a submodule bump landing after rust-lang#76332, we can fix this behavior to reduce downloads further.
Previously, we would throw away the `SyntaxContext` of any span with a dummy location during metadata encoding. This commit makes metadata Span encoding consistent with incr-cache Span encoding - an 'invalid span' tag is only used when it doesn't lose any information.
…enkov Ignore `|` and `+` tokens during proc-macro pretty-print check Fixes rust-lang#76182 This is an alternative to PR rust-lang#76188 These tokens are not preserved in the AST in certain cases (e.g. a leading `|` in a pattern or a trailing `+` in a trait bound). This PR ignores them entirely during the pretty-print/reparse check to avoid spuriously using the re-parsed tokenstream.
…Simulacrum Add a dedicated debug-logging option to config.toml `@Mark-Simulacrum` and I were talking in zulip and we found that turning on debug/trace logging in rustc is fairly confusing, as it effectively depends on debug-assertions and is not documented as such. `@Mark-Simulacrum` mentioned that we should probably have a separate option for logging anyways. this diff adds that, having the option follow debug-assertions (so everyone's existing config.toml should be fine) and if the option is false to test I ran ./x.py test <something> twice, once with `debug-logging = false` and once with `debug-logging = true` and made sure i only saw trace's when it was true
…r=lcnr Properly encode spans with a dummy location and non-root `SyntaxContext` Previously, we would throw away the `SyntaxContext` of any span with a dummy location during metadata encoding. This commit makes metadata Span encoding consistent with incr-cache Span encoding - an 'invalid span' tag is only used when it doesn't lose any information.
…ebank Fixing memory exhaustion when formatting short code suggestion Details can be found in issue rust-lang#76597. This PR replaces substractions with `saturating_sub`'s to avoid usize wrapping leading to memory exhaustion when formatting short suggestion messages.
Use shallow_resolve and is_valid to check if tuple has been resolved Check upvars_mentioned instead of substs to coerce closure to fnptr Co-authored-by: Aman Arora <me@aman-arora.com>
Co-authored-by: Aman Arora <me@aman-arora.com>
|
Changes have been rebased on the latest changes. I will do a more thorough analysis in the next few days, but, at first glance, there seems to be 2 recurring issues: caused by |
|
Moving the conversation to #23 as it is now impossible to keep track of all the changes as a result of the rebase |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Still a work in progress
Relies and blocked on rust-lang#74314
Closes rust-lang/project-rfc-2229#4
This change is