Filtering spans when emitting json#102922

kper · 2022-10-11T16:23:02Z

According to the issue #102902, we shouldn't emit spans which have an empty span and no suggested replacement.

rust-highfive · 2022-10-11T16:23:06Z

(rust-highfive has picked a reviewer for you, use r? to override)

oli-obk · 2022-10-11T18:34:25Z

Could we solve this at the time of registering the suggestion? I would hope we could just add a debug assertion and avoid ever having these suggestions at all.

Basically check this in span_suggestion and either don't register anything (in release mode) or panic (in debug mode).

kper · 2022-10-11T20:28:39Z

@oli-obk I have added the assertions for the debug build. I also changed the condition to avoid the emitting of the second suggestion.

compiler-errors · 2022-10-11T20:54:09Z

compiler/rustc_errors/src/diagnostic.rs

There's Span::is_empty I think

jyn514 · 2022-10-12T03:48:05Z

Why only a debug assert? Performance doesn't matter for diagnostics.

oli-obk · 2022-10-12T09:47:38Z

Why only a debug assert? Performance doesn't matter for diagnostics.

because I don't want to start crashing people's compiler. Maybe we could make it a full assert on nightly or sth.

compiler/rustc_hir_analysis/src/astconv/mod.rs

oli-obk · 2022-10-12T18:13:54Z

CI seems to have found more cases of such suggestions

compiler/rustc_expand/src/base.rs

kper · 2022-10-13T17:30:00Z

Why only a debug assert? Performance doesn't matter for diagnostics.

because I don't want to start crashing people's compiler. Maybe we could make it a full assert on nightly or sth.

I am not quite sure how to do this. Is there already something for that or should I create a new macro?

oli-obk · 2022-10-15T06:55:54Z

compiler/rustc_parse/src/parser/diagnostics.rs

Some useful suggestions are now missing. Maybe check here for emptyness and avoid adding it only then

I am checking the pre_span which might have the empty span before adding it in inc_dec_standalone_suggest.

if !pre_span.is_empty() { patches.push((pre_span, String::new())); }

jruderman · 2022-10-17T21:59:21Z

Could this check be expanded from "don't replace empty with empty" to "don't replace any string with the same string"?

oli-obk · 2022-10-18T09:38:05Z

I am not quite sure how to do this. I

That's ok I think we should just stick with the debug assert for now

oli-obk · 2022-10-18T09:42:58Z

That may be annoying to thread through. We don't always have access to the source map. But definitely something to explore.

@bors r+ rollup

bors · 2022-10-18T09:43:00Z

📌 Commit 9e2c2a5 has been approved by oli-obk

It is now in the queue for this repository.

… r=oli-obk Filtering spans when emitting json According to the issue rust-lang#102902, we shouldn't emit spans which have an empty span and no suggested replacement.

JohnTitor · 2022-10-18T12:21:05Z

Failed in rollup: #103184 (comment)
@bors r-

oli-obk · 2022-10-18T12:33:10Z

Ah looks like clippy also has such suggestions. You can run the clippy tests with ./x.py test src/tools/clippy

rustbot · 2022-10-20T06:14:02Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

oli-obk · 2022-10-20T08:07:56Z

Thanks this lgtm now.

Please squash your commits as they contain a lot of back and forth now.

…tions

kper · 2022-10-21T07:28:42Z

@oli-obk thank you for your quick reviews! I have rebased it.

oli-obk · 2022-10-21T07:39:50Z

@bors r+ rollup

bors · 2022-10-21T07:39:52Z

📌 Commit 28d0312 has been approved by oli-obk

It is now in the queue for this repository.

Rollup of 6 pull requests Successful merges: - rust-lang#102287 (Elaborate supertrait bounds when triggering `unused_must_use` on `impl Trait`) - rust-lang#102922 (Filtering spans when emitting json) - rust-lang#103051 (translation: doc comments with derives, subdiagnostic-less enum variants, more derive use) - rust-lang#103111 (Account for hygiene in typo suggestions, and use them to point to shadowed names) - rust-lang#103260 (Fixup a few tests needing asm support) - rust-lang#103321 (rustdoc: improve appearance of source page navigation bar) Failed merges: - rust-lang#103209 (Diagnostic derives: allow specifying multiple alternative suggestions) r? `@ghost` `@rustbot` modify labels: rollup

