Skip to content

Report all lints, even if other errors already occurred.#108813

Closed
oli-obk wants to merge 1 commit into
rust-lang:masterfrom
oli-obk:report_all_the_errors
Closed

Report all lints, even if other errors already occurred.#108813
oli-obk wants to merge 1 commit into
rust-lang:masterfrom
oli-obk:report_all_the_errors

Conversation

@oli-obk

@oli-obk oli-obk commented Mar 6, 2023

Copy link
Copy Markdown
Contributor

This reduces surprises of the "I fixed all the errors, now I'm getting new ones"-kind

r? @ghost

@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 Mar 6, 2023
@oli-obk

oli-obk commented Mar 6, 2023

Copy link
Copy Markdown
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 6, 2023
@bors

bors commented Mar 6, 2023

Copy link
Copy Markdown
Collaborator

⌛ Trying commit c913bdc2672f0ae9f8a8b18d71171129234b1d53 with merge d9845989f7fe27114bf2d89955cc4fa6bade7a67...

@bors

bors commented Mar 6, 2023

Copy link
Copy Markdown
Collaborator

☀️ Try build successful - checks-actions
Build commit: d9845989f7fe27114bf2d89955cc4fa6bade7a67 (d9845989f7fe27114bf2d89955cc4fa6bade7a67)

1 similar comment
@bors

bors commented Mar 6, 2023

Copy link
Copy Markdown
Collaborator

☀️ Try build successful - checks-actions
Build commit: d9845989f7fe27114bf2d89955cc4fa6bade7a67 (d9845989f7fe27114bf2d89955cc4fa6bade7a67)

@rust-timer

This comment has been minimized.

@oli-obk oli-obk force-pushed the report_all_the_errors branch from c913bdc to d1623a3 Compare March 6, 2023 16:51
@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (d9845989f7fe27114bf2d89955cc4fa6bade7a67): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -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

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 6, 2023
@oli-obk

oli-obk commented Mar 6, 2023

Copy link
Copy Markdown
Contributor Author

Okay... not sure why I ran perf at all 😃 it's not like perf benchmarks emit errors 🤦

@matthiaskrgr

Copy link
Copy Markdown
Member

Well it kind makes sense to have some warning/error related benchmarks (perhaps even clippy lints 📎 ? )
I have seen code where rustc would print a couple of initial errors, and then "freeze" for a couple of seconds (4-10?) just to not print any further errors which always seemed a bit weird 🙃

@oli-obk

oli-obk commented Mar 7, 2023

Copy link
Copy Markdown
Contributor Author

r? compiler

Will bless the test later today

@lcnr

lcnr commented Mar 7, 2023

Copy link
Copy Markdown
Contributor

how likely is it that we report lints which are invalid because of a previous error? Currently slightly overwhelmed with my other reviews, so

r? compiler

@rustbot rustbot assigned compiler-errors and unassigned lcnr Mar 7, 2023
@oli-obk

oli-obk commented Mar 7, 2023

Copy link
Copy Markdown
Contributor Author

how likely is it that we report lints which are invalid because of a previous error?

We should fix those lints by checking for error types or errored typeck in case that happens. I found no obviously wrong cases in the changed diagnostics

@cjgillot

cjgillot commented Mar 7, 2023

Copy link
Copy Markdown
Contributor

Type checking errors still early-exit because of the ? on rustc_hir_analysis::check_crate a few lines above.
Removing all the short-circuits in that function open floodgates of a larger magnitude.

@rust-log-analyzer

This comment has been minimized.

@oli-obk oli-obk force-pushed the report_all_the_errors branch from d1623a3 to c1027bd Compare March 7, 2023 12:00
@bors

bors commented Mar 13, 2023

Copy link
Copy Markdown
Collaborator

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

@riking riking Mar 15, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This lint is clearly incorrect here: needs an bail for pointer to Err?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's not incorrect, but the wording can use some improvements. Basically only raw pointers that are just integers with a raw pointer type are allowed. All other raw pointers are forbidden.

@oli-obk oli-obk force-pushed the report_all_the_errors branch from c1027bd to c61b050 Compare March 16, 2023 10:38
@rustbot

rustbot commented Mar 16, 2023

Copy link
Copy Markdown
Collaborator

These commits modify the Cargo.lock file. Random changes to Cargo.lock can be introduced when switching branches and rebasing PRs.
This was probably unintentional and should be reverted before this PR is merged.

If this was intentional then you can ignore this comment.

@oli-obk oli-obk force-pushed the report_all_the_errors branch from c61b050 to 1610aa1 Compare March 16, 2023 10:53
@rust-log-analyzer

This comment has been minimized.

@bors

bors commented Apr 6, 2023

Copy link
Copy Markdown
Collaborator

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

@compiler-errors

Copy link
Copy Markdown
Contributor

@oli-obk what's the status on this? do you want me to review it, or is does it need reworking? does it need an FCP? 🤔

@oli-obk

oli-obk commented Apr 19, 2023

Copy link
Copy Markdown
Contributor Author

I guess I could rebase it and then compiler FCP it. Not sure it needs an FCP, i'll bring it up in the T-compiler triage

@oli-obk oli-obk added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Apr 19, 2023
@wesleywiser

Copy link
Copy Markdown
Member

I think either an FCP or MCP would be fine 🙂

@jyn514 jyn514 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is a good idea and would second an MCP.

@apiraino apiraino removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Apr 25, 2023
@apiraino apiraino added the needs-mcp This change is large enough that it needs a major change proposal before starting work. label May 10, 2023
@apiraino

Copy link
Copy Markdown
Contributor

switching review flag to note that an MCP would be fine in this case (Zulip mention)

@rustbot author

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels May 10, 2023
This reduces surprises of the "I fixed all the errors, now I'm getting new ones"-kind
@oli-obk oli-obk force-pushed the report_all_the_errors branch from 1610aa1 to f77c183 Compare May 11, 2023 12:55
@bors

bors commented May 16, 2023

Copy link
Copy Markdown
Collaborator

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

@JohnCSimon

Copy link
Copy Markdown
Member

@oli-obk
ping from triage - can you post your status on this PR? There hasn't been an update in a few months, and I see a few merge conflicts Thanks!

@WaffleLapkin

Copy link
Copy Markdown
Member

@JohnCSimon Oli is currently on leave, I think they'll continue working on this once they are back in January =)

@oli-obk

oli-obk commented Jan 3, 2024

Copy link
Copy Markdown
Contributor Author

The MCP has not been approved and has concerns that can only be resolved by a larger discussion (RFC?) about whether we want to report all lints and errors upfront, or do it in a phased system like we do now.

@oli-obk oli-obk closed this Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-mcp This change is large enough that it needs a major change proposal before starting work. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.