[beta] Revert arg matrix algorithm from check_argument_types

[compiler-errors]

We decided in T-compiler meeting that this is best removed in beta, and reworked to be less ICE-y on nightly: https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bweekly.5D.202022-06-02/near/284755523

r? @jackh726 @estebank

[rust-highfive]

⚠️ Warning ⚠️

• Pull requests are usually filed against the master branch for this repo, but this one is against beta. Please double check that you specified the right target!

[compiler-errors]

I constructed this PR by reverting the body of check_argument_types to what exists on stable, then changing callers to revert the expected_input_tys change from Vec<Ty> to Option<Vec<Ty>>. I did keep the changes to sugg_tuple_wrap_args.

I walked through all other PRs that change fn_ctxt/checks.rs to make sure the function body revert didn't leave out any critical fixes. All other PRs that touched this function body have to do with the arg matrix algorithm or are small things like clippy.

[estebank]

lgtm

After merging, given the size of the revert I'd appreciate an extra crater run of the resulting beta.

[jackh726]

lgtm

After merging, given the size of the revert I'd appreciate an extra crater run of the resulting beta.

This just screams "we shouldn't backport this revert" to me. It scares me to land such a large PR directly on beta without testing (especially sense it seems like this was not just mechanical reverts but actually needed manual intervention). If we're worried enough that we need a crater run just to verify this doesn't break anything, I definitely think it's better to just backport smaller patches. (I mean, the bugs files for the arg mismatch code didn't show up in crater - and that's gotten a 6 week head start before landing on stable).

[compiler-errors]

it seems like this was not just mechanical reverts but actually needed manual intervention

Manual intervention here was extremely minimal. I did basically just copy and paste the old function body from stable (i.e. before the rewrite) and patch up the call-sites that used this function, of which there are only like 3.

the bugs filed for the arg mismatch code didn't show up in crater

Crater is biased to detect bugs in existing (mostly) compiling code. This arg-mismatch algorithm works in the cases that we have correct code, but crater is missing visibility of incorrect code, such as code in the middle of a large refactor or where genuine bugs (e.g. those encountered during the edit-check-edit loop).

But in any case, I'm afraid that we're back at square zero re: the discussion we had back regarding the original backport nomination of #97557... 😵‍💫

[jackh726]

Let's do r? @estebank for final signoff here either way.

[jackh726]

Actual changes seem fine to me (despite my hesitance to backport)

[Mark-Simulacrum]

Can you clarify what the plan on nightly is? In general, we prefer to avoid landing code solely on beta, since that may mean we need to keep doing so in future (and may forget).

It sounds like we're planning a separate patch there?

[compiler-errors]

#97542 and #97557 are proper fixes for the issue, and both can be landed on nightly.

[Mark-Simulacrum]

OK. If a crater is deemed warranted, that can take ~4+ days (and triage time), so we are relatively short on time if we're to do anything meaningful with the results, so we should likely get this merged pretty quickly. Ideally, I would like to see the master-targeted PRs at least r+'d I think before we move ahead here.

Is the primary reason we're not reverting on master that the patch is large and annoying to rebase, so creates pain for folks trying to keep up with master if it's not in-tree?

[jackh726]

It's definitely not worth reverting anything on master. That can just be patched (the patch to fix the smallish ICEs are in the above linked PRs).

[estebank]

@bors r+

[bors]

📌 Commit 337adc6b41b71595953103b52dd9b6dd8d6a71d8 has been approved by estebank

[compiler-errors]

@bors rollup=never

[bors]

⌛ Testing commit 337adc6b41b71595953103b52dd9b6dd8d6a71d8 with merge 1885963843195765f2672941f783ea838acc708d...

[bors]

💔 Test failed - checks-actions

[estebank]

I'm not entirely sure why that test failed 🤔

Just in case it's transient:

@bors retry

[compiler-errors]

@bors r- i need to bless the aarch64-linux-gnu tests

[compiler-errors]

(accidentally removed some .fixed tests by accident :/ oops)

[compiler-errors]

@bors r=estebank p=1

[bors]

📌 Commit 2f1bf16 has been approved by estebank

[bors]

⌛ Testing commit 2f1bf16 with merge 8fd9e5f...

[bors]

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 8fd9e5f to beta...