Avoid infinite loop in function arguments checking #100502

chenyukang · 2022-08-13T16:57:05Z

Fixes #100478
Fixes #101097

rust-highfive · 2022-08-13T16:57:08Z

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

chenyukang · 2022-08-13T17:04:59Z

@jackh726
My fix maybe not the best solution, feel free give other solutions.

If you would like to have a debug, here is a log for you to inspect:
check.log

compiler-errors · 2022-08-13T18:02:21Z

Thanks for finding this @chenyukang.

Also remember to use "fixes" instead of "fixing" :)

chenyukang · 2022-08-13T18:04:53Z

Thanks for finding this @chenyukang.

Also remember to use "fixes" instead of "fixing" :)

Aha, seems it's my habit😁
I found we also can link issues with Fixing, but fixing seems not work.

jackh726 · 2022-08-13T19:09:28Z

@chenyukang thanks for the PR. This indeed doesn't seem like the correct fix. I think this masks a different problem. There's an unstated assertion that if find_issue returns None, then we can eliminate at least some provided/expected args. The fact that this isn't the case seems like there is a discrepancy between find_issue and eliminate_satisfied.

Is the log that you provided for the test you added? The test seems to have 11 args and the log has 15/16?

jackh726 · 2022-08-13T19:24:17Z

Just looking at the log and comparing it to the what would happen. There's actually a couple things we could change.

But let's look going backwards at "most straightforward" fix.
Last cycle:

x|  0  1  2  3  4  5  -  -  -  -  -  - 
0|  0  1  0  0  0  0 
1|  0  0  1  0  0  0 
2|  0  0  0  1  0  0 
3|  0  0  0  0  1  0 
4|  0  0  0  0  0  1 
5|  1  0  1  0  0  0

We should detect this as a permutation, I think. Or that column 1 is extra, followed by row 5 extra. This suggests there's a bug in our permutation detection.

Actually though, all the back towards the beginning, first call to find_issue:

x|  0  1  2  3  4  5  6  7  8  9  -  -  -  -  -  -  -  -  -  - 
0|  0  1  0  0  0  0  0  0  0  0 
1|  0  0  1  0  0  0  0  0  0  0 
2|  0  0  0  1  0  0  0  0  0  0 
3|  0  0  0  0  1  0  0  0  0  0 
4|  0  0  0  0  0  1  0  0  0  0 
5|  1  0  1  0  0  0  1  0  0  0 
6|  0  0  0  0  0  0  0  1  0  0 
7|  0  0  0  0  0  0  0  0  1  0 
8|  0  0  0  0  0  0  0  0  0  1

It would be nice to identify that column 1 is "extra", rather than that row 9 is "missing".

chenyukang · 2022-08-14T00:14:44Z

It would be nice to identify that column 1 is "extra", rather than that row 9 is "missing".

I guess what you mean should be: column 0 is "missing", rather than that col 9 is "missing" ?since we lack an argument for a input.

Yes, the first iteration is not good.
I also tried this idea, actually my first try is fix this line https://github.com/rust-lang/rust/blob/master/compiler/rustc_typeck/src/check/fn_ctxt/arg_matrix.rs#L136

 // If we eliminate the last row, any left-over inputs are considered missing
 if i >= mat.len() {
    return Some(Issue::Missing(i));   // Fix to Issue::Missing(0)
 }

But this fix will broke another testcase, for function call in testcase https://github.com/rust-lang/rust/blob/master/src/test/ui/argument-suggestions/missing_arguments.rs#L30, this fix print the wrong type for missing arguments:

--- a/src/test/ui/argument-suggestions/missing_arguments.stderr
+++ b/src/test/ui/argument-suggestions/missing_arguments.stderr
@@ -210,10 +210,7 @@ error[E0061]: this function takes 3 arguments but 1 argument was supplied
   --> $DIR/missing_arguments.rs:30:3
    |
 LL |   three_diff(          1.0          );
-   |   ^^^^^^^^^^-------------------------
-   |             |          |
-   |             |          an argument of type `i32` is missing
-   |             an argument of type `&str` is missing
+   |   ^^^^^^^^^^------------------------- two arguments of type `i32` and `f32` are missing
    |
 note: function defined here
   --> $DIR/missing_arguments.rs:5:4
