Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run normalize_projection_ty in Canonical mode #79517

Closed
wants to merge 1 commit into from

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Nov 28, 2020

This allows recovering from overflow errors when normalizing a type.

This would have prevented the regression in #79506 (comment). I don't yet have a test, since normalization in rustdoc was reverted (#79469). If you want me to add one, I can add a -Z normalize-docs flag to rustdoc and use it in the test.

r? @Aaron1011 for review or reassignment.

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-traits Area: Trait system T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 28, 2020
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 28, 2020
Comment on lines 227 to 228
infcx,
freshener: infcx.freshener(),
intercrate: true,
intercrate_ambiguity_causes: None,
allow_negative_impls: false,
query_mode: TraitQueryMode::Standard,
..SelectionContext::new(infcx)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just cleanup, I can move it to a separate PR if you like.

@Aaron1011
Copy link
Member

I'm pretty sure that this is incorrect - the projection code shouldn't have it's behvaior changed just at for rustdkc.

Instaed, I think we need a way of enabling this only for the specific part of rustdoc that needs it.

@jyn514
Copy link
Member Author

jyn514 commented Nov 28, 2020

What about adding InferCtxt::query_mode: Option<TraitQueryMode> that can be used to override the default mode?

@jyn514
Copy link
Member Author

jyn514 commented Nov 28, 2020

Hmm, actually that might be tricky since rustc_infer doesn't currently depend on rustc_trait_selection :( I could move TraitQueryMode to rustc_infer instead?

@jyn514 jyn514 changed the title Run normalize_project_ty in Canonical mode Run normalize_projection_ty in Canonical mode Nov 28, 2020
@jyn514
Copy link
Member Author

jyn514 commented Nov 28, 2020

@Aaron1011 can you explain more about why using Canonical would be wrong for rustc (as opposed to rustdoc)?

@Aaron1011
Copy link
Member

I'm not very familiar with the details of how normalization works, but the current mode was chosen for a reason, and changing it will almost certainly have wider consequences.

@Aaron1011
Copy link
Member

cc @matthewjasper

This allows recovering from overflow errors when normalizing a type.
@jyn514
Copy link
Member Author

jyn514 commented Dec 16, 2020

ping @matthewjasper - do you know why it was in Standard mode before? If this change is wrong, do you have an alternative suggestion for how to make this work for rustdoc? For context, this is meant to avoid one of the process::exits when normalizing a type and the type-length limit gets hit, because rustdoc normalizes associated items the user didn't ask to normalize.

@jyn514
Copy link
Member Author

jyn514 commented Jan 7, 2021

ping @matthewjasper. I would prefer not to special case rustdoc here but I can if you think it makes more sense.

@jyn514
Copy link
Member Author

jyn514 commented Jan 16, 2021

Closing in favor of #81091.

@jyn514 jyn514 closed this Jan 16, 2021
@jyn514 jyn514 deleted the canonical-mode branch January 16, 2021 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-traits Area: Trait system S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants