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

Tiny difference of the diagnostics for generator on some environment #71222

Closed
JohnTitor opened this issue Apr 16, 2020 · 5 comments · Fixed by #71310
Closed

Tiny difference of the diagnostics for generator on some environment #71222

JohnTitor opened this issue Apr 16, 2020 · 5 comments · Fixed by #71310
Labels
A-async-await Area: Async & Await A-coroutines Area: Coroutines A-diagnostics Area: Messages for errors, warnings, and lints A-pretty Area: Pretty printing. A-reproducibility Area: Reproducible / Deterministic builds C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@JohnTitor
Copy link
Member

Follow-up of #71182
Consider this example: #67893 (comment)

It emits the stderr on my local (x86_64-unknown-linux-gnu):

error[E0277]: `std::sync::MutexGuard<'_, ()>` cannot be sent between threads safely
  --> $DIR/issue-67893.rs:9:5
   |
LL | fn g(_: impl Send) {}
   |              ---- required by this bound in `g`
...
LL |     g(issue_67893::run())
   |     ^ `std::sync::MutexGuard<'_, ()>` cannot be sent between threads safely
   | 
  ::: $DIR/auxiliary/issue_67893.rs:7:20
   |
LL | pub async fn run() {
   |                    - within this `impl std::future::Future`
   |
   = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `std::sync::MutexGuard<'_, ()>`
   = note: required because it appears within the type `for<'r, 's, 't0, 't1, 't2, 't3> {std::future::ResumeTy, std::sync::Arc<std::sync::Mutex<()>>, &'r std::sync::Mutex<()>, std::result::Result<std::sync::MutexGuard<'s, ()>, std::sync::PoisonError<std::sync::MutexGuard<'t0, ()>>>, &'t1 std::sync::MutexGuard<'t2, ()>, std::sync::MutexGuard<'t3, ()>, (), impl std::future::Future}`
   = note: required because it appears within the type `[static generator@DefId(15:11 ~ issue_67893[8787]::run[0]::{{closure}}[0]) for<'r, 's, 't0, 't1, 't2, 't3> {std::future::ResumeTy, std::sync::Arc<std::sync::Mutex<()>>, &'r std::sync::Mutex<()>, std::result::Result<std::sync::MutexGuard<'s, ()>, std::sync::PoisonError<std::sync::MutexGuard<'t0, ()>>>, &'t1 std::sync::MutexGuard<'t2, ()>, std::sync::MutexGuard<'t3, ()>, (), impl std::future::Future}]`
   = note: required because it appears within the type `std::future::from_generator::GenFuture<[static generator@DefId(15:11 ~ issue_67893[8787]::run[0]::{{closure}}[0]) for<'r, 's, 't0, 't1, 't2, 't3> {std::future::ResumeTy, std::sync::Arc<std::sync::Mutex<()>>, &'r std::sync::Mutex<()>, std::result::Result<std::sync::MutexGuard<'s, ()>, std::sync::PoisonError<std::sync::MutexGuard<'t0, ()>>>, &'t1 std::sync::MutexGuard<'t2, ()>, std::sync::MutexGuard<'t3, ()>, (), impl std::future::Future}]>`
   = note: required because it appears within the type `impl std::future::Future`
   = note: required because it appears within the type `impl std::future::Future`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.

But on some other environment on CI, e.g. dist-i586-gnu-i586-i686-musl or Linux test-various, it emits it with a tiny difference:

14	   |
15	   = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `std::sync::MutexGuard<'_, ()>`
16	   = note: required because it appears within the type `for<'r, 's, 't0, 't1, 't2, 't3> {std::future::ResumeTy, std::sync::Arc<std::sync::Mutex<()>>, &'r std::sync::Mutex<()>, std::result::Result<std::sync::MutexGuard<'s, ()>, std::sync::PoisonError<std::sync::MutexGuard<'t0, ()>>>, &'t1 std::sync::MutexGuard<'t2, ()>, std::sync::MutexGuard<'t3, ()>, (), impl std::future::Future}`
-	   = note: required because it appears within the type `[static generator@DefId(15:11 ~ issue_67893[8787]::run[0]::{{closure}}[0]) for<'r, 's, 't0, 't1, 't2, 't3> {std::future::ResumeTy, std::sync::Arc<std::sync::Mutex<()>>, &'r std::sync::Mutex<()>, std::result::Result<std::sync::MutexGuard<'s, ()>, std::sync::PoisonError<std::sync::MutexGuard<'t0, ()>>>, &'t1 std::sync::MutexGuard<'t2, ()>, std::sync::MutexGuard<'t3, ()>, (), impl std::future::Future}]`
-	   = note: required because it appears within the type `std::future::from_generator::GenFuture<[static generator@DefId(15:11 ~ issue_67893[8787]::run[0]::{{closure}}[0]) for<'r, 's, 't0, 't1, 't2, 't3> {std::future::ResumeTy, std::sync::Arc<std::sync::Mutex<()>>, &'r std::sync::Mutex<()>, std::result::Result<std::sync::MutexGuard<'s, ()>, std::sync::PoisonError<std::sync::MutexGuard<'t0, ()>>>, &'t1 std::sync::MutexGuard<'t2, ()>, std::sync::MutexGuard<'t3, ()>, (), impl std::future::Future}]>`
+	   = note: required because it appears within the type `[static generator@DefId(14:11 ~ issue_67893[8787]::run[0]::{{closure}}[0]) for<'r, 's, 't0, 't1, 't2, 't3> {std::future::ResumeTy, std::sync::Arc<std::sync::Mutex<()>>, &'r std::sync::Mutex<()>, std::result::Result<std::sync::MutexGuard<'s, ()>, std::sync::PoisonError<std::sync::MutexGuard<'t0, ()>>>, &'t1 std::sync::MutexGuard<'t2, ()>, std::sync::MutexGuard<'t3, ()>, (), impl std::future::Future}]`
+	   = note: required because it appears within the type `std::future::from_generator::GenFuture<[static generator@DefId(14:11 ~ issue_67893[8787]::run[0]::{{closure}}[0]) for<'r, 's, 't0, 't1, 't2, 't3> {std::future::ResumeTy, std::sync::Arc<std::sync::Mutex<()>>, &'r std::sync::Mutex<()>, std::result::Result<std::sync::MutexGuard<'s, ()>, std::sync::PoisonError<std::sync::MutexGuard<'t0, ()>>>, &'t1 std::sync::MutexGuard<'t2, ()>, std::sync::MutexGuard<'t3, ()>, (), impl std::future::Future}]>`
19	   = note: required because it appears within the type `impl std::future::Future`
20	   = note: required because it appears within the type `impl std::future::Future`

It's ideal to have the same diagnostics on all the environment.

See also:

@JohnTitor JohnTitor added A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-coroutines Area: Coroutines labels Apr 16, 2020
@jonas-schievink jonas-schievink added A-reproducibility Area: Reproducible / Deterministic builds A-async-await Area: Async & Await labels Apr 16, 2020
@estebank
Copy link
Contributor

DefIds shouldn't be leaked in user output.

@JohnTitor
Copy link
Member Author

Yeah, other places look like:

required because it appears within the type `[generator@$DIR/issue-68112.rs:48:20: 51:6 {impl std::ops::Generator, ()}]`

Perhaps we emit DefId accidentally in somewhere.

@estebank
Copy link
Contributor

@JohnTitor that makes me think that we probably have some {:?} instead of {} somewhere.

@JohnTitor
Copy link
Member Author

The relevant code is:

ObligationCauseCode::BuiltinDerivedObligation(ref data) => {
let parent_trait_ref = self.resolve_vars_if_possible(&data.parent_trait_ref);
let ty = parent_trait_ref.skip_binder().self_ty();
err.note(&format!("required because it appears within the type `{}`", ty));
obligated_types.push(ty);
let parent_predicate = parent_trait_ref.without_const().to_predicate();
if !self.is_recursive_obligation(obligated_types, &data.parent_code) {
self.note_obligation_cause_code(
err,
&parent_predicate,
&data.parent_code,
obligated_types,
);
}
}

I don't find any difference between here and similar places yet. @estebank thoughts?

@estebank
Copy link
Contributor

estebank commented Apr 19, 2020

@JohnTitor the place to look at is:

// FIXME(eddyb) should use `def_span`.
if let Some(hir_id) = self.tcx().hir().as_local_hir_id(did) {
p!(write("@{:?}", self.tcx().hir().span(hir_id)));
if substs.as_generator().is_valid() {
let upvar_tys = substs.as_generator().upvar_tys();
let mut sep = " ";
for (&var_id, upvar_ty) in self
.tcx()
.upvars(did)
.as_ref()
.iter()
.flat_map(|v| v.keys())
.zip(upvar_tys)
{
p!(write("{}{}:", sep, self.tcx().hir().name(var_id)), print(upvar_ty));
sep = ", ";
}
}
} else {
// Cross-crate closure types should only be
// visible in codegen bug reports, I imagine.
p!(write("@{:?}", did));
if substs.as_generator().is_valid() {
let upvar_tys = substs.as_generator().upvar_tys();
let mut sep = " ";
for (index, upvar_ty) in upvar_tys.enumerate() {
p!(write("{}{}:", sep, index), print(upvar_ty));
sep = ", ";
}
}
}

CC @eddyb

The same applies to closures, but we are likely not displaying them this way anywhere (or we've just gotten lucky).

@estebank estebank added the A-pretty Area: Pretty printing. label Apr 19, 2020
@bors bors closed this as completed in ab44c77 Apr 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await A-coroutines Area: Coroutines A-diagnostics Area: Messages for errors, warnings, and lints A-pretty Area: Pretty printing. A-reproducibility Area: Reproducible / Deterministic builds C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants