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

Migrate rustc_mir_build diagnostics #104417

Merged
merged 18 commits into from
Dec 18, 2022
Merged

Migrate rustc_mir_build diagnostics #104417

merged 18 commits into from
Dec 18, 2022

Conversation

mejrs
Copy link
Contributor

@mejrs mejrs commented Nov 14, 2022 •
edited
Loading

Rebases #100854

The remaining issue is how to better resolve 72bea68

The diagnostic macros seems to generate a broken diagnostic, and I couldn't figure out how to manually format the fluent message, so I hardcoded the format string for now. I'd like pointers to a better fix for this.

Also, I'm not 100% sure I didn't mess up a rebase somewhere 🙂

r? @davidtwco

@rustbot
Copy link
Collaborator

rustbot commented Nov 14, 2022

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

Please see the contribution instructions for more information.

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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 14, 2022
@rustbot
Copy link
Collaborator

rustbot commented Nov 14, 2022

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

@bors

This comment was marked as resolved.

Sign in to view
@davidtwco
Copy link
Member

The remaining issue is how to better resolve 72bea68

The diagnostic macros seems to generate a broken diagnostic, and I couldn't figure out how to manually format the fluent message, so I hardcoded the format string for now. I'd like pointers to a better fix for this.

You should be able to fix this by using #[subdiagnostic(eager)] where this subdiagnostic is used.

@davidtwco davidtwco mentioned this pull request Nov 15, 2022
Closed
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@mejrs
Copy link
Contributor Author

mejrs commented Nov 15, 2022

You should be able to fix this by using #[subdiagnostic(eager)] where this subdiagnostic is used.

Ah that's good to know. I've fixed that and rebased.

davidtwco
davidtwco approved these changes Nov 23, 2022
View reviewed changes
compiler/rustc_mir_build/src/thir/pattern/check_match.rs
} else {
// FIXME: migration of this diagnostic will require list support
Copy link
Member

Choose a reason for hiding this comment

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

You can try using the list support added in #104047, maybe better as a follow-up so we can land this though.

@davidtwco
Copy link
Member

Apologies for the delay in getting back to this.

@bors r

@bors
Copy link
Contributor

bors commented Nov 23, 2022

📌 Commit 2354697 has been approved by davidtwco

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 Nov 23, 2022
Manishearth added a commit to Manishearth/rust that referenced this pull request Nov 23, 2022
@Manishearth
Rollup merge of rust-lang#104417 - mejrs:mir_build, r=davidtwco
dd066cd 
Migrate rustc_mir_build diagnostics

Rebases rust-lang#100854

~~The remaining issue is how to better resolve rust-lang@72bea68af4ee2a41c44998916f6a789163f12e7d~~

~~The diagnostic macros seems to generate a broken diagnostic, and I couldn't figure out how to manually format the fluent message, so I hardcoded the format string for now. I'd like pointers to a better fix for this.~~

Also, I'm not 100% sure I didn't mess up a rebase somewhere 🙂

r? `@davidtwco`
@Manishearth Manishearth mentioned this pull request Nov 23, 2022
Closed
@Manishearth
Copy link
Member

@bors r-

#104790 (comment)

@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 Nov 23, 2022
@bors
Copy link
Contributor

bors commented Nov 24, 2022

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

@mejrs
Copy link
Contributor Author

mejrs commented Nov 26, 2022

Rebased

@rustbot ready

@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 Nov 26, 2022
@jyn514
Copy link
Member

jyn514 commented Dec 8, 2022

@bors r=davidtwco

@bors
Copy link
Contributor

bors commented Dec 8, 2022

📌 Commit 1d324dd has been approved by davidtwco

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 8, 2022
lunabunja and others added 15 commits December 17, 2022 19:08
@mejrs
Copy link
Contributor Author

mejrs commented Dec 17, 2022

I think this fixes the ice that was hit during rollup, but I'm not sure how to test it locally.

@mejrs mejrs marked this pull request as draft December 17, 2022 21:54
@mejrs
Copy link
Contributor Author

mejrs commented Dec 17, 2022

I think this works on dist-riscv64-linux now - see https://github.com/rust-lang/rust/actions/runs/3721937031/jobs/6312424723

@mejrs mejrs marked this pull request as ready for review December 17, 2022 22:31
@Noratrieb
Copy link
Member

@bors r=davidtwco
Let's hope it really works now

@bors
Copy link
Contributor

bors commented Dec 18, 2022

📌 Commit f7e894c has been approved by davidtwco

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 18, 2022
@bors
Copy link
Contributor

bors commented Dec 18, 2022

⌛ Testing commit f7e894c with merge 35a99ee...

@bors
Copy link
Contributor

bors commented Dec 18, 2022

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 35a99ee to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 18, 2022
@bors bors merged commit 35a99ee into rust-lang:master Dec 18, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 18, 2022
@bors bors mentioned this pull request Dec 18, 2022
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (35a99ee): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

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)		 2.3% [2.3%, 2.3%] 1
Regressions ❌
(secondary)		 1.3% [1.3%, 1.3%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 2.3% [2.3%, 2.3%] 1

Cycles

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

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

@davidtwco davidtwco davidtwco approved these changes

Assignees

@davidtwco davidtwco

Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.68.0
Development

Successfully merging this pull request may close these issues.
10 participants
@mejrs @rustbot @bors @davidtwco @rust-log-analyzer @Manishearth @jyn514 @Noratrieb @rust-timer @lunabunja