remove intercrate ambiguity hints

[nikomatsakis]

The scheme was causing overflows during coherence checking (e.g. #47139). This is sort of a temporary fix; the proper fix I think involves reworking trait selection in deeper ways.

cc @sgrif -- this should fix diesel

cc @qnighy -- I'd like to discuss you with alternative techniques for achieving the same end. =) Actually, it might be good to put some energy into refactoring traits first.

r? @eddyb

[nikomatsakis]

er, oops -- let me add a test!

[nikomatsakis]

I actually have to figure out how to formulate one. The test I have which reproduces diesel's problem isn't, I think, affected by this change.

[nikomatsakis]

(Also, the problem was sort of order dependent, but I might be able to fix that.)

[eddyb]

r=me

[nikomatsakis]

ok, added a test that fails on beta/nightly and which I believe is representative of the diesel error

[nikomatsakis]

@bors r+ p=1 -- important to get diesel up and going again

[bors]

📌 Commit cb4aa5f has been approved by nikomatsakis

[qnighy]

@nikomatsakis oh bad 😞 Anyway I'm interested in both refactoring trait selection and reintroducing the hints. It may take time for me to remember things around trait selection though...

[qnighy]

For reference: the PR that introduced the hints is #43426

[nikomatsakis]

@bors r- // travis failures, will investigate

[arielb1]

I can't say I like this fix - this regresses from beta because it removes the coherence problem detection feature.

Maybe we should just disable intercrate ambiguity hints "initially", and then call trait selection again with them enabled only if there was a coherence error. That's it, add a report_intercrate_hints: bool flag to the SelectionContext, do the first overlap check with it being false, and if there is overlap, call selection again with the flag set to true.

That way the error detection code will not be called unless there is an actual coherence error, in which case we don't care as much if there is an overflow.

[nikomatsakis]

It is a brute force fix, that's true. Doing it in two passes is a decent idea.

[arielb1]

I'll review with the two-pass thing

[nikomatsakis]

I did the two-pass change but for .. some reason I do not know I am not getting the full range of hints yet. I'll debug in a bit, but pushing intermediate status.

[arielb1]

A squash would be nice.

[arielb1]

@nikomatsakis

The missing errors might be caused by the infcx evaluation_cache caching the evaluations, so that the selection context does not go through evaluating these predicates again, and therefore does not add them to the intercrate ambiguity causes.

The reason this worked before is because intercrate mode only uses the per-infcx evaluation cache:

rust/src/librustc/traits/select.rs

Lines 1249 to 1277 in 6b99ade

	/// Returns true if the global caches can be used.
	/// Do note that if the type itself is not in the
	/// global tcx, the local caches will be used.
	fncan_use_global_caches(&self,param_env: ty::ParamEnv<'tcx>) -> bool{
	// If there are any where-clauses in scope, then we always use
	// a cache local to this particular scope. Otherwise, we
	// switch to a global cache. We used to try and draw
	// finer-grained distinctions, but that led to a serious of
	// annoying and weird bugs like #22019 and #18290. This simple
	// rule seems to be pretty clearly safe and also still retains
	// a very high hit rate (~95% when compiling rustc).
	if !param_env.caller_bounds.is_empty(){
	returnfalse;
	}
	// Avoid using the master cache during coherence and just rely
	// on the local cache. This effectively disables caching
	// during coherence. It is really just a simplification to
	// avoid us having to fear that coherence results "pollute"
	// the master cache. Since coherence executes pretty quickly,
	// it's not worth going to more trouble to increase the
	// hit-rate I don't think.
	ifself.intercrate.is_some(){
	returnfalse;
	}
	// Otherwise, we can use the global cache.
	true
	}

And coherence uses a fresh inference context for each overlap check, starting with an empty evaluation cache:
https://github.com/rust-lang/rust/blob/master/src/librustc/traits/specialize/specialization_graph.rs#L135-L140

To fix this, I think the best way would be to create a fresh inference context for the "error reporting" check.

[nikomatsakis]

@arielb1 ok, cleaned up, take a look -- running full tests now but seems to work

[bors]

⌛ Testing commit 2fc5739 with merge fb4d05e0e9c83a68626168b86e0fd26fc3aa736d...

[bors]

💔 Test failed - status-appveyor

[kennytm]

@bors retry #46903

[bors]

⌛ Testing commit 2fc5739 with merge 20d027e9d7d9d50f1eb257fa38c4a2b7dd0cd4ee...

[bors]

💔 Test failed - status-appveyor

[kennytm]

@bors retry #46903

[sgrif]

I can confirm that this PR fixes Diesel's build

[sgrif]

It doesn't look like bors recognized the retry request

[Mark-Simulacrum]

@bors retry #46903

[bors]

⌛ Testing commit 2fc5739 with merge 60e2baaa825fc13e1d1f764e32348ee6af7d3314...

[bors]

💔 Test failed - status-travis

[nikomatsakis]

https://travis-ci.org/rust-lang/rust/jobs/335887927#L1 ... spurious?

[kennytm]

@bors retry

[bors]

⌛ Testing commit 2fc5739 with merge 56733bc...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 56733bc to master...