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

Fix a span in parse_ty_bare_fn. #126724

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Fix a span in parse_ty_bare_fn. #126724

merged 1 commit into from
Jun 26, 2024

Conversation

nnethercote
Copy link
Contributor

It currently goes one token too far.

Example: line 259 of tests/ui/abi/compatibility.rs:

test_abi_compatible!(fn_fn, fn(), fn(i32) -> i32);

This commit changes the span for the second element from fn(), to fn(), i.e. removes the extraneous comma.

This doesn't affect any tests. I found it while debugging some other code. Not a big deal but an easy fix so I figure it worth doing.

r? @spastorino

@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 Jun 20, 2024
compiler-errors
compiler-errors approved these changes Jun 20, 2024
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Oof, yeah. Span handling in the parser can be very manual.

@compiler-errors
Copy link
Member

r? compiler-errors @bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 20, 2024

📌 Commit e9a5dae has been approved by compiler-errors

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 Jun 20, 2024
@compiler-errors
Copy link
Member

Actually, wait. Please add a test lmao

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 20, 2024
@compiler-errors
Copy link
Member

Unless there's a good reason to believe that this test is impossible to create

@nnethercote nnethercote force-pushed the fix-parse_ty_bare_fn-span branch from e9a5dae to c39b86a Compare June 21, 2024 06:22
@nnethercote
Copy link
Contributor Author

nnethercote commented Jun 21, 2024 •
edited
Loading

@compiler-errors: the decl_span field has a single meaningful use:

rust/compiler/rustc_resolve/src/late.rs

Line 813 in 7a08f84

let span = ty.span.shrink_to_lo().to(bare_fn.decl_span.shrink_to_lo());

and the hi field (i.e. the part changed by this PR) isn't even used, lol.

So the only way to test this would be with a -Zunpretty=ast-tree test, but that seems like overkill. I suggest we merge as is.

Please note that I have also modified a comment in the updated version.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 25, 2024

📌 Commit c39b86a has been approved by compiler-errors

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 25, 2024
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 25, 2024
@compiler-errors
Rollup merge of rust-lang#126724 - nnethercote:fix-parse_ty_bare_fn-s…
f9a58c6 
…pan, r=compiler-errors

Fix a span in `parse_ty_bare_fn`.

It currently goes one token too far.

Example: line 259 of `tests/ui/abi/compatibility.rs`:
```
test_abi_compatible!(fn_fn, fn(), fn(i32) -> i32);
```
This commit changes the span for the second element from `fn(),` to `fn()`, i.e. removes the extraneous comma.

This doesn't affect any tests. I found it while debugging some other code. Not a big deal but an easy fix so I figure it worth doing.

r? `@spastorino`
@compiler-errors compiler-errors mentioned this pull request Jun 25, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 25, 2024
@bors
Auto merge of rust-lang#126949 - compiler-errors:rollup-2ma9x7n, r=co…
7716d1a 
…mpiler-errors

Rollup of 9 pull requests

Successful merges:

 - rust-lang#126326 (Un-unsafe the `StableOrd` trait)
 - rust-lang#126618 (Mark assoc tys live only if the corresponding trait is live)
 - rust-lang#126724 (Fix a span in `parse_ty_bare_fn`.)
 - rust-lang#126746 (Deny `use<>` for RPITITs)
 - rust-lang#126868 (not use offset when there is not ends with brace)
 - rust-lang#126884 (Do not ICE when suggesting dereferencing closure arg)
 - rust-lang#126915 (Don't suggest awaiting in closure patterns)
 - rust-lang#126916 (Specify target specific linker for `riscv64gc-gnu` job)
 - rust-lang#126926 (Tweak a confusing comment in `create_match_candidates`)

r? `@ghost`
`@rustbot` modify labels: rollup
@compiler-errors
Copy link
Member

needs blessing: #126949 (comment)

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 25, 2024
@nnethercote
Fix a span in parse_ty_bare_fn.
cf0251d 
It currently goes one token too far.

Example: line 259 of `tests/ui/abi/compatibility.rs`:
```
test_abi_compatible!(fn_fn, fn(), fn(i32) -> i32);
```
This commit changes the span for the second element from `fn(),` to
`fn()`, i.e. removes the extraneous comma.
@nnethercote nnethercote force-pushed the fix-parse_ty_bare_fn-span branch from c39b86a to cf0251d Compare June 25, 2024 22:24
@nnethercote
Copy link
Contributor Author

