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

Make rustdoc JSON not cause ICE #3190

Merged
merged 2 commits into from
May 31, 2022

Conversation

Enselic
Copy link
Contributor

@Enselic Enselic commented May 26, 2022

To tro to help figure out what is causing rust-lang/rust#93588

Comment on lines 335 to 343
impl<T: Query> AsQuery for T {
type SqlType = <Self as Query>::SqlType;
type Query = Self;

fn as_query(self) -> Self::Query {
self
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for working on this ❤️

This change would unfortunately break most of diesel's query builder. As a blind guess I would expect that this is also the main fix for the ICE, as this impl implies quite a lot of other concrete impls. (Just have a look what else implements AsQuery or Query to get a feeling for that). If I can help with concrete information here just reach out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for taking a look! I was just being a bit lazy. I pushed a commit where I did the same change as I did for AsExpression, namely replacing Self with T, and now the diff should look less problematic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weiznich If you are familiar with the diesel code base, the best way you can help is probably to remove more and more code from diesel/diesel until the ICE stops from happening. Do it "one line at a time". Then we will know what line that is crucial to making the ICE happen. I have tried this myself but I found the diesel code base to be riddled with dependencies spread across the code base via macros and such, and I wasn't tenacious enough to follow through. But finding a minimal way to reproduce the problem we see would certainly help. That is what rust-lang/rust#93588 (comment) was talking about too.

Once have such an MRE it will probably not be hard to fix the root problem.

@Enselic
Copy link
Contributor Author

Enselic commented May 27, 2022

Sorry! I realized just now that I accidentally created a PR in this repo instead of my fork.

I created a PR in my fork now: Enselic#1, so please refer to that instead.

@Enselic Enselic closed this May 27, 2022
@weiznich
Copy link
Member

To be sure: If it's as simple as changing these three impls to prevent this ICE from happening I would be totally fine with just changing diesel here.

@Enselic
Copy link
Contributor Author

Enselic commented May 30, 2022

If you asses that to be a good near-term solution, feel free to merge this code. That would be totally fine with me. However, I think at least some other crates in the diesel workspace still causes ICE when generating rustdoc JSON, but maybe it is just as easy to fix them as it is to fix the main crate.

Long-term, rustdoc of course needs to fix the ICE for diesel and other crates that has the same code structure. It should't be necessary change client code in order to make rustdoc JSON work.

BTW, I am very curious, so I hope you don't mind me asking: What do you use the diesel rustdoc JSON for?

@weiznich
Copy link
Member

What do you use the diesel rustdoc JSON for?

I've played around with something like your cargo-public-api tool, especially the diff mode. That's nothing that worked at the time due to the ICE issues.

@weiznich weiznich reopened this May 31, 2022
@weiznich weiznich marked this pull request as ready for review May 31, 2022 14:00
@weiznich weiznich merged commit 661870c into diesel-rs:master May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants