Suppress suggestions in derive macro #120272

long-long-float · 2024-01-23T16:46:06Z

I suppress warnings inside derive macros.

For example, the compiler emits following error by a program described in #118809 (comment) with a suggestion that indicates invalid syntax.

error[E0308]: `?` operator has incompatible types
 --> src/main.rs:3:17
  |
3 | #[derive(Debug, Deserialize)]
  |                 ^^^^^^^^^^^ expected `u32`, found `u64`
  |
  = note: `?` operator cannot convert from `u64` to `u32`
  = note: this error originates in the derive macro `Deserialize` (in Nightly builds, run with -Z macro-backtrace for more info)
help: you can convert a `u64` to a `u32` and panic if the converted value doesn't fit
  |
3 | #[derive(Debug, Deserialize.try_into().unwrap())]
  |                            ++++++++++++++++++++

For more information about this error, try `rustc --explain E0308`.
error: could not compile `serde_test` (bin "serde_test") due to 2 previous errors

In this PR, suggestions to cast are suppressed.

error[E0308]: `?` operator has incompatible types
 --> src/main.rs:3:17
  |
3 | #[derive(Debug, Deserialize)]
  |                 ^^^^^^^^^^^ expected `u32`, found `u64`
  |
  = note: `?` operator cannot convert from `u64` to `u32`
  = note: this error originates in the derive macro `Deserialize` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0308`.
error: could not compile `serde_test` (bin "serde_test") due to 2 previous errors

rustbot · 2024-01-23T16:46:14Z

r? @oli-obk

(rustbot has picked a reviewer for you, use r? to override)

Teapot4195 · 2024-01-23T19:44:52Z

LGTM (please don't take my word for it, I've only messed with this section of the compiler twice :D ).

I am qualified enough to ask for a squash though.

long-long-float · 2024-01-24T04:11:11Z

@Teapot4195 Thank you for comment. I squashed my commits.

oli-obk · 2024-01-24T08:48:25Z

I think we could check this directly in push_suggestion, so that no suggestions at all get reported on expansions of derives. Can you try that out and see if it works for you?

Adding a test for this is rather involved I'd presume, so manually testing it seems ok to me. But if you wanna try adding a test, we do have tests like /home/ubuntu/rustc/rust10/tests/ui/proc-macro/derive-b.rs so you can probably just copy one of these tests and their derive and create a derive that will always emit your diagnostic.

long-long-float · 2024-01-24T15:21:10Z

@oli-obk
Thank you for your suggestion. I fixed to check in push_suggestion.
I will try to add a test tomorrow.

oli-obk · 2024-01-24T15:36:15Z

Looks like rustc has tests 😆 try running ./x.py test --bless, that should do it. Though potentially clippy is also affected, so you'd need to bless that, too

long-long-float · 2024-01-25T10:15:49Z

@oli-obk
Thank you for telling me. I checked the result of test and noticed that some valid suggestions are removed after modification.

--- a/tests/ui/pattern/usefulness/issue-119778-type-error-ice.stderr
+++ b/tests/ui/pattern/usefulness/issue-119778-type-error-ice.stderr
@@ -14,11 +14,6 @@ error[E0425]: cannot find value `S` in this scope
    |
 LL |     struct Foo([u8; S]);
    |                     ^ not found in this scope
-   |
-help: you might be missing a const parameter
-   |
-LL |     struct Foo<const S: /* Type */>([u8; S]);
-   |               +++++++++++++++++++++

 error[E0658]: `impl Trait` in type aliases is unstable

I think the reason of test failure is that the checks in push_suggestion are affecting other suggestions than casting (checking in suggest_cast). To minimize effect, I think it's better to check in suggest_cast.
I would like to hear your opinion.

oli-obk · 2024-01-25T13:07:27Z

I think it's ok to lose this suggestion if it is still reported in another error in the same test. If that is not the case, then we need to figure out why this code is marked as expanded by a derive.

oli-obk · 2024-01-29T13:49:58Z

some tests are still failing

long-long-float · 2024-01-29T16:18:17Z

@oli-obk
I noticed that. And I investigated how to explain why some code is marked as expanded by a derive.

I have one assumption of this. For example, the suggestion in tests/ui/associated-types/issue-38821.rs is suppressed in this PR.

--- a/tests/ui/associated-types/issue-38821.stderr
+++ b/tests/ui/associated-types/issue-38821.stderr
@@ -12,10 +12,6 @@ LL | impl<T: NotNull> IntoNullable for T {
    |         |
    |         unsatisfied trait bound introduced here
    = note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info)
-help: consider further restricting the associated type
-   |
-LL |     Expr: Expression<SqlType=<Col::SqlType as IntoNullable>::Nullable>, <Col as Expression>::SqlType: NotNull,
-   |                                                                       +++++++++++++++++++++++++++++++++++++++
 
 error: aborting due to 1 previous error

This suggestion is came from Copy. It can be also checked from part.span.ctxt().outer_expn_data().

part.span.ctxt().outer_expn_data() = ExpnData {
    kind: Macro(
        Derive,
        "Copy",
    ),
    parent: crate0::{{expn2}},
    call_site: test.rs:23:17: 23:21 (#0),
    disambiguator: 0,
    def_site: /rust/library/core/src/marker.rs:480:1: 480:15 (#0),
    allow_internal_unstable: Some(
        [
            "core_intrinsics",
            "derive_clone_copy",
        ],
    ),
    edition: Edition2021,
    macro_def_id: Some(
        DefId(2:2960 ~ core[36fa]::marker::Copy),
    ),
    parent_module: Some(
        DefId(2:2944 ~ core[36fa]::marker),
    ),
    allow_internal_unsafe: false,
    local_inner_macros: false,
    collapse_debuginfo: false,
}

Also I checked the result of expansion by -Zunpretty=expanded.

// ...
//~^ ERROR the trait bound `<Col as Expression>::SqlType: NotNull` is not satisfied                                                     
pub enum ColumnInsertValue<Col, Expr> where Col: Column,                                                                                
    Expr: Expression<SqlType = <Col::SqlType as IntoNullable>::Nullable> {                                                              
    Expression(Col, Expr),                                                                                                              
    Default(Col),                                                                                                                       
}                                                                                                                                       
// ...
#[automatically_derived]
impl<Col: ::core::marker::Copy, Expr: ::core::marker::Copy>
    ::core::marker::Copy for ColumnInsertValue<Col, Expr> where Col: Column,
    Expr: Expression<SqlType = <Col::SqlType as IntoNullable>::Nullable> /* here? */ {
}
// ...

I am guessing that the suggestion is in the here? location, but I don't know how to explain it exactly.

Could you tell me how to explain this or is there better explanation? Thank you.

oli-obk · 2024-01-29T16:37:14Z

Ah yea, that makes sense :/ Not sure what the best way forward is.

I guess ideally we'd make the expansion somehow signal that it is referring to code copied from non-expanded code (in contrast to fully generated code, which is what the original issue was about).

That would require modifying our derives, and would not help for proc macro derives, for which I don't know how we can address it at this point.

long-long-float · 2024-02-02T03:48:20Z

@oli-obk
If the suggest position(Span) is within the original code's derive(...) in the original code, it can be suppressed. However it is possible that there is a correct suggestion even within the derive(...).

I think the essence of this problem is that it is necessary to determine whether a suggestion is grammatically correct or not. It would be impossible to determine that using only in_derive_expansion. Even the simple method of judging in the first suggest_cast may suppress a correct suggestion.

As a different approach, I have an idea to check if the code to which the suggestion is applied is grammatically correct, and suppress it if it is not. I would like to implement a PoC if I can afford it.

oli-obk · 2024-02-02T14:33:44Z

As a different approach, I have an idea to check if the code to which the suggestion is applied is grammatically correct, and suppress it if it is not. I would like to implement a PoC if I can afford it.

that sound scary. I don't think we should go down that route (if I understand correctly what you're trying to do)

I think the essence of this problem is that it is necessary to determine whether a suggestion is grammatically correct or not. It would be impossible to determine that using only in_derive_expansion. Even the simple method of judging in the first suggest_cast may suppress a correct suggestion.

that sounds doable. we store information about the derive's span in the expansion info. So we could check if it overlaps with that.

long-long-float · 2024-02-04T08:29:51Z

@oli-obk
On second thought, your suggested method seems to solve the issue.
However, let me confirm that I did not fully understand how to implement it.

In derive exapantion, we store information about the derive's span in the expansion info ExpnData .

// in ExpnData
/// Whether this ExpnData is between an attribute `derive(...)`
bool in_derive_attribute,

In push_suggestion , we check whether spans are in derive(...) using the information, not using in_derive_expansion
- We just check in_derive_attribute

I don't (can't) take care about proc-macro now.

oli-obk · 2024-02-04T09:55:54Z

That sounds good to me. We can figure out proc macros later, doesn't have to be in this PR

long-long-float · 2024-02-04T15:14:24Z

Thank you! I will implement by this method.

long-long-float · 2024-02-10T17:45:50Z

Thank you for your review. I added a test for the issue and fix to use for-loop.

oli-obk · 2024-02-10T18:26:47Z

Great! Let's land this then

@bors r+ rollup

bors · 2024-02-10T18:26:50Z

📌 Commit 1e59e66 has been approved by oli-obk

It is now in the queue for this repository.

oli-obk · 2024-02-10T18:48:07Z

@bors r-

I've been told about a possible perf impact that previous attempts have had

oli-obk · 2024-02-10T18:48:20Z

@bors try @rust-timer queue

…-in-derive-macro, r=<try> Suppress suggestions in derive macro close rust-lang#118809 I suppress warnings inside derive macros. For example, the compiler emits following error by a program described in rust-lang#118809 (comment) with a suggestion that indicates invalid syntax. ``` error[E0308]: `?` operator has incompatible types --> src/main.rs:3:17 | 3 | #[derive(Debug, Deserialize)] | ^^^^^^^^^^^ expected `u32`, found `u64` | = note: `?` operator cannot convert from `u64` to `u32` = note: this error originates in the derive macro `Deserialize` (in Nightly builds, run with -Z macro-backtrace for more info) help: you can convert a `u64` to a `u32` and panic if the converted value doesn't fit | 3 | #[derive(Debug, Deserialize.try_into().unwrap())] | ++++++++++++++++++++ For more information about this error, try `rustc --explain E0308`. error: could not compile `serde_test` (bin "serde_test") due to 2 previous errors ``` In this PR, suggestions to cast are suppressed. ``` error[E0308]: `?` operator has incompatible types --> src/main.rs:3:17 | 3 | #[derive(Debug, Deserialize)] | ^^^^^^^^^^^ expected `u32`, found `u64` | = note: `?` operator cannot convert from `u64` to `u32` = note: this error originates in the derive macro `Deserialize` (in Nightly builds, run with -Z macro-backtrace for more info) For more information about this error, try `rustc --explain E0308`. error: could not compile `serde_test` (bin "serde_test") due to 2 previous errors ```

