-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Better user experience when attempting to call associated functions with dot notation #54308
Conversation
r? @zackmdavis (rust_highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
cc @estebank |
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.
Thanks for your contribution! Some revisions will be needed for tests to pass.
In addition to having the issue number in the commit message and PR description, a brief summary of the change (not more than 72 characters) is also useful for future readers of the repository history; you can edit the commit message with git commit --amend
.
Note: we don't call these "static method"s but rather "associated function"s. Methods are only those which you may call with dot notation. Therefore it may be confusing to call them "static method" in error messages. |
This comment has been minimized.
This comment has been minimized.
c4d9e53
to
50bf6f8
Compare
The build is still failing locally, it would be nice if you can have a second look ;) |
This comment has been minimized.
This comment has been minimized.
src/librustc_resolve/lib.rs
Outdated
err.span_suggestion_with_applicability( | ||
parent.span, | ||
"incorrect associated function syntax", | ||
format!("try {}::{}()?", |
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.
@dsciarra The third argument to the span_suggestion_
methods should be a code snippet to replace the span passed as the first argument. We're going to need something more like
err.span_suggestion_with_applicability(
parent.span,
"use `::` to access an associated function",
format!("{}::{}", path_str, path_assignment.ident),
Applicability::MaybeIncorrect
);
(But this probably isn't literally correct as written. In the UI test output, it looks like our span includes the call args (here, just ()
), which we don't want to replace.)
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.
If you need to get part of the literal code, you can use Codemap::span_to_snippet
to get it, like done here:
rust/src/librustc_typeck/check/demand.rs
Line 270 in f1694ea
if let Ok(src) = cm.span_to_snippet(sp) { |
50bf6f8
to
1811360
Compare
Is there a better way to write this? |
Other than the inline comment, it seems reasonable to me. |
1811360
to
0390736
Compare
@bors r+ rollup 🎉🎉🎉 |
📌 Commit 0390736 has been approved by |
Better user experience when attempting to call associated functions with dot notation Closes rust-lang#22692
Rollup of 13 pull requests Successful merges: - #53784 (Document that slices cannot be larger than `isize::MAX` bytes) - #54308 (Better user experience when attempting to call associated functions with dot notation) - #54488 (in which we include attributes in unused `extern crate` suggestion spans) - #54544 (Indicate how to move value out of Box in docs.) - #54623 (Added help message for `impl_trait_in_bindings` feature gate) - #54641 (A few cleanups and minor improvements to rustc/infer) - #54656 (Correct doc for WorkQueue<T>::pop().) - #54674 (update miri) - #54676 (Remove `-Z disable_ast_check_for_mutation_in_guard`) - #54679 (Improve bug! message for impossible case in Relate) - #54681 (Rename sanitizer runtime libraries on OSX) - #54708 (Make ./x.py help <cmd> invoke ./x.py <cmd> -h on its own) - #54713 (Add nightly check for tool_lints warning)
Closes #22692