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

Detect extra space in keyword for better hint #113309

Closed
wants to merge 1 commit into from
Closed

Detect extra space in keyword for better hint #113309

wants to merge 1 commit into from

Conversation

chenyukang
Copy link
Member

Fixes #89640

r? @Nilstrieb

I met the same issue, then found out this old issue :)

@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 Jul 4, 2023
chenyukang
chenyukang commented Jul 4, 2023
View reviewed changes
tests/ui/parser/utf16-be-without-bom.stderr Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is:
image

Copy link
Member

Choose a reason for hiding this comment

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

this isn't exactly great but I guess it's okay. I think finding such invalid tokens should actually be a fatal error, but I'm not sure about that. Maybe try putting up a PR that makes source files must contain UTF-8 encoded text, unexpected null bytes might occur when a different encoding is used errors fatal and see what wg-diagnostics thinks?

@chenyukang chenyukang force-pushed the yukang-fix-89640-space branch from 6484899 to b344bfa Compare July 4, 2023 02:18
Noratrieb
Noratrieb approved these changes Jul 4, 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.

r=me after fixing formatting

compiler/rustc_parse/src/parser/diagnostics.rs Show resolved Hide resolved
compiler/rustc_parse/src/parser/diagnostics.rs Outdated Show resolved Hide resolved
tests/ui/parser/utf16-be-without-bom.stderr Outdated
Copy link
Member

Choose a reason for hiding this comment

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

this isn't exactly great but I guess it's okay. I think finding such invalid tokens should actually be a fatal error, but I'm not sure about that. Maybe try putting up a PR that makes source files must contain UTF-8 encoded text, unexpected null bytes might occur when a different encoding is used errors fatal and see what wg-diagnostics thinks?

fmease
fmease requested changes Jul 4, 2023
View reviewed changes
tests/ui/suggestions/issue-89640.stderr Outdated Show resolved Hide resolved
fmease
fmease requested changes Jul 4, 2023
View reviewed changes
tests/ui/suggestions/issue-89640.stderr Outdated Show resolved Hide resolved
@chenyukang chenyukang force-pushed the yukang-fix-89640-space branch from d3fa2fa to 799d291 Compare July 4, 2023 10:13
fmease
fmease approved these changes Jul 4, 2023
View reviewed changes
Copy link
Member

@fmease fmease left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Ofc, the let-chain in diagnostics.rs is not formatted quite right (body indented by 8 spaces, && indented by 2) but since rustfmt can't handle them yet it's fine. We're gonna have to format-the-world anyways once it does.

@chenyukang
Copy link
Member Author

Ofc, the let-chain in diagnostics.rs is not formatted quite right (body indented by 8 spaces, && indented by 2) but since rustfmt can't handle them yet it's fine. We're gonna have to format-the-world anyways once it does.

yes, I was always mad with format when using let-chains,
seems we already have a draft style for it: #110568

@chenyukang
Copy link
Member Author

@bors r=Nilstrieb

@bors
Copy link
Contributor

bors commented Jul 4, 2023

📌 Commit 799d291 has been approved by Nilstrieb

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 Jul 4, 2023
@bors
Copy link
Contributor

bors commented Jul 4, 2023

⌛ Testing commit 799d291 with merge 4dbc7e3...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 4, 2023
@bors
Auto merge of rust-lang#113309 - chenyukang:yukang-fix-89640-space, r…
4dbc7e3 
…=Nilstrieb

Detect extra space in keyword for better hint

Fixes rust-lang#89640

r? `@Nilstrieb`

I met the same issue, then found out this old issue :)
@bors
Copy link
Contributor

bors commented Jul 4, 2023

☀️ Test successful - checks-actions
Approved by: Nilstrieb
Pushing 4dbc7e3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 4, 2023
@Noratrieb
Copy link
Member

Thank you GitHub for closing this PR!

@chenyukang
Copy link
Member Author

Thank you GitHub for closing this PR!

What happened? Seems it is a Github bug ....

@fmease
Copy link
Member

fmease commented Jul 4, 2023

Last time this happened to one of my PRs it got merged in the end with a one hour delay...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4dbc7e3): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.7% [2.7%, 2.7%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 654.344s -> 653.659s (-0.10%)

@chenyukang
Copy link
Member Author

Last time this happened to one of my PRs it got merged in the end with a one hour delay...

Github says this PR has conflicts:
image

While I checked there is no conflict...

@Noratrieb
Copy link
Member

I'll just close the PR manually. The commit is on master, GitHub is just confused. Everything is fine.

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

@Noratrieb Noratrieb Noratrieb approved these changes

@fmease fmease fmease approved these changes

Assignees

@Noratrieb Noratrieb

Labels
merged-by-bors This PR was explicitly merged by bors. 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
No milestone
Development

Successfully merging this pull request may close these issues.

Suggest joining two identifiers if the grammer is invalid and they would form a keyword
6 participants
@chenyukang @bors @Noratrieb @fmease @rust-timer @rustbot