bors · 2024-02-10T18:49:31Z

⌛ Trying commit 1e59e66 with merge 7ad2516...

bors · 2024-02-10T20:18:33Z

☀️ Try build successful - checks-actions
Build commit: 7ad2516 (7ad25162fa95115bd7470b9314ba8b2e4edaa45e)

rust-timer · 2024-02-10T21:32:55Z

Finished benchmarking commit (7ad2516): 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)	-	-	0
Improvements ✅ (primary)	-2.9%	[-2.9%, -2.9%]	1
Improvements ✅ (secondary)	-3.7%	[-5.1%, -2.3%]	2
All ❌✅ (primary)	-2.9%	[-2.9%, -2.9%]	1

Cycles

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

Binary size

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

Bootstrap: 667.259s -> 666.873s (-0.06%)
Artifact size: 308.02 MiB -> 308.08 MiB (0.02%)

oli-obk · 2024-02-10T21:40:33Z

@bors r+ rollup

bors · 2024-02-10T21:40:35Z

📌 Commit 1e59e66 has been approved by oli-obk

It is now in the queue for this repository.

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#119213 (simd intrinsics: add simd_shuffle_generic and other missing intrinsics) - rust-lang#120272 (Suppress suggestions in derive macro) - rust-lang#120773 (large_assignments: Allow moves into functions) - rust-lang#120874 (Take empty `where` bounds into account when suggesting predicates) - rust-lang#120882 (interpret/write_discriminant: when encoding niched variant, ensure the stored value matches) - rust-lang#120883 (interpret: rename ReadExternStatic → ExternStatic) - rust-lang#120890 (Adapt `llvm-has-rust-patches` validation to take `llvm-config` into account.) - rust-lang#120895 (don't skip coercions for types with errors) - rust-lang#120896 (Print kind of coroutine closure) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#120272 - long-long-float:suppress-suggestions-in-derive-macro, r=oli-obk Suppress suggestions in derive macro close rust-lang#118809 I suppress warnings inside derive macros. For example, the compiler emits following error by a program described in rust-lang#118809 (comment) with a suggestion that indicates invalid syntax. ``` error[E0308]: `?` operator has incompatible types --> src/main.rs:3:17 | 3 | #[derive(Debug, Deserialize)] | ^^^^^^^^^^^ expected `u32`, found `u64` | = note: `?` operator cannot convert from `u64` to `u32` = note: this error originates in the derive macro `Deserialize` (in Nightly builds, run with -Z macro-backtrace for more info) help: you can convert a `u64` to a `u32` and panic if the converted value doesn't fit | 3 | #[derive(Debug, Deserialize.try_into().unwrap())] | ++++++++++++++++++++ For more information about this error, try `rustc --explain E0308`. error: could not compile `serde_test` (bin "serde_test") due to 2 previous errors ``` In this PR, suggestions to cast are suppressed. ``` error[E0308]: `?` operator has incompatible types --> src/main.rs:3:17 | 3 | #[derive(Debug, Deserialize)] | ^^^^^^^^^^^ expected `u32`, found `u64` | = note: `?` operator cannot convert from `u64` to `u32` = note: this error originates in the derive macro `Deserialize` (in Nightly builds, run with -Z macro-backtrace for more info) For more information about this error, try `rustc --explain E0308`. error: could not compile `serde_test` (bin "serde_test") due to 2 previous errors ```

