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.

Sign up for GitHub

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

Jump to bottom

Move reporting recursion limit errors outside of the trait system #81091

Open
jyn514 opened this issue Jan 16, 2021 · 6 comments
Open

Move reporting recursion limit errors outside of the trait system #81091

jyn514 opened this issue Jan 16, 2021 · 6 comments
Labels
A-trait-system Area: Trait system C-cleanup Category: PRs that clean code up or issues documenting cleanup. fixed-by-next-solver Fixed by the next-generation trait solver, `-Znext-solver`. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Jan 16, 2021

This is the proper fix for the breakage in #79506 and #79459.

The steps are basically to find all of the places that create a OverflowError or SelectionError::Overflow and make sure that they are always early returned until outside of rustc_trait_selection::traits::{select, project}, at which point they should be emitted. Overflow errors should never be converted to any other kind of error (such as EvaluatedToRecur or EvaluatedToError)

Originally posted by @matthewjasper in #81055 (comment)
@jyn514 jyn514 mentioned this issue Jan 16, 2021
Closed
@jyn514 jyn514 self-assigned this Jan 16, 2021
@jonas-schievink jonas-schievink added A-trait-system Area: Trait system C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 16, 2021
@jyn514
Copy link
Member Author

jyn514 commented Jan 17, 2021

@jonas-schievink I don't know if I would describe this as cleanup - this is blocking a major rustdoc feature: #79525

@jyn514 jyn514 mentioned this issue Jan 18, 2021
Closed
jyn514 added a commit to jyn514/rust that referenced this issue Jan 18, 2021
@jyn514
Inline evaluate_obligation instead of going through the query system
1548e0f 
There are several reasons I changed this:

- `evaluate_obligation` is only used in one place in
  `rustc_trait_selection`, and in fact warns you against using it
  anywhere else.
- The implementation in `rustc_traits` was considerably more complicated
  than necessary, because it didn't have the context available in
  `rustc_trait_selection`.
- It allows moving OverflowError into rustc_trait_selection, making
  rust-lang#81091 simpler (in particular,
  it allows holding an `Obligation` in OverflowError, which is only
  defined in `rustc_infer` and not available in rustc_middle).

The only reason to keep the previous behavior is if the cache from the
query system made it significantly faster.
@jyn514 jyn514 mentioned this issue Mar 2, 2021
Open
5 tasks
@jyn514
Copy link
Member Author

jyn514 commented Mar 2, 2021

I made a (very broken) start on this in master...jyn514:recover-overflow.

@koivunej
Copy link

Rebased the above as master...koivunej:recover-overflow-continued

@WaffleLapkin WaffleLapkin mentioned this issue Feb 6, 2022
Closed
@jyn514 jyn514 removed their assignment Feb 15, 2022
This was referenced Sep 20, 2022
Merged
Closed
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Sep 20, 2022
@Dylan-DPC
Rollup merge of rust-lang#102037 - jyn514:normalize-docs, r=lcnr
e4e4eec 
Make cycle errors recoverable

In particular, this allows rustdoc to recover from cycle errors when normalizing associated types for documentation.

In the past, `@jackh726` has said we need to be careful about overflow errors: rust-lang#91430 (comment)

> Off the top of my head, we definitely should be careful about treating overflow errors the same as
"not implemented for some reason" errors. Otherwise, you could end up with behavior that is
different depending on recursion depth. But, that might be context-dependent.

But cycle errors should be safe to unconditionally report; they don't depend on the recursion depth, they will always be an error whenever they're encountered.

Helps with rust-lang#81091.

r? `@lcnr` cc `@matthewjasper`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Sep 22, 2022
@Dylan-DPC
Rollup merge of rust-lang#102037 - jyn514:normalize-docs, r=lcnr
0d9120c 
Make cycle errors recoverable

In particular, this allows rustdoc to recover from cycle errors when normalizing associated types for documentation.

In the past, ``@jackh726`` has said we need to be careful about overflow errors: rust-lang#91430 (comment)

> Off the top of my head, we definitely should be careful about treating overflow errors the same as
"not implemented for some reason" errors. Otherwise, you could end up with behavior that is
different depending on recursion depth. But, that might be context-dependent.

But cycle errors should be safe to unconditionally report; they don't depend on the recursion depth, they will always be an error whenever they're encountered.

Helps with rust-lang#81091.

r? ``@lcnr`` cc ``@matthewjasper``
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Sep 22, 2022
@Dylan-DPC
Rollup merge of rust-lang#102037 - jyn514:normalize-docs, r=lcnr
d5ae673 
Make cycle errors recoverable

In particular, this allows rustdoc to recover from cycle errors when normalizing associated types for documentation.

In the past, ```@jackh726``` has said we need to be careful about overflow errors: rust-lang#91430 (comment)

> Off the top of my head, we definitely should be careful about treating overflow errors the same as
"not implemented for some reason" errors. Otherwise, you could end up with behavior that is
different depending on recursion depth. But, that might be context-dependent.

But cycle errors should be safe to unconditionally report; they don't depend on the recursion depth, they will always be an error whenever they're encountered.

Helps with rust-lang#81091.

r? ```@lcnr``` cc ```@matthewjasper```
@fmease fmease mentioned this issue Mar 10, 2023
Open
@fmease
Copy link
Member

fmease commented Mar 10, 2023

I assume the fix proposed in this issue will fix the recently reported I-hang issue #108826, too. :)

@jyn514
Copy link
Member Author

jyn514 commented Mar 10, 2023

I think T-types is planning to fix this by landing the new trait solver, which is unlikely to have the same bugs as the existing solver (instead they will be new and more exciting bugs).

@fmease
Copy link
Member

fmease commented Mar 10, 2023

Right, that's true :)

@fmease fmease added the fixed-by-next-solver Fixed by the next-generation trait solver, `-Znext-solver`. label Sep 6, 2023
@WaffleLapkin WaffleLapkin mentioned this issue Jan 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-trait-system Area: Trait system C-cleanup Category: PRs that clean code up or issues documenting cleanup. fixed-by-next-solver Fixed by the next-generation trait solver, `-Znext-solver`. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@koivunej @jonas-schievink @fmease @jyn514