Don't run resolve_vars_if_possible in normalize_erasing_regions

[jyn514]

Neither @eddyb nor I could figure out what this was for. I changed it to assert_eq!(normalized_value, infcx.resolve_vars_if_possible(&normalized_value)); and it passed the UI test suite.

Outdated, I figured out the issue - needs_infer() needs to come after erasing the lifetimes

Strangely, if I change it to assert!(!normalized_value.needs_infer()) it panics almost immediately:

query stack during panic:
#0 [normalize_generic_arg_after_erasing_regions] normalizing `<str::IsWhitespace as str::pattern::Pattern>::Searcher`
#1 [needs_drop_raw] computing whether `str::iter::Split<str::IsWhitespace>` needs drop
#2 [mir_built] building MIR for `str::<impl str>::split_whitespace`
#3 [unsafety_check_result] unsafety-checking `str::<impl str>::split_whitespace`
#4 [mir_const] processing MIR for `str::<impl str>::split_whitespace`
#5 [mir_promoted] processing `str::<impl str>::split_whitespace`
#6 [mir_borrowck] borrow-checking `str::<impl str>::split_whitespace`
#7 [analysis] running analysis passes on this crate
end of query stack

I'm not entirely sure what's going on - maybe the two disagree?

For context, this came up while reviewing #77467 (cc @lcnr).

Possibly this needs a crater run?

r? @nikomatsakis
cc @matthewjasper

[eddyb]

I know what it's for (normalize may create inference variables), I'm just surprised it doesn't seem to be needed.
(Also, we probably don't want to land this PR as-is)

[jyn514]

@lcnr said:

I expect resolve_vars_if_possible to be a noop if it is called after normalize in a fresh infcx.

So possibly there are other call sites that also don't need to do this?

[nikomatsakis]

Is the motivation here purely performance?

[jyn514]

The performance doesn't hurt, but mostly I'm trying to understand why this is necessary in the first place. eddyb suggested that I add resolve_vars_if_possible to #77467, but we couldn't come up with an example where it made a difference in the output.

[lcnr]

I believe that resolve_vars_if_possible should always be a noop after normalize in a fresh InferCtxt as I expect all infer variables created during normalization to be still unknown after it.

So I do think it is valuable to remove that afaict unnecessary step here.

[nikomatsakis]

I think I put it in there mostly as a precaution, in case inference variables wound up in the output. I'm not sure if that can happen or not to be honest.

[nikomatsakis]

In particular I don't think of it is as an invariant that normalization will yield a result free of inference variables-- do you think it is an invariant, @lcnr ?

[lcnr]

I don't expect the result to be free to inference variables, I expect the result to be free of Known inference variables.

My personal expectation is that this should be an invariant and believe that this is already the case, even if not documented rn.

I have only recently familiarized myself with this code so I might be incorrect here though

[nikomatsakis]

@lcnr "known" inference variables are indeed what I meant. I guess I don't generally expect that as an invariant from any particular function I imagine that sometimes we wind up substituting or returning things that may reference inference variables that not been fully "resolved" to their known types. I can believe that it's true that in this case that we maintain the invariant, but if so I don't feel like it was intentional (but maybe if I re-read the code I'll realize it has to be that way).

[bors]

☔ The latest upstream changes (presumably #78313) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[lcnr]

@nikomatsakis would you mind adding a debug assert here for now so that if we do ever create known inference variables, we have a test case?

[nikomatsakis]

@lcnr in other words, landing this PR but with a debug assert? I would be ok with that.

[jyn514]

in other words, landing this PR but with a debug assert? I would be ok with that.

Ok, I updated this to use a debug assert in both cases.

[nikomatsakis]

I'm ok with this, but I'm not sure if this is what @lcnr meant -- I suspect they meant to move the resolve_vars_if_possible into the debug assert as well. I guess that I mildly prefer this in that I don't think it's generally meant to be an invariant that functions will return "fully resolved" types that don't contain inference variables.

[lcnr]

I think the approach by @jyn514 is good as it doesn't even change the behavior, so it most definity can't break anything

I would keep it as is, even if it isn't what i originally intended

[jyn514]

This is ready for review.

[lcnr]

looks simple enough to me

@bors r+ rollup

[bors]

📌 Commit 6354e85 has been approved by lcnr

[bors]

⌛ Testing commit 6354e85 with merge 2e57231...

[bors]

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 2e57231 to master...