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

regression: overflow evaluating, Send/Sync? #61472

Closed
Mark-Simulacrum opened this issue Jun 3, 2019 · 17 comments
Closed

regression: overflow evaluating, Send/Sync? #61472

Mark-Simulacrum opened this issue Jun 3, 2019 · 17 comments
Labels
P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jun 3, 2019

root: fuzzy-pickles - 1 detected crates which regressed due to this; cc @shepmaster -- resolved

root: lang-c - 1 detected crates which regressed due to this; cc @vickenty -- resolved

 * lang-c-0.5.1: [start](https://crater-reports.s3.amazonaws.com/beta-1.36-2/1.35.0/reg/lang-c-0.5.1/log.txt) v. [end](https://crater-reports.s3.amazonaws.com/beta-1.36-2/beta-2019-05-30/reg/lang-c-0.5.1/log.txt); cc @vickenty

root: conch-runtime - 1 detected crates which regressed due to this; cc @ipetkov -- resolved

root: kailua_langsvr - 1 detected crates which regressed due to this; cc @lifthrasiir -- cannot reproduce, appears fixed?

@Mark-Simulacrum Mark-Simulacrum added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Jun 3, 2019
@ipetkov
Copy link
Contributor

ipetkov commented Jun 3, 2019

A fix for conch-runtime has been published to crates.io, but build times will suffer due to #60846

@shepmaster
Copy link
Member

I've been told that this is due to #60444. What's strange about the fuzzy-pickles regression is that it only appears when running rustdoc. Why would running rustdoc have different behavior for evaluating these bounds?

@pnkfelix
Copy link
Member

pnkfelix commented Jun 6, 2019

Maybe it arises from rustdoc's attempt to determine what traits to list as implemented? I don't think rustc itself will attempt to check if a type implements Send/Sync until you actually try to use a type in a context requiring one of those, but rustdoc certainly does check that for every struct/enum/union definition it documents, right?

Update: of course, if that is indeed the case, then that is a not-great failure mode for rustdoc vs rustc...

Update 2: I think I have validated the above hypothesis for the cause. I tried adding the line below to ast.rs in fuzzy-pickles:

const _ASSERT_FILE_IS_SEND: () = { struct S<T: Send>(Option<T>); S::<File>(None); () };

which acts as a way to force rustc to check up front whether a type is Send.

@pnkfelix
Copy link
Member

pnkfelix commented Jun 6, 2019

(BTW adding #![recursion_limit="128"], as suggested by the diagnostic, does at least seem to address the problem for fuzzy-pickles....)

@nikomatsakis
Copy link
Contributor

@shepmaster rustdoc does some different access patterns, especially around traits -- for example, it wants to print out whether or not a type is Send, even if there is no need for that (i.e., the crate doesn't ever do anything that would require Send).

@nikomatsakis
Copy link
Contributor

So: I'm working on addressing the performance hurdles here, but I think that the "correctness side" is correct, and we ought not to revert it. The only real question is whether to issue a warning period. I'm not really sure how to go about doing that -- it may be possible, I'd have to think on it. I'd prefer to focus on improving the performance, however.

@shepmaster
Copy link
Member

shepmaster commented Jun 6, 2019

Creating a sequence of 63 nested structs triggers this problem:

N=63; (echo "struct S0;"; for i in $(seq 0 $N); do echo "struct S$(( $i + 1 ))(S${i});"; done; echo "fn is_send<T: Send>() {}"; echo "fn main() { is_send::<S${N}>(); }") > long.rs
$ rustc +nightly long.rs
error[E0275]: overflow evaluating the requirement `S0: std::marker::Send`
  --> long.rs:67:13
   |
67 | fn main() { is_send::<S63>(); }
   |             ^^^^^^^^^^^^^^
   |
   = help: consider adding a `#![recursion_limit="128"]` attribute to your crate
   = note: required because it appears within the type `S1`
   = note: required because it appears within the type `S2`
   = note: required because it appears within the type `S3`
   = note: required because it appears within the type `S4`
   = note: ... blah blah blah ...
   = note: required because it appears within the type `S61`
   = note: required because it appears within the type `S62`
   = note: required because it appears within the type `S63`
note: required by `is_send`
  --> long.rs:66:1
   |
66 | fn is_send<T: Send>() {}
   | ^^^^^^^^^^^^^^^^^^^^^
Whole program
struct S0;
struct S1(S0);
struct S2(S1);
struct S3(S2);
struct S4(S3);
struct S5(S4);
struct S6(S5);
struct S7(S6);
struct S8(S7);
struct S9(S8);
struct S10(S9);
struct S11(S10);
struct S12(S11);
struct S13(S12);
struct S14(S13);
struct S15(S14);
struct S16(S15);
struct S17(S16);
struct S18(S17);
struct S19(S18);
struct S20(S19);
struct S21(S20);
struct S22(S21);
struct S23(S22);
struct S24(S23);
struct S25(S24);
struct S26(S25);
struct S27(S26);
struct S28(S27);
struct S29(S28);
struct S30(S29);
struct S31(S30);
struct S32(S31);
struct S33(S32);
struct S34(S33);
struct S35(S34);
struct S36(S35);
struct S37(S36);
struct S38(S37);
struct S39(S38);
struct S40(S39);
struct S41(S40);
struct S42(S41);
struct S43(S42);
struct S44(S43);
struct S45(S44);
struct S46(S45);
struct S47(S46);
struct S48(S47);
struct S49(S48);
struct S50(S49);
struct S51(S50);
struct S52(S51);
struct S53(S52);
struct S54(S53);
struct S55(S54);
struct S56(S55);
struct S57(S56);
struct S58(S57);
struct S59(S58);
struct S60(S59);
struct S61(S60);
struct S62(S61);
struct S63(S62);
struct S64(S63);
fn is_send<T: Send>() {}
fn main() { is_send::<S63>(); }

@pnkfelix
Copy link
Member

pnkfelix commented Jun 19, 2019

(This may be an instance of the umbrella issue #61960 .)

@pnkfelix
Copy link
Member

I have confirmed that that the fuzzy-pickles issue was resolved somewhere between nightly-2019-06-16 (0dc9e9c) and nightly-2019-06-17 (4edff84) which is enough to convince me that PR #61754 probably resolved the problem here for fuzzy-pickles.

@pnkfelix
Copy link
Member

@shepmaster By the way, I'm not sure your long.rs demonstrates the regression here. From what I can tell, that code does not involve a cyclic structure that we end up throwing out (and would not have previously thrown out).

did you double-check that your long.rs actually compiles on stable? From what I can tell, the stable rustc (rustc 1.35.0 (3c235d5 2019-05-20)) issues the same overflow error diagnostic.

@pnkfelix
Copy link
Member

pnkfelix commented Jun 19, 2019

I have confirmed that that the conch-runtime issue was resolved somewhere between nightly-2019-06-16 (0dc9e9c) and nightly-2019-06-17 (4edff84) which is enough to convince me that PR #61754 probably resolved the problem here for conch-runtime

@shepmaster
Copy link
Member

did you double-check that your long.rs actually compiles on stable

😞 I did not. I apologize for the noise!

@nikomatsakis
Copy link
Contributor

I have confirmed that the lang-c crate issue was resolved between nightly-2019-06-16 and nightly-2019-06-17.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jun 21, 2019

I am not able to reproduce the problems from kailua_langsvr -- everything seems to build fine when I checkout the particular commit, both with today's nightly and with nightly-2019-06-16. I tried doing: carog check, cargo build, and cargo doc.

@pnkfelix
Copy link
Member

pnkfelix commented Jun 24, 2019

This all seems like evidence that the regression is addressed by #61754.

(So the only question remaining is whether to backport, a question which will be addressed this week; see discussion on the PR itself)

@pnkfelix pnkfelix added the P-high High priority label Jun 24, 2019
@pnkfelix
Copy link
Member

pnkfelix commented Jun 24, 2019

(I'm going to close this as resolved, as the bug is fixed on nightly, and the decision of whether to backport PR #61754 will be made independently of whether this issue is closed or open.)

@mati865

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants