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.

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

Migrate rustc_lint lint diagnostics #101138

Merged
merged 33 commits into from
Jan 13, 2023

Conversation

Rejyr
Copy link
Contributor

@Rejyr Rejyr commented Aug 28, 2022

Part 2 of Migrate rustc_lint errors to SessionDiagnostic

r? @davidtwco

TODO

  • Refactor some lints manually implementing DecorateLint to use Option<Subdiagnostic>.
  • Add #[rustc_lint_diagnostics] to lint functions in context.rs.
  • Migrate hidden_unicode_codepoints.rs.
  • Migrate UnsafeCode in builtin.rs.
  • Migrate the rest of builtin.rs.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 28, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 28, 2022
@Rejyr
Copy link
Contributor Author

Rejyr commented Aug 28, 2022

@rustbot label: +A-diagnostics

@rustbot rustbot added the A-diagnostics Area: Messages for errors, warnings, and lints label Aug 28, 2022
@Rejyr Rejyr force-pushed the diagnostic-migration-rustc-lint-pt2 branch 2 times, most recently from 47cf0c8 to 8d6c390 Compare August 31, 2022 10:03
@davidtwco davidtwco added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Aug 31, 2022
@davidtwco davidtwco mentioned this pull request Aug 31, 2022
84 tasks
@Rejyr
Copy link
Contributor Author

Rejyr commented Sep 4, 2022

@davidtwco How would I untangle emit_ffi_unsafe_type_lint and FfiResult? Would I only modify emit_ffi_unsafe_type_lint, create separate LintDiagnostic types for each lint, or just leave it?

@rust-log-analyzer

This comment has been minimized.

@Rejyr Rejyr force-pushed the diagnostic-migration-rustc-lint-pt2 branch 2 times, most recently from 859887d to 6181790 Compare September 4, 2022 20:03
@davidtwco
Copy link
Member

@davidtwco How would I untangle emit_ffi_unsafe_type_lint and FfiResult? Would I only modify emit_ffi_unsafe_type_lint, create separate LintDiagnostic types for each lint, or just leave it?

I think emit_ffi_unsafe_type_lint/FfiResult might be okay as they are - this lint is using translated diagnostics already, it isn't in a type implementing SessionDiagnostic but that's probably okay for this case.

@Rejyr Rejyr force-pushed the diagnostic-migration-rustc-lint-pt2 branch from 2d6a3c6 to e5be009 Compare September 5, 2022 16:02
@Rejyr
Copy link
Contributor Author

Rejyr commented Sep 5, 2022

@davidtwco How would I untangle emit_ffi_unsafe_type_lint and FfiResult? Would I only modify emit_ffi_unsafe_type_lint, create separate LintDiagnostic types for each lint, or just leave it?

@davidtwco Same thing for check_panic in non_fmt_panic.rs?

@Rejyr Rejyr force-pushed the diagnostic-migration-rustc-lint-pt2 branch from 3fab63e to ba2f365 Compare September 5, 2022 19:20
@Rejyr
Copy link
Contributor Author

Rejyr commented Sep 5, 2022

@davidtwco How would I migrate usages of struct_lint and struct_lint_level in levels.rs?

@davidtwco
Copy link
Member

@davidtwco How would I migrate usages of struct_lint and struct_lint_level in levels.rs?

We have emit_spanned_lint and #[derive(LintDiagnostic)] - I think that covers what you need for struct_lint? struct_lint_level might require a new function which does something similar to emit_spanned_lint - probably just changing its decorate argument to be a impl DecorateLint?

@davidtwco Same thing for check_panic in non_fmt_panic.rs?

I think so, yeah.

@Rejyr
Copy link
Contributor Author

Rejyr commented Sep 18, 2022

@davidtwco There's no emit_spanned_lint for LintLevelsBuilder, should I add one? For struct_lint_level, in levels.rs, it only uses LintLevelsBuilder.sess for the Session and LintLevelsBuilder.lint_level(..) or LintLevelsBuilder.provider.get_lint_level(..) for the Level and LintLevelSource, so should I add a method like emit_spanned_lint_level to LintLevelsBuilder?

