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

Don't pass lint back out of lint decorator #118727

Merged
merged 2 commits into from
Dec 16, 2023
Merged

Don't pass lint back out of lint decorator #118727

merged 2 commits into from
Dec 16, 2023

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Dec 8, 2023 •
edited
Loading

Change the decorator function in the signature of the emit_lint/span_lint/etc family of methods from impl for<'a, 'b> FnOnce(&'b mut DiagnosticBuilder<'a, ()>) -> &'b mut DiagnosticBuilder<'a, ()> to impl for<'a, 'b> FnOnce(&'b mut DiagnosticBuilder<'a, ()>). I consider it easier to read this way, especially when there's control flow involved.

r? nnethercote though feel free to reassign

@rustbot
Copy link
Collaborator

rustbot commented Dec 8, 2023

⚠️ Warning ⚠️

  • These commits modify submodules.

@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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 8, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 8, 2023

Some changes occurred in src/tools/cargo

cc @ehuss

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@compiler-errors
Copy link
Member Author

compiler-errors commented Dec 8, 2023 •
edited
Loading

Sorry, fixed submodules.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot
Copy link
Collaborator

rustbot commented Dec 8, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

nnethercote
nnethercote reviewed Dec 8, 2023
View reviewed changes
compiler/rustc_lint/src/context.rs
decorate: impl for<'a, 'b> FnOnce(
&'b mut DiagnosticBuilder<'a, ()>,
) -> &'b mut DiagnosticBuilder<'a, ()>,
decorate: impl for<'a, 'b> FnOnce(&'b mut DiagnosticBuilder<'a, ()>),
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a comment at the top of this method that points to a long comment on struct_lint_level that explains why the return value is present, which @WaffleLapkin wrote, so I will forward the review to them.

Though I will say that I wonder if the comment is correct, because as written the change seems like an improvement, shortening the code a little. And any function of the form f(..., &mut T) -> &mut T is really weird.

r? @WaffleLapkin

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, well, I guess I missed that. I'm not necessarily convinced by either of the arguments, but yeah, I'll leave it up to waffle to weigh in.

Copy link
Member

Choose a reason for hiding this comment

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

Though I will say that I wonder if the comment is correct, because as written the change seems like an improvement, shortening the code a little. And any function of the form f(..., &mut T) -> &mut T is really weird.

The shortening comment refers to is the |lint| { lint.span_note(""); } (which auto-formats to 3 lines) vs |lint| lint.span_note("") (auto-formats to one). I think I overestimated how common this is.

I don't personally find &mut T -> &mut T a weird interface though, it's just the usual builder function (just like span_note).

Copy link
Contributor

Choose a reason for hiding this comment

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

&mut self -> &mut Self is one of the typical builder patterns, right? So this is similar but not quite the same. Not as weird as I first thought.

Copy link
Member

Choose a reason for hiding this comment

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

Yes

@rustbot rustbot assigned WaffleLapkin and unassigned nnethercote Dec 8, 2023
@bors
Copy link
Contributor

bors commented Dec 13, 2023

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

WaffleLapkin
WaffleLapkin approved these changes Dec 13, 2023
View reviewed changes
Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

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

I do kinda still agree with my comment about accidental use of .emit(), but I'm not sure that this bug is common enough for us to have an interface like that.

I'm ok with this change, it looks like it slightly improves things.

(please update my comments on struct_lint_level and lookup_with_diagnostics (maybe other places?) so that they mention you don't need .emit() but don't mention the weird signature)

compiler/rustc_codegen_ssa/src/codegen_attrs.rs Show resolved Hide resolved
@compiler-errors compiler-errors mentioned this pull request Dec 15, 2023
Merged
@compiler-errors
Copy link
Member Author

@bors r=WaffleLapkin

@bors
Copy link
Contributor

bors commented Dec 15, 2023

📌 Commit 252d99a has been approved by WaffleLapkin

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 15, 2023
@workingjubilee workingjubilee mentioned this pull request Dec 15, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 15, 2023
@bors
Auto merge of rust-lang#119002 - workingjubilee:rollup-dbfet7s, r=wor…
ca5c68a 
…kingjubilee

Rollup of 5 pull requests

Successful merges:

 - rust-lang#118396 (Collect lang items from AST, get rid of `GenericBound::LangItemTrait`)
 - rust-lang#118727 (Don't pass lint back out of lint decorator)
 - rust-lang#118956 (Make CStr documentation consistent ("nul" instead of "null"))
 - rust-lang#118981 (Remove an unneeded allocation)
 - rust-lang#118998 (Link to is_benchmark from the Ipv6Addr::is_global documentation)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 58353fa into rust-lang:master Dec 16, 2023
11 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 16, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 16, 2023
@rust-timer
Unrolled build for rust-lang#118727
b083786 
Rollup merge of rust-lang#118727 - compiler-errors:lint-decorate, r=WaffleLapkin

Don't pass lint back out of lint decorator

Change the decorator function in the signature of the `emit_lint`/`span_lint`/etc family of methods from `impl for<'a, 'b> FnOnce(&'b mut DiagnosticBuilder<'a, ()>) -> &'b mut DiagnosticBuilder<'a, ()>` to `impl for<'a, 'b> FnOnce(&'b mut DiagnosticBuilder<'a, ()>)`. I consider it easier to read this way, especially when there's control flow involved.

r? nnethercote though feel free to reassign
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Dec 16, 2023
@workingjubilee
Rollup merge of rust-lang#118989 - compiler-errors:lint-decorator-2, …
c7b492e 
…r=WaffleLapkin

Simplify lint decorator derive too

See last commit, since this is stacked on top of rust-lang#118727.

r? WaffleLapkin
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 16, 2023
@rust-timer
Unrolled build for rust-lang#118989
4492c19 
Rollup merge of rust-lang#118989 - compiler-errors:lint-decorator-2, r=WaffleLapkin

Simplify lint decorator derive too

See last commit, since this is stacked on top of rust-lang#118727.

r? WaffleLapkin
@xFrednet xFrednet mentioned this pull request Dec 17, 2023
Open
flip1995 pushed a commit to flip1995/rust that referenced this pull request Dec 28, 2023
@workingjubilee
Rollup merge of rust-lang#118727 - compiler-errors:lint-decorate, r=W…
d517ae6 
…affleLapkin

Don't pass lint back out of lint decorator

Change the decorator function in the signature of the `emit_lint`/`span_lint`/etc family of methods from `impl for<'a, 'b> FnOnce(&'b mut DiagnosticBuilder<'a, ()>) -> &'b mut DiagnosticBuilder<'a, ()>` to `impl for<'a, 'b> FnOnce(&'b mut DiagnosticBuilder<'a, ()>)`. I consider it easier to read this way, especially when there's control flow involved.

r? nnethercote though feel free to reassign
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WaffleLapkin WaffleLapkin WaffleLapkin approved these changes

@nnethercote nnethercote nnethercote left review comments

Assignees

@WaffleLapkin WaffleLapkin

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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.76.0
Development

Successfully merging this pull request may close these issues.
6 participants
@compiler-errors @rustbot @rust-log-analyzer @bors @nnethercote @WaffleLapkin