Talk about serendipity: this incorrect span had no meaningful uses until @spastorino added one in #126758, a PR that came after this one! But now we can see the effect in the changed test output, so that's nice.

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Jun 25, 2024

📌 Commit cf0251d has been approved by compiler-errors

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 25, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 26, 2024
@matthiaskrgr
Rollup merge of rust-lang#126724 - nnethercote:fix-parse_ty_bare_fn-s…
6207e6d 
…pan, r=compiler-errors

Fix a span in `parse_ty_bare_fn`.

It currently goes one token too far.

Example: line 259 of `tests/ui/abi/compatibility.rs`:
```
test_abi_compatible!(fn_fn, fn(), fn(i32) -> i32);
```
This commit changes the span for the second element from `fn(),` to `fn()`, i.e. removes the extraneous comma.

This doesn't affect any tests. I found it while debugging some other code. Not a big deal but an easy fix so I figure it worth doing.

r? `@spastorino`
@matthiaskrgr matthiaskrgr mentioned this pull request Jun 26, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 26, 2024
@bors
Auto merge of rust-lang#126974 - matthiaskrgr:rollup-2neyugj, r=matth…
add1ce4 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#126158 (Disallow setting some built-in cfg via set the command-line)
 - rust-lang#126724 (Fix a span in `parse_ty_bare_fn`.)
 - rust-lang#126812 (Rename `tcx` to `cx` in new solver generic code)
 - rust-lang#126879 (fix Drop items getting leaked in Filter::next_chunk)
 - rust-lang#126941 (Migrate `run-make/llvm-ident` to `rmake.rs`)
 - rust-lang#126954 (resolve: Tweak some naming around import ambiguities)
 - rust-lang#126968 (Don't ICE during RPITIT refinement checking for resolution errors after normalization)
 - rust-lang#126973 (Fix bad replacement for unsafe extern block suggestion)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr matthiaskrgr mentioned this pull request Jun 26, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 26, 2024
@bors
Auto merge of rust-lang#126979 - matthiaskrgr:rollup-fdqledz, r=matth…
7731802 
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#126724 (Fix a span in `parse_ty_bare_fn`.)
 - rust-lang#126812 (Rename `tcx` to `cx` in new solver generic code)
 - rust-lang#126879 (fix Drop items getting leaked in Filter::next_chunk)
 - rust-lang#126925 (Change E0369 to give note informations for foreign items.)
 - rust-lang#126938 (miri: make sure we can find link_section statics even for the local crate)
 - rust-lang#126954 (resolve: Tweak some naming around import ambiguities)
 - rust-lang#126964 (Migrate `lto-empty`, `invalid-so` and `issue-20626` `run-make` tests to rmake.rs)
 - rust-lang#126968 (Don't ICE during RPITIT refinement checking for resolution errors after normalization)
 - rust-lang#126971 (Bump black, ruff and platformdirs)
 - rust-lang#126973 (Fix bad replacement for unsafe extern block suggestion)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit dd6b046 into rust-lang:master Jun 26, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 26, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 26, 2024
@rust-timer
Unrolled build for rust-lang#126724
94a5ffd 
Rollup merge of rust-lang#126724 - nnethercote:fix-parse_ty_bare_fn-span, r=compiler-errors

Fix a span in `parse_ty_bare_fn`.

It currently goes one token too far.

Example: line 259 of `tests/ui/abi/compatibility.rs`:
```
test_abi_compatible!(fn_fn, fn(), fn(i32) -> i32);
```
This commit changes the span for the second element from `fn(),` to `fn()`, i.e. removes the extraneous comma.

This doesn't affect any tests. I found it while debugging some other code. Not a big deal but an easy fix so I figure it worth doing.

r? ``@spastorino``
@nnethercote nnethercote deleted the fix-parse_ty_bare_fn-span branch June 26, 2024 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors approved these changes

Assignees

@compiler-errors compiler-errors

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.81.0
Development

Successfully merging this pull request may close these issues.
5 participants
@nnethercote @compiler-errors @bors @spastorino @rustbot