-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Unify wording of resolve error #145399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Unify wording of resolve error #145399
Conversation
This PR modifies |
This comment has been minimized.
This comment has been minimized.
Remove "failed to resolve" and use the same format we use in other resolution errors "cannot find `name`". ``` error[E0433]: cannot find `nonexistent` in `existent` --> $DIR/custom_attr_multisegment_error.rs:5:13 | LL | #[existent::nonexistent] | ^^^^^^^^^^^ could not find `nonexistent` in `existent` ```
fd49d2a
to
298dec7
Compare
@@ -1,4 +1,4 @@ | |||
error[E0433]: failed to resolve: partially resolved path in a macro | |||
error[E0433]: cannot find `bar` in `Foo` | |||
--> $DIR/extern-macro.rs:5:13 | |||
| | |||
LL | let _ = Foo::bar!(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a diagnostic that needs improvement regardless: we need to explain that Foo
is a type and that macros can't be in them. It would be particularly bad if Foo
had an associated item bar
as well.
@@ -41,7 +41,7 @@ LL - bar: st::cell::Cell<bool> | |||
LL + bar: cell::Cell<bool> | |||
| | |||
|
|||
error[E0433]: failed to resolve: function `bar` is not a crate or module | |||
error[E0433]: cannot find `bar` in this scope |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I found I'd opened a separate PR already with some of these changes, and you had a comment about this change in particular
I agree that this isn't an improvement, but the primary label does still mention the intended issue. I'll see over the weekend what I can do about this.
The job Click to see the possible cause of the failure (guessed by this bot)
|
@@ -7,6 +7,6 @@ macro_rules! cbor_map { | |||
fn main() { | |||
cbor_map! { #[test(test)] 4}; | |||
//~^ ERROR removing an expression is not supported in this position | |||
//~| ERROR attribute must be of the form `#[test]` | |||
//~| ERROR attribute must be of the form |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary change.
} | ||
} | ||
_ => format!("this scope"), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is evaluated on a good path, and very often, but we don't actually need the result unless PathResult::failed
is called with finalize == true
.
PathResult::failed
already takes a closure that is only evaluated when necessary, this logic can also be called from that closure.
Remove "failed to resolve" from the main error message and use the same format we use in other resolution errors "cannot find
name
":The intent behind this is to end up with all resolve errors eventually be on the form of
A category of errors that is interest are those that involve keywords. For example:
and
For these the label provides the actual help, while the message is less informative beyond telling you "couldn't find
name
".This is an off-shoot of #126810 and #128086, a subset of the intended changes there with review comments applied.
r? @petrochenkov