@@ -222,8 +219,8 @@ LL | fn three_diff(_a: i32, _b: f32, _c: &str) {}
    |    ^^^^^^^^^^ -------  -------  --------
 help: provide the arguments
    |
-LL |   three_diff(/* i32 */, 1.0, /* &str */);
-   |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+LL |   three_diff(/* i32 */, /* f32 */, /* &str */);
+   |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

chenyukang · 2022-08-14T00:18:25Z

@chenyukang thanks for the PR. This indeed doesn't seem like the correct fix. I think this masks a different problem. There's an unstated assertion that if find_issue returns None, then we can eliminate at least some provided/expected args. The fact that this isn't the case seems like there is a discrepancy between find_issue and eliminate_satisfied.

Is the log that you provided for the test you added? The test seems to have 11 args and the log has 15/16?

oh, sorry, I minilized the test case later, droping some several extra parameters at the beginning.

chenyukang · 2022-08-14T05:52:01Z

@jackh726
The root cause is by the special Arc::default(), which could satisfy multiple arguments, and this will introduce we can not eliminate the col 0 as Missing.

This part of checking will not find out the missing 0.

x| 0 1 2 3 4 5 6 7 8 9 - - - - - - - - - -
0| 0 1 0 0 0 0 0 0 0 0
1| 0 0 1 0 0 0 0 0 0 0
2| 0 0 0 1 0 0 0 0 0 0
3| 0 0 0 0 1 0 0 0 0 0
4| 0 0 0 0 0 1 0 0 0 0
5| 1 0 1 0 0 0 1 0 0 0
6| 0 0 0 0 0 0 0 1 0 0
7| 0 0 0 0 0 0 0 0 1 0
8| 0 0 0 0 0 0 0 0 0 1

I worked out another solution, next_unmatched_idx used to tracking the next unmatched indexes, when broke from the for loop, it's a suitable index to return, this will also return the right index for the case three_diff I mentioned above.

By the way, I fixed some comments which didn't get right between the arguments and input. I found in code, input means the user's input and arguments means the function's expected parameter list.

This is how the compatibility_matrix is constructed:

let compatibility_matrix = (0..provided_count)
            .map(|i| {
                (0..expected_input_count)
                    .map(|j| is_compatible(ProvidedIdx::from_usize(i), ExpectedIdx::from_usize(j)))
                    .collect()
            })
            .collect();
        ArgMatrix {
            provided_indices: (0..provided_count).map(ProvidedIdx::from_usize).collect(),
            expected_indices: (0..expected_input_count).map(ExpectedIdx::from_usize).collect(),
            compatibility_matrix,
        }

...

let ai = &self.expected_indices;
let ii = &self.provided_indices;

This is not match with the comments in b6c87c5:

It helps to understand this argument by paying special attention to terminology: "inputs" refers to the inputs being expected by the function, and "arguments" refers to what has been provided at the call site.
...

Construct a boolean grid, with a row for each argument, and a column for each input. The cell [i, j] is true if the i'th argument could satisfy the j'th input.

Anyway, I think we'd better keep the naming choice in the code now.

lcnr · 2022-08-17T08:10:11Z

r? @jackh726 it seems like you're already familiar with this code

chenyukang · 2022-08-30T02:12:37Z

@jackh726
#101097, there is another case will cause infinite loop in arguments checking, the algorithm didn't cover the case that there are multiple inputs compatiable with each other.

Debugging code FYI: https://github.com/chenyukang/rust/pull/1/files

struct A;
struct B;
struct C;
struct D;

fn f(
    a1: A,
    a2: A,
    b1: B,
    b2: B,
    c1: C,
    c2: C,
) {}

fn main() {
    f(C, C, A, A, B, B);
}

The debug log is:

