Diagnostic suggests adding type [type error]

[camelid]

I think the suggestion is trying to suggest a type, but gets a TyKind::Error due to the type error in the body of the const. Ideally we would just not suggest a type, but there are probably a lot of other places that this issue occurs.

const FOO = "hello" + 1;

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
error[E0369]: cannot add `{integer}` to `&str`
 --> src/lib.rs:1:21
  |
1 | const FOO = "hello" + 1;
  |             ------- ^ - {integer}
  |             |
  |             &str

error: missing type for `const` item
 --> src/lib.rs:1:7
  |
1 | const FOO = "hello" + 1;
  |       ^^^ help: provide a type for the item: `FOO: [type error]`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0369`.
error: could not compile `playground`.

To learn more, run the command again with --verbose.

[oli-obk]

Fix instructions:

1. find the place where the provide a type for the item suggestion is emitted
2. prevent the suggestion from being created if the suggested type is
   • ty::Err
   • ty::FnDef (const FOO = main; fn main() {} gives that)
   • ty::Closure
   • possibly other variants of https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/enum.TyKind.html

[ghost]

Hi i am interesting in tackling this issue! This would be my first time contributing.

[oli-obk]

Cool! You can mark yourself as workin on this issue by writing @rustbot claim in it. Feel free to ask away with anything that is unclear. Or actually, just anything ^^ If you prefer chat over github messages, you can also reach out in zulip by opening a new thread.

[ghost]

@rustbot claim

[ghost]

The error is emitted here:

match tcx.sess.diagnostic().steal_diagnostic(span, StashKey::ItemNoType) {
        Some(mut err) => {
            // The parser provided a sub-optimal `HasPlaceholders` suggestion for the type.
            // We are typeck and have the real type, so remove that and suggest the actual type.
            err.suggestions.clear();
            err.span_suggestion(
                span,
                "provide a type for the item",
                format!("{}: {}", item_ident, ty),
                Applicability::MachineApplicable,
            )
            .emit();
        }
        None => {
            let mut diag = bad_placeholder_type(tcx, vec![span]);
            if !matches!(ty.kind(), ty::Error(_)) {
                diag.span_suggestion(
                    span,
                    "replace `_` with the correct type",
                    ty.to_string(),
                    Applicability::MaybeIncorrect,
                );
            }
            diag.emit();
        }
    }

within fn infer_placeholder_type, but I am confused by what is happening when tcx.sess.diagnostic().steal_diagnostic(span, StashKey::ItemNoType) matches with none, since it emits a different error. If it is unrelated to this issue, should I check for ty::Err and it variants from within Some(mut err)? I was thinking of matching on ty like so:

match ty.kind() {
        TyKind::Err | TyKind::FnDef | TyKind::Closure | ... => { }
        _ =>    {
                       err.suggestions.clear();
                       err.span_suggestion(
                           span,
                           "provide a type for the item",
                           format!("{}: {}", item_ident, ty),
                           Applicability::MachineApplicable,
                           )
                           .emit();
       }
}

[ghost]

I've managed to block the second error:

Some(mut err) => {
            // The parser provided a sub-optimal `HasPlaceholders` suggestion for the type.
            // We are typeck and have the real type, so remove that and suggest the actual type.
            match ty.kind() {
                ty::Error(_) => {
                    err.delay_as_bug();
                },
                _ => {
                    err.suggestions.clear();
                    err.span_suggestion(
                        span,
                        "provide a type for the item",
                        format!("{}: {}", item_ident, ty),
                        Applicability::MachineApplicable,
                    )
                    .emit();
                }
            }
        }

I used err.delay_as_bug() because I was getting an internal compiler error for an unused error. I saw emit_unless(&mut self, delay: bool) which does the same thing and seems more appropriate, but is delay_as_bug() the fn that I should use?

Using emit_unless(bool):

Some(mut err) => {
            err.suggestions.clear();
            err.span_suggestion(
                span,
                "provide a type for the item",
                format!("{}: {}", item_ident, ty),
                Applicability::MachineApplicable,
            )
            .emit_unless(
                match ty.kind() {
                    ty::Error(_) => {true},
                    _ => {false}  // Is _ ok here?
                }
            );
        }

[oli-obk]

I like the delay_as_bug() variant more.

Though I think we should add an ignore method that takes a DelaySpanBugEmitted, which we get from the ty::Error. What do you think @rust-lang/wg-diagnostics

[petar-dambovaliev]

cannot add {integer} to &str

Shouldn't it also be cannot add `&str` to `{integer}, same order as in the code?

[ghost]

@rustbot claim
reclaiming this issue with a new account

[oli-obk]

cannot add {integer} to &str

Shouldn't it also be cannot add `&str` to `{integer}, same order as in the code?

The order is indeed odd, but at least grammatically correct. If you have 5 + 6, you are adding 6 to 5. This along with another order swap around <SomeType as Add<OtherType>> being for an impl Add<OtherType> for SomeType and meaning an operation value_of_some_type + value_of_other_type. I think we could have an entire design meeting just around how to get all of that into a diagnostic without causing more confusion than the status quo. If you want you can open an issue about just the order confusion, I'll add the appropriate tags.

[estebank]

I like the delay_as_bug() variant more.

I'm concerned about the possibility of there being a case where [type error] is generated without any other error (but that shouldn't happen). The _ case is weird and I think we should emit the error in that case (although maybe not a suggestion, but not the end of the world). I like the possibility of using emit_unless:

        Some(mut err) => {
            err.suggestions.clear();
            err.span_suggestion(
                span,
                "provide a type for the item",
                format!("{}: {}", item_ident, ty),
                Applicability::MachineApplicable,
            )
            .emit_unless(matches!(ty.kind(), ty::Error(_)));
        }

[petar-dambovaliev]

cannot add {integer} to &str

Shouldn't it also be cannot add `&str` to `{integer}, same order as in the code?

The order is indeed odd, but at least grammatically correct. If you have 5 + 6, you are adding 6 to 5. This along with another order swap around <SomeType as Add<OtherType>> being for an impl Add<OtherType> for SomeType and meaning an operation value_of_some_type + value_of_other_type. I think we could have an entire design meeting just around how to get all of that into a diagnostic without causing more confusion than the status quo. If you want you can open an issue about just the order confusion, I'll add the appropriate tags.

I don't know enough about the language design space to be opening up meaningful issues. I was just making a comment. I don't know whether it's an issue or not and I certainly don't want to create issues that will be closed immediately.

[estebank]

I don't know enough about the language design space to be opening up meaningful issues.

@petar-dambovaliev That shouldn't stop you. Some times there are things that the team hasn't yet noticed and it is a good idea to rise the point. Other times tickets are duplicated but that is also a useful signal that can be a proxy for urgency.

I don't know whether it's an issue or not and I certainly don't want to create issues that will be closed immediately.

We treat these kind of issues seriously. We rarely immediately close tickets around here unless they are a duplicate (where we close with a link) or completely off-topic (where we point of more reasonable venues). If you still feel this comment isn't worth a new ticket but still want to have a conversation around it, feel free to visit https://internals.rust-lang.org/ and start a new thread.

IIRC, I am the one that wrote that message and I went back and forth between a sentence that made grammatical sense and one that looked like the provided code, and landed in the current message and a main label in the order of the code. Since the output has changed slightly and the later has been "lost". As the "culprit" of the current state, I welcome any conversation around this. That's how we improve :)

[henryboisdequin]

@rustbot claim

#82720 is open :)