rustbot assigned oli-obk Jan 23, 2024

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 Jan 23, 2024

long-long-float force-pushed the suppress-suggestions-in-derive-macro branch from 2dedac8 to 8ac80a9 Compare January 24, 2024 04:10

This comment has been minimized.

Sign in to view

oli-obk 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 Jan 29, 2024

long-long-float added 3 commits February 6, 2024 00:32

Suppress suggestions in derive macro

87ea0d7

Check in push_suggestion

4909258

Update UI tests that generates duplicated result

8cc5084

long-long-float force-pushed the suppress-suggestions-in-derive-macro branch from b4d2e2f to 8cc5084 Compare February 5, 2024 15:33

This comment has been minimized.

Sign in to view

Check with overlaps_or_adjacent

4e7941c

Fix to use for loop

1e59e66

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 Feb 10, 2024

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 Feb 10, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 10, 2024

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 10, 2024

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 Feb 10, 2024

matthiaskrgr mentioned this pull request Feb 11, 2024

Rollup of 9 pull requests #120903

Merged

bors merged commit e525bc9 into rust-lang:master Feb 11, 2024
12 checks passed

rustbot added this to the 1.78.0 milestone Feb 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suppress suggestions in derive macro #120272

Suppress suggestions in derive macro #120272

long-long-float commented Jan 23, 2024