ai: [ExpectedIdx(0), ExpectedIdx(1), ExpectedIdx(2), ExpectedIdx(3), ExpectedIdx(4), ExpectedIdx(5)]
ii: [ProvidedIdx(0), ProvidedIdx(1), ProvidedIdx(2), ProvidedIdx(3), ProvidedIdx(4), ProvidedIdx(5)]
================== find_issue: ==================
x|  0  1  2  3  4  5
-|  -  -  -  -  -  -
0|  0  0  0  0  1  1
1|  0  0  0  0  1  1
2|  1  1  0  0  0  0
3|  1  1  0  0  0  0
4|  0  0  1  1  0  0
5|  0  0  1  1  0  0
i: 0, is_input: true, is_arg: true, useless: false, unsatisfiable: false
i: 1, is_input: true, is_arg: true, useless: false, unsatisfiable: false
i: 2, is_input: true, is_arg: true, useless: false, unsatisfiable: false
i: 3, is_input: true, is_arg: true, useless: false, unsatisfiable: false
i: 4, is_input: true, is_arg: true, useless: false, unsatisfiable: false
i: 5, is_input: true, is_arg: true, useless: false, unsatisfiable: false
loop i: 0 j: 0 compat: [4, 5]
permutation_found: false
now permutation: [Some(None), None, None, None, None, None]
loop i: 1 j: 1 compat: [4, 5]
permutation_found: false
now permutation: [Some(None), Some(None), None, None, None, None]
loop i: 2 j: 2 compat: [0, 1]
permutation_found: false
now permutation: [Some(None), Some(None), Some(None), None, None, None]
loop i: 3 j: 3 compat: [0, 1]
permutation_found: false
now permutation: [Some(None), Some(None), Some(None), Some(None), None, None]
loop i: 4 j: 4 compat: [2, 3]
permutation_found: false
now permutation: [Some(None), Some(None), Some(None), Some(None), Some(None), None]
loop i: 5 j: 5 compat: [2, 3]
permutation_found: false
now permutation: [Some(None), Some(None), Some(None), Some(None), Some(None), Some(None)]
res: None
thread 'rustc' panicked at 'didn't eliminated any indice in this round', compiler/rustc_typeck/src/check/fn_ctxt/arg_matrix.rs:412:21

chenyukang · 2022-08-30T02:13:47Z

b9bdc01
will fix this issue.

chenyukang · 2022-08-30T02:16:18Z

Another similar case is:

struct A;
struct B;
struct C;
struct D;

fn f(
    a1: A,
    a2: A,
    b1: B,
    b2: B,
    c1: C,
    c2: C,
) {}

fn main() {
    f(C, A, A, A, B, B, C);
}

with log:

ai: [ExpectedIdx(0), ExpectedIdx(2), ExpectedIdx(3), ExpectedIdx(4), ExpectedIdx(5)]
ii: [ProvidedIdx(0), ProvidedIdx(2), ProvidedIdx(3), ProvidedIdx(4), ProvidedIdx(5), ProvidedIdx(6)]
================== find_issue: ==================
x|  0  1  2  3  4
-|  -  -  -  -  -
0|  0  0  0  1  1
1|  1  0  0  0  0
2|  1  0  0  0  0
3|  0  1  1  0  0
4|  0  1  1  0  0
5|  0  0  0  1  1
i: 0, is_input: true, is_arg: true, useless: false, unsatisfiable: false
i: 1, is_input: true, is_arg: true, useless: false, unsatisfiable: false
i: 2, is_input: true, is_arg: true, useless: false, unsatisfiable: false
i: 3, is_input: true, is_arg: true, useless: false, unsatisfiable: false
i: 4, is_input: true, is_arg: true, useless: false, unsatisfiable: false
i: 5, is_input: true, is_arg: false, useless: false, unsatisfiable: true
loop i: 0 j: 0 compat: [3, 4]
permutation_found: false
now permutation: [Some(None), None, None, None, None, None]
loop i: 1 j: 1 compat: [0]
loop i: 1 j: 0 compat: [3, 4]
permutation_found: false
now permutation: [Some(None), None, None, None, None, None]
now permutation: [Some(None), Some(None), None, None, None, None]
loop i: 2 j: 2 compat: [0]
loop i: 2 j: 0 compat: [3, 4]
permutation_found: false
now permutation: [Some(None), Some(None), None, None, None, None]
now permutation: [Some(None), Some(None), Some(None), None, None, None]
loop i: 3 j: 3 compat: [1, 2]
permutation_found: false
now permutation: [Some(None), Some(None), Some(None), Some(None), None, None]
loop i: 4 j: 4 compat: [1, 2]
permutation_found: false
now permutation: [Some(None), Some(None), Some(None), Some(None), Some(None), None]
loop i: 5 j: 5 compat: [3, 4]
permutation_found: false
now permutation: [Some(None), Some(None), Some(None), Some(None), Some(None), Some(None)]
res: None
thread 'rustc' panicked at 'didn't eliminated any indice in this round', compiler/rustc_typeck/src/check/fn_ctxt/arg_matrix.rs:412:21

jackh726 · 2022-09-12T02:33:09Z

@bors r+

bors · 2022-09-12T02:33:11Z

📌 Commit 7e7dfb8 has been approved by jackh726

It is now in the queue for this repository.

bors · 2022-09-12T06:05:36Z

⌛ Testing commit 7e7dfb8 with merge 56e7678...

bors · 2022-09-12T08:46:35Z

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 56e7678 to master...

rust-timer · 2022-09-12T10:19:09Z

Finished benchmarking commit (56e7678): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	5.0%	[5.0%, 5.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	3.6%	[2.6%, 4.6%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-4.9%	[-4.9%, -4.9%]	1
Improvements ✅ (secondary)	-0.9%	[-0.9%, -0.9%]	1
All ❌✅ (primary)	0.8%	[-4.9%, 4.6%]	3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	3.4%	[3.4%, 3.4%]	1
Regressions ❌ (secondary)	2.9%	[2.9%, 2.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.4%	[3.4%, 3.4%]	1

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

rust-highfive assigned lcnr Aug 13, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 13, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 13, 2022

chenyukang force-pushed the fix-100478 branch from 25e5170 to af7b167 Compare August 13, 2022 18:01

chenyukang force-pushed the fix-100478 branch from af7b167 to 24a4a4f Compare August 14, 2022 05:45

chenyukang force-pushed the fix-100478 branch from 24a4a4f to 01e11e1 Compare August 14, 2022 06:11

rust-highfive assigned jackh726 and unassigned lcnr Aug 17, 2022

This comment has been minimized.

Sign in to view

chenyukang force-pushed the fix-100478 branch from 0d63de6 to b9bdc01 Compare August 30, 2022 02:03

Avoid infinite loop in function arguments checking

50e5d03

chenyukang force-pushed the fix-100478 branch from b9bdc01 to 5d81010 Compare September 8, 2022 15:51

This comment has been minimized.

Sign in to view

fix rust-lang#101097, avoid infinite loop in fn arguments checking

7e7dfb8

chenyukang force-pushed the fix-100478 branch from 5d81010 to 7e7dfb8 Compare September 9, 2022 01:51

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 12, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 12, 2022

bors merged commit 56e7678 into rust-lang:master Sep 12, 2022

rustbot added this to the 1.65.0 milestone Sep 12, 2022

chenyukang mentioned this pull request Nov 1, 2022

rustc v1.64.0 hangs for a crate with a syntax error #103823

Closed

Avoid infinite loop in function arguments checking #100502

Avoid infinite loop in function arguments checking #100502

Uh oh!

Conversation

chenyukang commented Aug 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Aug 13, 2022

Uh oh!

chenyukang commented Aug 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

compiler-errors commented Aug 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chenyukang commented Aug 13, 2022

Uh oh!

jackh726 commented Aug 13, 2022

Uh oh!

jackh726 commented Aug 13, 2022

Uh oh!

chenyukang commented Aug 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chenyukang commented Aug 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chenyukang commented Aug 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lcnr commented Aug 17, 2022

Uh oh!

This comment has been minimized.

chenyukang commented Aug 30, 2022

Uh oh!

chenyukang commented Aug 30, 2022

Uh oh!

chenyukang commented Aug 30, 2022

Uh oh!

This comment has been minimized.

jackh726 commented Sep 12, 2022

Uh oh!

bors commented Sep 12, 2022

Uh oh!

bors commented Sep 12, 2022

Uh oh!

bors commented Sep 12, 2022

Uh oh!

rust-timer commented Sep 12, 2022

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

Uh oh!

Uh oh!

chenyukang commented Aug 13, 2022 •

edited

Loading

chenyukang commented Aug 13, 2022 •

edited

Loading

compiler-errors commented Aug 13, 2022 •

edited

Loading

chenyukang commented Aug 14, 2022 •

edited

Loading

chenyukang commented Aug 14, 2022 •

edited

Loading

chenyukang commented Aug 14, 2022 •

edited

Loading