@Rejyr Rejyr force-pushed the diagnostic-migration-rustc-lint-pt2 branch 3 times, most recently from b9d1e5c to 00e4d6d Compare September 18, 2022 14:42
@bors

This comment was marked as resolved.

@Rejyr Rejyr force-pushed the diagnostic-migration-rustc-lint-pt2 branch from 6f4f315 to a3c0081 Compare September 22, 2022 00:47
@davidtwco
Copy link
Member

@davidtwco There's no emit_spanned_lint for LintLevelsBuilder, should I add one? For struct_lint_level, in levels.rs, it only uses LintLevelsBuilder.sess for the Session and LintLevelsBuilder.lint_level(..) or LintLevelsBuilder.provider.get_lint_level(..) for the Level and LintLevelSource, so should I add a method like emit_spanned_lint_level to LintLevelsBuilder?

What diagnostic are you trying to migrate where this is necessary?

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

LGTM so far, thanks for your continued work on this.

@Rejyr
Copy link
Contributor Author

Rejyr commented Sep 22, 2022

@davidtwco There's no emit_spanned_lint for LintLevelsBuilder, should I add one? For struct_lint_level, in levels.rs, it only uses LintLevelsBuilder.sess for the Session and LintLevelsBuilder.lint_level(..) or LintLevelsBuilder.provider.get_lint_level(..) for the Level and LintLevelSource, so should I add a method like emit_spanned_lint_level to LintLevelsBuilder?

What diagnostic are you trying to migrate where this is necessary?

@davidtwco Mostly the diagnostics in levels.rs. All of them are either struct_lint (one instance) or struct_lint_level (6 instances).
There's a usage of struct_lint in context.rs at line 1013, but it's in the implementation of LintContext for EarlyContext.

@Rejyr
Copy link
Contributor Author

Rejyr commented Sep 22, 2022

@davidtwco Also, should I make a new PR for refactoring the diagnostic structs in errors.rs since I've gotten more accustomed to migrating them, or can I add them to this PR?

@Rejyr Rejyr force-pushed the diagnostic-migration-rustc-lint-pt2 branch from 0ea6b44 to 1f0b080 Compare January 9, 2023 23:39
@Rejyr Rejyr force-pushed the diagnostic-migration-rustc-lint-pt2 branch from 1f0b080 to 88e5dd2 Compare January 9, 2023 23:58
@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 10, 2023

📌 Commit 88e5dd2 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 Jan 10, 2023
@bors
Copy link
Contributor

bors commented Jan 10, 2023

⌛ Testing commit 88e5dd2 with merge c62631d39f8d2ecafc2bf57779be6bc835c116fd...

@bors
Copy link
Contributor

bors commented Jan 10, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 10, 2023
@ehuss
Copy link
Contributor

ehuss commented Jan 10, 2023

@bors retry

#106682

@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 Jan 10, 2023
@rust-log-analyzer

This comment was marked as resolved.

@bors
Copy link
Contributor

bors commented Jan 13, 2023

⌛ Testing commit 88e5dd2 with merge bfffe40...

@bors
Copy link
Contributor

bors commented Jan 13, 2023

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 13, 2023
@bors bors merged commit bfffe40 into rust-lang:master Jan 13, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 13, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (bfffe40): 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.7% [0.7%, 0.7%] 1
Regressions ❌
(secondary)
1.9% [1.2%, 2.5%] 2
Improvements ✅
(primary)
-2.9% [-3.2%, -2.7%] 2
Improvements ✅
(secondary)
-1.1% [-1.1%, -1.1%] 1
All ❌✅ (primary) -1.7% [-3.2%, 0.7%] 3

Cycles

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

@Rejyr Rejyr deleted the diagnostic-migration-rustc-lint-pt2 branch January 13, 2023 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants