forego caching for all participants in cycles, apart from root node

[nikomatsakis]

This is a targeted fix for #60010, which uncovered a pretty bad failure of our caching strategy in the face of coinductive cycles. The problem is explained in the comment in the PR on the new field, in_cycle, but I'll reproduce it here:

Starts out as false -- if, during evaluation, we encounter a
cycle, then we will set this flag to true for all participants
in the cycle (apart from the "head" node). These participants
will then forego caching their results. This is not the most
efficient solution, but it addresses #60010. The problem we
are trying to prevent:

• If you have A: AutoTrait requires B: AutoTrait and C: NonAutoTrait
• B: AutoTrait requires A: AutoTrait (coinductive cycle, ok)
• C: NonAutoTrait requires A: AutoTrait (non-coinductive cycle, not ok)

you don't want to cache that B: AutoTrait or A: AutoTrait
is EvaluatedToOk; this is because they were only considered
ok on the premise that if A: AutoTrait held, but we indeed
encountered a problem (later on) with A: AutoTrait. So we currently set a flag on the stack node for B: AutoTrait(as well as the second instance ofA: AutoTrait`) to supress
caching.

This is a simple, targeted fix. The correct fix requires
deeper changes, but would permit more caching: we could
basically defer caching until we have fully evaluated the
tree, and then cache the entire tree at once.

I'm not sure what the impact of this fix will be in terms of existing crates or performance: we were accepting incorrect code before, so there will perhaps be some regressions, and we are now caching less.

As the comment above notes, we could do a lot better than this fix, but that would involve more invasive rewrites. I thought it best to start with something simple.

r? @pnkfelix -- but let's do crater/perf run
cc @arielb1

[pnkfelix]

in what sense are you using the word "correct" here?

Is this use of "correct" meant to imply that there are cases of unsound code that on master we currently incorrectly accept, and a correct fix would reject, but this targeted fix continues to accept?

Or is this use of "correct" just meant to emphasize that this fix is not ideal with respect to compiler efficiency?

[nikomatsakis]

Oh, the latter -- efficiency.

[pnkfelix]

@bors try

[bors]

⌛ Trying commit bd005a2 with merge cf8e5a0...

[pnkfelix]

(the change seems fine, though I would like a small revision to the comment where I noted a somewhat ambiguous of the word "correct")

I'll try to get the crater and perf runs going.

[bors]

☀️ Try build successful - checks-travis
Build commit: cf8e5a0

[nikomatsakis]

@rust-timer build cf8e5a0

[rust-timer]

Success: Queued cf8e5a0 with parent 92b5e20, comparison URL.

[rust-timer]

Finished benchmarking try commit cf8e5a0

[nikomatsakis]

Seems like it is a wash performance wise, which is great.

[nikomatsakis]

@craterbot run mode=check-only

[craterbot]

👌 Experiment pr-60444 created and queued.
🤖 Automatically detected try build cf8e5a0
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-60444 is now running on agent aws-2.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[arielb1]

This looks like an annoying problem, and I agree we shouldn't fix it the "right way" if Chalk is going to be merged soon-ish.

I was afraid for some time that a problem like this might be present, but didn't have time to dig in deeper. I would have preferred to do a Tarjan's-style mechanism using EvaluatedToOk etc. (which would cache cycles as a unit), but this is OK too if doesn't break some edge-case too horribly.

[arielb1]

I also think the performance impact here shouldn't be so horrible: every time this is hit, we do cache at least one trait, so we only evaluate each member of a cycle up to N times, where N is the length of the cycle. This means the performance impact is bounded and we shouldn't have any terrible worst-cases.

Would be nice to write this in a comment. r=me with that comment and a clean crater.

[craterbot]

🎉 Experiment pr-60444 is completed!
📊 6 regressed and 0 fixed (60951 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[nikomatsakis]

Dear crate authors,

This PR fixes a soundness hole in our trait resolution caching scheme. I'm not sure how easily we'll be able to do a warning system for this fix (I can't immediately think of how to do one), so I'm contacting you to let you know that your crates may be affected. If you'd like to try out this build, you should be able to install it using @kennytm's rustup-toolchain-install-master script and installing the toolchain cf8e5a06b289fbcdaf75ca55c057b2cb09fff33b.

There are two crates from crates.io that appear to be affected:

• gluon_repl, cc @Marwes
• rusty-asm, cc @jeremydavis519

I will take a look at those crates and see if I can offer any suggestions.

There are also two crates appear unmaintained (last update was >2 years ago) and a git repository that appears similar (last commit was Sep 2018). Apologies if this is incorrect!

• conch-parser v0.1.0, by @ipetkov
• kailua_langsvr by @lifthrasiir
• the git repo vt6/6shell maintained by @laerling

As these appear unmaintained, I don't plan to investigate them further.

[ipetkov]

Hey @nikomatsakis, thanks for flagging!

conch-parser is maintained in the sense that I intend to add more to it, but without anyone using it/sending bug reports, me trying to work on its sister crate conch-runtime, and, well life happening in general, I haven't updated it in a while 😅

I wasn't able to build this locally as the install script failed due to a missing component:

detecting the channel of the `cf8e5a06b289fbcdaf75ca55c057b2cb09fff33b` toolchain...
downloading ...
error: missing component `rustc` on toolchain `cf8e5a06b289fbcdaf75ca55c057b2cb09fff33b` on channel `nightly` for target `x86_64-apple-darwin`

I peeked at the build logs, it appears due to the compiler getting stuck when attempting to verify Send/Sync bounds on a very deeply recursive and generic type hierarchy. I'm happy to elaborate more on what the crate does so you don't have to do too much spelunking.

The crate offers the functionality to parse shell programs. Each piece of the grammar is represented as a node which can hold generic sub-nodes. The reasoning for this is so that the crate consumer could customize their AST with different/custom nodes, while reusing existing implementations.

The shell grammar is deeply recursive. Basically each command can vary in complexity (compound commands such as case, for, or simple commands like echo foo), but is ultimately made up of a list of shell words (literals, interpolations, etc.). Because each word can contain a command substitution, the AST type is recursive (a Command<W> has a Word<C> type, which gives us Command<Word<Command<...>>).

There are two "top-level" type definitions which seek to unify the entire AST tree concretely which are basically TopLevelCommand(Command<TopLevelWord>) TopLevelWord(Word<TopLevelCommand>). There are a few tests which assert that the appropriate concrete AST trees are Send/Sync as appropriate, which is where the build is appearing to fail.

[ipetkov]

cases where increasing the threshold will fix things

Confirming that increasing the recursion limit to 128 has fixed building my crates. However, I'm seeing a significant perf issue when building. I'll open a separate issue for this

[Zoxc]

I think this PR makes rustc unable to build itself with [rust] parallel-compiler = true. Building libsyntax ends up taking too much time, probably when trying to prove Send or Sync bounds for the AST.