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

99144 enable doctests #106574

Closed
wants to merge 4 commits into from
Closed

99144 enable doctests #106574

wants to merge 4 commits into from

Conversation

technetos
Copy link
Member

This PR adds fixes for doc tests, fixes #99144

r? @jyn514

@rustbot
Copy link
Collaborator

rustbot commented Jan 7, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jyn514 (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added 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. labels Jan 7, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 7, 2023

Some changes occurred in need_type_info.rs

cc @lcnr

Noratrieb
Noratrieb reviewed Jan 13, 2023
View reviewed changes
Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

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

Thanks! I have a few small comments on things to improve

compiler/rustc_infer/src/infer/outlives/verify.rs
@@ -288,7 +288,7 @@ impl<'cx, 'tcx> VerifyBoundCx<'cx, 'tcx> {
///
/// It will not, however, work for higher-ranked bounds like:
///
/// ```compile_fail,E0311
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

If this compiles now then the previous comment is wrong as well (and we should find out why)

compiler/rustc_ast_lowering/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/error_reporting/need_type_info.rs Show resolved Hide resolved
compiler/rustc_infer/src/infer/region_constraints/mod.rs
Comment on lines +265 to +270
/// # trait SomeTrait<'a> {
/// # type Item;
/// # }
/// fn foo<'a, 'b, T: SomeTrait<'a>>()
/// where
/// <T as SomeTrait<'a>>::Item: 'a
/// {
///
/// }
Copy link
Member

Choose a reason for hiding this comment

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

This is a lot more verbose and makes it harder to understand that the where clause is what's relevant here. I think it makes sense to ignore this block.

@jyn514
Copy link
Member

jyn514 commented Jan 13, 2023

r? @Nilstrieb

❤️

@rustbot rustbot assigned Noratrieb and unassigned jyn514 Jan 13, 2023
@Noratrieb Noratrieb added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 13, 2023
This was referenced Jan 15, 2023
Closed
Merged
reez12g
reez12g reviewed Jan 24, 2023
View reviewed changes
compiler/rustc_infer/src/infer/error_reporting/need_type_info.rs Show resolved Hide resolved
compiler/rustc_infer/src/infer/error_reporting/nice_region_error/find_anon_type.rs
@@ -14,8 +14,8 @@ use rustc_middle::ty::{self, Region, TyCtxt};
/// br - the bound region corresponding to the above region which is of type `BrAnon(_)`
///
/// # Example
/// ```compile_fail,E0623
/// fn foo(x: &mut Vec<&u8>, y: &u8)
/// ```
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove the compile_fail here?

compiler/rustc_infer/src/infer/outlives/test_type_match.rs
@@ -13,9 +13,11 @@ use crate::infer::region_constraints::VerifyIfEq;

/// Given a "verify-if-eq" type test like:
///
/// ```ignore (not valid syntax)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like it would be better to align the description with other parts of the document and make it ignore(illustrative).

@jyn514 jyn514 mentioned this pull request Jan 24, 2023
Closed
@bors
Copy link
Contributor

bors commented Jan 30, 2023

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

@Dylan-DPC
Copy link
Member

@technetos any updates on this?

@technetos
Copy link
Member Author

Oh ill fix this today

@technetos technetos force-pushed the 99144-enable-doctests branch from 98d590d to fad8d28 Compare March 5, 2023 18:10
@rustbot
Copy link
Collaborator

rustbot commented Mar 5, 2023

These commits modify the Cargo.lock file. Random changes to Cargo.lock can be introduced when switching branches and rebasing PRs.
This was probably unintentional and should be reverted before this PR is merged.

If this was intentional then you can ignore this comment.

@technetos
Copy link
Member Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 5, 2023
@Noratrieb
Copy link
Member

Noratrieb commented Mar 6, 2023 •
edited
Loading

You haven't addressed my and reez12g's feedback yet :)
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 6, 2023
@technetos @Noratrieb
Update compiler/rustc_ast_lowering/src/lib.rs
6841944 
Co-authored-by: nils <48135649+Nilstrieb@users.noreply.github.com>
@bors
Copy link
Contributor

bors commented May 8, 2023

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

@Noratrieb
Copy link
Member

This was subsumed by #106486, closing it. Thank you for the PR anyways!

@Noratrieb Noratrieb closed this May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Noratrieb Noratrieb Noratrieb left review comments

@reez12g reez12g reez12g left review comments

Assignees

@Noratrieb Noratrieb

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Run compiler doctests in CI
7 participants
@technetos @rustbot @jyn514 @bors @Dylan-DPC @Noratrieb @reez12g