Skip to content

rustc_on_unimplemented is not std-agnostic #112923

@WaffleLapkin

Description

@WaffleLapkin
Member

If rustc_on_unimplemented contains a path mentioning std::, it won't work in #![no_std] crates, even if the type exists in the core. Similarly if you use core:: path instead, the diagnostic will only work in #![no_std] crates.

As an example take this annotation for ? desugaring:

on(
all(
from_desugaring = "QuestionMark",
_Self = "std::option::Option<T>",
R = "std::result::Result<T, E>",
),
message = "the `?` operator can only be used on `Option`s, not `Result`s, \
in {ItemContext} that returns `Option`",
label = "use `.ok()?` if you want to discard the `{R}` error information",
parent_label = "this function returns an `Option`"
),

It should (and does) trigger for the following code:

fn f() -> Option<()> {
    Err::<(), _>(())?;
    None
}
error[E0277]: the `?` operator can only be used on `Option`s, not `Result`s, in a function that returns `Option`
 --> src/lib.rs:2:21
  |
1 | fn f() -> Option<()> {
  | -------------------- this function returns an `Option`
2 |     Err::<(), _>(())?;
  |                     ^ use `.ok()?` if you want to discard the `Result<Infallible, ()>` error information
  |
  = help: the trait `FromResidual<Result<Infallible, ()>>` is not implemented for `Option<()>`
  = help: the following other types implement trait `FromResidual<R>`:
            <Option<T> as FromResidual<Yeet<()>>>
            <Option<T> as FromResidual>

(play;note the use .ok()? label)

However, if you add #![no_std] the label disappears:

#![no_std]

fn f() -> Option<()> {
    Err::<(), _>(())?;
    None
}
error[E0277]: the `?` operator can only be used in a function that returns `Result` or `Option` (or another type that implements `FromResidual`)
 --> src/lib.rs:4:21
  |
3 | fn f() -> Option<()> {
  | -------------------- this function should return `Result` or `Option` to accept `?`
4 |     Err::<(), _>(())?;
  |                     ^ cannot use the `?` operator in a function that returns `Option<()>`
  |
  = help: the trait `FromResidual<Result<Infallible, ()>>` is not implemented for `Option<()>`
  = help: the following other types implement trait `FromResidual<R>`:
            <Option<T> as FromResidual<Yeet<()>>>
            <Option<T> as FromResidual>

(play; the error message is also misleading, the function does return option)


rustc in general is not great at being std-agnostic in diagnostics, and this is just another annoying example of that.

Meta

rustc --version --verbose:

rustc 1.70.0 (90c541806 2023-05-31)
binary: rustc
commit-hash: 90c541806f23a127002de5b4038be731ba1458ca
commit-date: 2023-05-31
host: aarch64-unknown-linux-gnu
release: 1.70.0
LLVM version: 16.0.2

Activity

added
A-diagnosticsArea: Messages for errors, warnings, and lints
C-bugCategory: This is a bug.
D-confusingDiagnostics: Confusing error or lint that should be reworked.
D-diagnostic-infraDiagnostics: Issues that affect all diagnostics, or relate to the diagnostic machinery itself.
on Jun 22, 2023
estebank

estebank commented on Jun 23, 2023

@estebank
Contributor

The filters can be extended to also account for core:: paths, by mixing any and all in the condition. Sadly that solution has to be peppered throughout all the annotations in std, but is a solution we can implement today.

WaffleLapkin

WaffleLapkin commented on Jun 26, 2023

@WaffleLapkin
MemberAuthor

@estebank I think we should still (eventually) investigate how we can make rustc_on_unimplemented more reliable in general, but for the time being your solution is a step forward.

I'll mark this as E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. + E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. , so someone can work on this.

Instructions

If you want to work on this, use @rustbot claim to assign this issue to yourself, so that everyone knows someone is already working on it and won't duplicate the work.

Then find all mentions of rustc_on_unimplemented in the standard libraries (./library) and for each path that mentions std::, replace it with any(... = "core::...", ... = "std::...") if the type is present in the core or with any(... = "alloc::...", ... = "std::...") if the type is present in alloc (or with any(... = "core::...", ... = "alloc::...", ... = "std::...") in the rare case that the type is present in both core and alloc).

For example:

--- a/library/core/src/convert/mod.rs
+++ b/library/core/src/convert/mod.rs
@@ -534,7 +534,10 @@ pub trait Into<T>: Sized {
 #[rustc_on_unimplemented(on(
     all(
         _Self = "&str",
-        T = "std::string::String"
+        any(
+            T = "alloc::string::String",
+            T = "std::string::String",
+        )
     ),
     note = "to coerce a `{T}` into a `{Self}`, use `&*` as a prefix",
 ))]
added
E-easyCall for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
E-mentorCall for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
on Jun 26, 2023
Rageking8

Rageking8 commented on Jun 26, 2023

@Rageking8
Contributor

@rustbot claim

added 2 commits that reference this issue on Jun 27, 2023
b6cf0ac
b6b5ae6
estebank

estebank commented on Jun 27, 2023

@estebank
Contributor

investigate how we can make rustc_on_unimplemented more reliable in general, but for the time being your solution is a step forward.

I believe that the way forward is with annotations in the types themselves that influence the E0277 errors. This design will have to be explored as part of the #[diagnostic::*] namespace effort, as there it will be required for anyone that needs to customize errors caused by local types and foreign traits.

added a commit that references this issue on Jun 29, 2023
f35f213
DogPawHat

DogPawHat commented on Aug 25, 2023

@DogPawHat
Contributor

@rustbot claim

removed
E-easyCall for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
E-mentorCall for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
on Aug 25, 2023
WaffleLapkin

WaffleLapkin commented on Aug 25, 2023

@WaffleLapkin
MemberAuthor

@estebank annotating the types seems backwards, as it would require annotating all types, at which point you re-invented paths, or users won't be able to add "#[diagnostic::on_unimplemented]" with a type that wasn't annotated in some dependency (including std)...

Couldn't we instead resolve paths in "on_unimpleneted" annotations? So that std::option::Option and core::option::Option are the same, in the same way they are the same in the language?

added a commit that references this issue on Aug 26, 2023
c3668d3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

A-diagnosticsArea: Messages for errors, warnings, and lintsC-bugCategory: This is a bug.D-confusingDiagnostics: Confusing error or lint that should be reworked.D-diagnostic-infraDiagnostics: Issues that affect all diagnostics, or relate to the diagnostic machinery itself.

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

    Development

    No branches or pull requests

      Participants

      @estebank@DogPawHat@WaffleLapkin@Rageking8

      Issue actions

        `rustc_on_unimplemented` is not std-agnostic · Issue #112923 · rust-lang/rust