fix: resolve const generic param-env panic in type projection

[benodiwal]

Closes #21173

[ChayimFriedman2]

Please add a test. You can take the code from #21173, but please verify it fails without the changes and succeeds with them.

[benodiwal]

Please add a test. You can take the code from #21173, but please verify it fails without the changes and succeeds with them.

I have tested it manually for both the cases on my machine, let me try to add some tests as well.

[benodiwal]

Hey @ChayimFriedman2, I've tested it manually. Panics on master, works with the fix.

I tried adding an automated test but ran into issues. The panic happens deep in the trait solver during normalization, and salsa catches panics from queries. Even calling borrowck_query directly in tests doesn't trigger it the same way analysis-stats does.

Any suggestions on how to set up a test that would catch this? Happy to add one if you can point me in the right direction.

Results of analysis-stats:

1. Mater branch:

  thread 'main' panicked at ra-ap-rustc_type_ir-0.139.0/src/inherent.rs:580:13:
  cannot find `!BoundConst { var: 0 }` in param-env: ParamEnv {
      clauses: [],
  }

2. This branch with fix branch:

  Inference:           154.38ms, 0b
  MIR lowering:        308.83µs, 0b
  Mir failed bodies: 0 (0%)
  Data layouts:        68.71µs, 0b
  Failed data layouts: 0 (0%)
  Const evaluation:    4.08µs, 0b
  IDE:                 1.61ms, 0b (1 files)
  Total:               2.68s, 0b

[ChayimFriedman2]

It should just panic and fail the test, doesn't it?

[ShoyuVanilla]

salsa doesn't catch and silence the panics, at least the ones emitted from the type system. So, if you failed to reproduce the panic, it just didn't happened.
You might need to import some minicore items or try different test functions

[benodiwal]

@ChayimFriedman2 @ShoyuVanilla I have added some tests about the panic.

NOTE: I have add an additional test for the case of assignment which would pass anyways for both the branches.

[ChayimFriedman2]

We don't need that many tests, and I also don't think the long comment above the test is a good idea - the explanation for the test can be looked up by opening the issue. Other than that, LGTM.

[ChayimFriedman2]

Oh, and please squash.

[benodiwal]

We don't need that many tests, and I also don't think the long comment above the test is a good idea - the explanation for the test can be looked up by opening the issue. Other than that, LGTM.

Sure Got it. Should i keep this test or not ?

the case of assignment which would pass anyways for both the branches.

[ChayimFriedman2]

Ah no. Please spend some more effort and try to create a test. Possibly of help will be to include the minicore item sized.

[benodiwal]

Ah no. Please spend some more effort and try to create a test. Possibly of help will be to include the minicore item sized.

Oh I think you misunderstood. There were 4 tests earlier, 3 of them regarding this issue and one in which assignment operation was there. It was not for this issue as it was working on both branches. I just added it to show that this case is not replicating on this issue. This was kinda unncessary.

So I have removed the last 3 tests and kept only one which is a test for this functionality. Panics on master and pass on this branch

[benodiwal]

So I have removed the unnecessary test cases.
Now only the required test case is there. And it shows the fix as well perfectly. Thanks !!

[ChayimFriedman2]

Ah yes, I misunderstood then.

Thanks!