rustbot commented Jan 23, 2024

Teapot4195 commented Jan 23, 2024

long-long-float commented Jan 24, 2024

oli-obk commented Jan 24, 2024

long-long-float commented Jan 24, 2024

This comment has been minimized.

oli-obk commented Jan 24, 2024

long-long-float commented Jan 25, 2024

oli-obk commented Jan 25, 2024

This comment has been minimized.

oli-obk commented Jan 29, 2024

long-long-float commented Jan 29, 2024

oli-obk commented Jan 29, 2024

long-long-float commented Feb 2, 2024

oli-obk commented Feb 2, 2024

long-long-float commented Feb 4, 2024

oli-obk commented Feb 4, 2024

long-long-float commented Feb 4, 2024

This comment has been minimized.

long-long-float commented Feb 10, 2024

oli-obk commented Feb 10, 2024

bors commented Feb 10, 2024

oli-obk commented Feb 10, 2024

oli-obk commented Feb 10, 2024

This comment has been minimized.

bors commented Feb 10, 2024

bors commented Feb 10, 2024

This comment has been minimized.

rust-timer commented Feb 10, 2024

oli-obk commented Feb 10, 2024

bors commented Feb 10, 2024

Suppress suggestions in derive macro #120272

Suppress suggestions in derive macro #120272

Conversation

long-long-float commented Jan 23, 2024

rustbot commented Jan 23, 2024

Teapot4195 commented Jan 23, 2024

long-long-float commented Jan 24, 2024

oli-obk commented Jan 24, 2024

long-long-float commented Jan 24, 2024

This comment has been minimized.

oli-obk commented Jan 24, 2024

long-long-float commented Jan 25, 2024

oli-obk commented Jan 25, 2024

This comment has been minimized.

oli-obk commented Jan 29, 2024

long-long-float commented Jan 29, 2024

oli-obk commented Jan 29, 2024

long-long-float commented Feb 2, 2024

oli-obk commented Feb 2, 2024

long-long-float commented Feb 4, 2024

oli-obk commented Feb 4, 2024

long-long-float commented Feb 4, 2024

This comment has been minimized.

long-long-float commented Feb 10, 2024

oli-obk commented Feb 10, 2024

bors commented Feb 10, 2024

oli-obk commented Feb 10, 2024

oli-obk commented Feb 10, 2024

This comment has been minimized.

bors commented Feb 10, 2024

bors commented Feb 10, 2024

This comment has been minimized.

rust-timer commented Feb 10, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

oli-obk commented Feb 10, 2024

bors commented Feb 10, 2024