… r=oli-obk Filtering spans when emitting json According to the issue rust-lang#102902, we shouldn't emit spans which have an empty span and no suggested replacement.

Rollup of 6 pull requests Successful merges: - rust-lang#102287 (Elaborate supertrait bounds when triggering `unused_must_use` on `impl Trait`) - rust-lang#102922 (Filtering spans when emitting json) - rust-lang#103051 (translation: doc comments with derives, subdiagnostic-less enum variants, more derive use) - rust-lang#103111 (Account for hygiene in typo suggestions, and use them to point to shadowed names) - rust-lang#103260 (Fixup a few tests needing asm support) - rust-lang#103321 (rustdoc: improve appearance of source page navigation bar) Failed merges: - rust-lang#103209 (Diagnostic derives: allow specifying multiple alternative suggestions) r? `@ghost` `@rustbot` modify labels: rollup

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 11, 2022

rust-highfive assigned oli-obk Oct 11, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 11, 2022

This comment has been minimized.

Sign in to view

compiler-errors reviewed Oct 11, 2022

View reviewed changes

compiler/rustc_errors/src/diagnostic.rs Outdated

Copy link

Contributor

compiler-errors Oct 11, 2022

Choose a reason for hiding this comment

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

There's Span::is_empty I think

This comment has been minimized.

Sign in to view

oli-obk reviewed Oct 12, 2022

View reviewed changes

compiler/rustc_hir_analysis/src/astconv/mod.rs Outdated Show resolved Hide resolved

kper force-pushed the bugfix/102902-filtering-json branch from 46e051b to 4f48fc1 Compare October 13, 2022 16:05

kper commented Oct 13, 2022

View reviewed changes

compiler/rustc_expand/src/base.rs Outdated Show resolved Hide resolved

oli-obk reviewed Oct 15, 2022

View reviewed changes

kper force-pushed the bugfix/102902-filtering-json branch from 4f48fc1 to 9e2c2a5 Compare October 15, 2022 14:07

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 Oct 18, 2022

Dylan-DPC mentioned this pull request Oct 18, 2022

Rollup of 6 pull requests #103184

Closed

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 Oct 18, 2022

kper force-pushed the bugfix/102902-filtering-json branch from 9e2c2a5 to 2b495d2 Compare October 20, 2022 06:13

Implement assertions and fixes to not emit empty spans without sugges…

28d0312

…tions

kper force-pushed the bugfix/102902-filtering-json branch from 3e6f1fb to 28d0312 Compare October 20, 2022 08:58

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 21, 2022

Dylan-DPC mentioned this pull request Oct 21, 2022

Rollup of 6 pull requests #103344

Merged

bors merged commit 0a0e9f7 into rust-lang:master Oct 21, 2022

rustbot added this to the 1.66.0 milestone Oct 21, 2022

jruderman mentioned this pull request Oct 24, 2022

let (_ = 3; causes suggestion to remove a paren that is only hypothetical #103497

Closed

Uh oh!

Conversation

kper commented Oct 11, 2022

Uh oh!

rust-highfive commented Oct 11, 2022

Uh oh!

This comment has been minimized.

oli-obk commented Oct 11, 2022

Uh oh!

kper commented Oct 11, 2022

Uh oh!

compiler-errors Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

jyn514 commented Oct 12, 2022

Uh oh!

oli-obk commented Oct 12, 2022

Uh oh!

Uh oh!

oli-obk commented Oct 12, 2022

Uh oh!

Uh oh!

kper commented Oct 13, 2022

Uh oh!

oli-obk Oct 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kper Oct 15, 2022

Choose a reason for hiding this comment

Uh oh!

jruderman commented Oct 17, 2022

Uh oh!

oli-obk commented Oct 18, 2022

Uh oh!

oli-obk commented Oct 18, 2022

Uh oh!

bors commented Oct 18, 2022

Uh oh!

JohnTitor commented Oct 18, 2022

Uh oh!

oli-obk commented Oct 18, 2022

Uh oh!

rustbot commented Oct 20, 2022

Uh oh!

oli-obk commented Oct 20, 2022

Uh oh!

kper commented Oct 21, 2022

Uh oh!

oli-obk commented Oct 21, 2022

Uh oh!

bors commented Oct 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

oli-obk Oct 15, 2022 •

edited

Loading