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

Improve error message for infinitely recursive impl Trait type #47659

Closed
cramertj opened this issue Jan 22, 2018 · 10 comments · Fixed by #56074
Closed

Improve error message for infinitely recursive impl Trait type #47659

cramertj opened this issue Jan 22, 2018 · 10 comments · Fixed by #56074
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one.

Comments

@cramertj
Copy link
Member

cramertj commented Jan 22, 2018

After #47529, attempts to compile code which builds an infinitely recursive impl Trait type will error with "overflow evaluating the requirement impl Quux". This isn't super user-friendly, and should be improved.

@TimNN TimNN added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-diagnostics Area: Messages for errors, warnings, and lints labels Jan 23, 2018
@tailhook
Copy link

tailhook commented Aug 4, 2018

Just encountered this issue when refactoring large parser using combine crate. It used to have a wrapper function parser(func) for creating reusable parsers. Now the recommended way is to rely on -> impl Parser return type instead (see second example).

Parsers inherently have many branches and are self-recursive. So after refactoring the codebase I encountered this error and had to guess which parser from 15 refactored functions has this issue.

It wasn't very hard when I understood the issue, but the stack overflow error wasn't very informative of what happened at all.

@ZerothLaw
Copy link

Would this be a good issue for a relative newcomer to tackle? As I understand it, its mostly about adding a more informative error message.

@cramertj
Copy link
Member Author

@ZerothLaw I'm not sure-- it's hard to know exactly what to report. You know that an overflow has occurred, but not that it is necessarily due to recursive use of impl Trait. This is where the current overflow reporting is triggered-- we could add a suggestion hinting that this may be the result of attempting to build an infinitely recursive impl Trait type, but we don't know for sure that that is the cause. You could record the DefId of each TyAnon type encountered while folding recursively and then report a recursion error if you see a repeat DefId.

@ZerothLaw
Copy link

That's a good point. So before working on it, one should first figure out what the acceptable solution would be?

@alexreg
Copy link
Contributor

alexreg commented Nov 12, 2018

@ZerothLaw Did you make any progress on this lately? I can maybe help give you more pointers, if needed.

@ZerothLaw
Copy link

I haven't made any attempt on this. Any suggestions?

@alexreg
Copy link
Contributor

alexreg commented Nov 13, 2018

@ZerothLaw

@cramertj provides a good approach in his comment. To be specific, you might want to start by:

  • Add a new field to the AssociatedTypeNormalizer struct for storing already-seen types (values of ty parameter to fold_ty) that you're folding with. (I think @cramertj's suggestion of using the def-ids for the types would work in most cases, but may fail in some complex edge cases. Maybe he can chip in here.) A HashSet (FxHashSet?) may be the best type for this.
  • At the top of the fold_ty function, check for the newly-encountered type in the set of already-seen types. If you find it, raise the error; if not, add the type to the set and continue.
  • Model the error reporting on one of the examples @estebank gave in his above comment. Probably the second one is a better prototype. Here is the code for reporting it; I suggest creating a new method that's pretty similar to that one.
  • Create a test for this issue under src/tests/ui/issues/issue-47659 with an appropriate minimal reproducible case. Make sure you "bless" the test once everything is running.

The rustc development guide is pretty handy if you're curious about how the compiler fits together, or what purpose a particular type or function serves, in many cases. Still, if you have any difficulties with the above, post back here, or hop onto the #compiler channel on Discord. Good luck!

@alexreg
Copy link
Contributor

alexreg commented Dec 25, 2018

@ZerothLaw Had a chance to tackle the above yet, by chance?

@cramertj
Copy link
Member Author

@alexreg This is covered by #56074.

bors added a commit that referenced this issue Jan 4, 2019
…ikomatsakis

Forbid recursive impl trait

There is no type T, such that `T = [T; 2]`, but impl Trait could sometimes
be to circumvented this.

This patch makes it a hard error for an opaque type to resolve to such a
"type". Before this can be merged it needs

- [x] A better error message - it's good enough for now.
- [x] A crater run (?) to see if this any real-world code

closes #47659
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants