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

Keep track of the start of the argument block of a closure #104199

Merged
merged 1 commit into from
Dec 4, 2022
Merged

Keep track of the start of the argument block of a closure #104199

merged 1 commit into from
Dec 4, 2022

Conversation

SarthakSingh31
Copy link
Contributor

This removes a call to tcx.sess.source_map() from compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs as required by #97417.

VsCode automatically applied rustfmt to the files I edited under src/tools. I can undo that if its a problem.

r? @cjgillot

@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 Nov 9, 2022
@rustbot
Copy link
Collaborator

rustbot commented Nov 9, 2022

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@calebcartwright
Copy link
Member

VsCode automatically applied rustfmt to the files I edited under src/tools. I can undo that if its a problem.

Yes this will almost certainly be problematic (especially looking at the diff to the rustfmt source) and should be reverted

@SarthakSingh31
Copy link
Contributor Author

@calebcartwright I removed the formatting.

@bors
Copy link
Contributor

bors commented Nov 18, 2022

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

@bors
Copy link
Contributor

bors commented Nov 23, 2022

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

cjgillot
cjgillot reviewed Nov 27, 2022
View reviewed changes
compiler/rustc_ast/src/ast.rs Outdated
@@ -1282,6 +1282,7 @@ pub struct Closure {
pub body: P<Expr>,
/// The span of the argument block `|...|`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you correct the comment to mention it contains the eventual move too?

compiler/rustc_ast/src/ast.rs Outdated
@@ -1282,6 +1282,7 @@ pub struct Closure {
pub body: P<Expr>,
/// The span of the argument block `|...|`.
pub fn_decl_span: Span,
pub cl_pipe_start: BytePos,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment?
Should we just have a span for move || -> ty and one for just ||?
Like fn_arg_span you made for HIR.

Copy link
Contributor Author

@SarthakSingh31 SarthakSingh31 Nov 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I wrote this before #101562 to try to minimize the size of ast::Expr. Now that ast::Closure is boxed inside ast::Expr I agree changing this to the span of |...| will be better.

compiler/rustc_ast_lowering/src/expr.rs Outdated
@@ -639,6 +642,8 @@ impl<'hir> LoweringContext<'_, 'hir> {
fn_decl,
body,
fn_decl_span: self.lower_span(span),
// FIXME(SarthakSingh31): This span needs to only span the arguments but it doesn't
Copy link
Contributor

Choose a reason for hiding this comment

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

This span comes from a desugaring, nothing corresponds to the arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the hir::Closure::fn_arg_span Option<Span> to better represent this

@cjgillot
Copy link
Contributor

cjgillot commented Dec 3, 2022

Thanks @SarthakSingh31.
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 3, 2022

📌 Commit 8f705e2 has been approved by cjgillot

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 3, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 3, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 3, 2022
@bors
Auto merge of rust-lang#105218 - matthiaskrgr:rollup-8d3k08n, r=matth…
b8a52e3 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#104199 (Keep track of the start of the argument block of a closure)
 - rust-lang#105050 (Remove useless borrows and derefs)
 - rust-lang#105153 (Create a hacky fail-fast mode that stops tests at the first failure)
 - rust-lang#105164 (Restore `use` suggestion for `dyn` method call requiring `Sized`)
 - rust-lang#105193 (Disable coverage instrumentation for naked functions)
 - rust-lang#105200 (Remove useless filter in unused extern crate check.)
 - rust-lang#105201 (Do not call fn_sig on non-functions.)
 - rust-lang#105208 (Add AmbiguityError for inconsistent resolution for an import)
 - rust-lang#105214 (update Miri)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c89bff2 into rust-lang:master Dec 4, 2022
@rustbot rustbot added this to the 1.67.0 milestone Dec 4, 2022
@SarthakSingh31 SarthakSingh31 deleted the issue-97417-1 branch December 4, 2022 01:25
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
@matthiaskrgr
Rollup merge of rust-lang#104199 - SarthakSingh31:issue-97417-1, r=cj…
caca5ed 
…gillot

Keep track of the start of the argument block of a closure

This removes a call to `tcx.sess.source_map()` from [compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs](https://github.com/rust-lang/rust/compare/master...SarthakSingh31:issue-97417-1?expand=1#diff-8406bbc0d0b43d84c91b1933305df896ecdba0d1f9269e6744f13d87a2ab268a) as required by rust-lang#97417.

VsCode automatically applied `rustfmt` to the files I edited under `src/tools`. I can undo that if its a problem.

r? `@cjgillot`
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
@bors
Auto merge of rust-lang#105218 - matthiaskrgr:rollup-8d3k08n, r=matth…
612a8be 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#104199 (Keep track of the start of the argument block of a closure)
 - rust-lang#105050 (Remove useless borrows and derefs)
 - rust-lang#105153 (Create a hacky fail-fast mode that stops tests at the first failure)
 - rust-lang#105164 (Restore `use` suggestion for `dyn` method call requiring `Sized`)
 - rust-lang#105193 (Disable coverage instrumentation for naked functions)
 - rust-lang#105200 (Remove useless filter in unused extern crate check.)
 - rust-lang#105201 (Do not call fn_sig on non-functions.)
 - rust-lang#105208 (Add AmbiguityError for inconsistent resolution for an import)
 - rust-lang#105214 (update Miri)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
calebcartwright pushed a commit to calebcartwright/rust that referenced this pull request Jan 24, 2023
@matthiaskrgr
Rollup merge of rust-lang#104199 - SarthakSingh31:issue-97417-1, r=cj…
2119f7d 
…gillot

Keep track of the start of the argument block of a closure

This removes a call to `tcx.sess.source_map()` from [compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs](https://github.com/rust-lang/rust/compare/master...SarthakSingh31:issue-97417-1?expand=1#diff-8406bbc0d0b43d84c91b1933305df896ecdba0d1f9269e6744f13d87a2ab268a) as required by rust-lang#97417.

VsCode automatically applied `rustfmt` to the files I edited under `src/tools`. I can undo that if its a problem.

r? `@cjgillot`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjgillot cjgillot cjgillot left review comments

Assignees

@cjgillot cjgillot

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.67.0
Development

Successfully merging this pull request may close these issues.
5 participants
@SarthakSingh31 @rustbot @calebcartwright @bors @cjgillot