-
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
Implement tool_attributes feature (RFC 2103) #50030
Conversation
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
I'll likely leave reviewing this until the next weekend.
Could you leave this for a separate PR? |
Marking as waiting-on-author until tests are fixed and Travis is green. |
☔ The latest upstream changes (presumably #50120) made this pull request unmergeable. Please resolve the merge conflicts. |
Yeah sure!
Somehow the |
src/libsyntax/attr.rs
Outdated
let span = span.with_hi(segments.last().unwrap().ident.span.hi()); | ||
ast::Path { span, segments } | ||
} else { | ||
ident.span = span; |
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 hack I would like to change before this PR lands, but I'm not sure what the right thing to do here would be. Apparently Token::Ident(ident, _)
ident
s have the DUMMY_SP
as their Span
(Is this intended?). To create the Path
s correctly we need the correct Span
, which is the Span
of TokenTree::Token(span, _)
.
But the Path(Segment)::from_ident()
function only takes an Ident
since the recent changes. Should I add a function Path(Segment)::from_ident_span()
? Or what would be the right thing to do here?
@@ -714,6 +714,22 @@ pub trait PrintState<'a> { | |||
Ok(()) | |||
} | |||
|
|||
fn print_attribute_path(&mut self, path: &ast::Path) -> io::Result<()> { |
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 pretty much a copy of State::print_path
, as already mentioned here comment.
Should I move the print_path
method to the PrintState
trait? Or use self.writer().word(&path_to_string(path))
?
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Travis is green now 👍 |
@flip1995
That's not urgent, but we'll need to squash at least before merging. |
Running |
What do you mean by "to create the Paths correctly"? If |
Tentatively r=me after replacing |
☔ The latest upstream changes (presumably #50317) made this pull request unmergeable. Please resolve the merge conflicts. |
The
Thanks! I missed this method! |
@bors r=petrochenkov |
@flip1995: 🔑 Insufficient privileges: Not in reviewers |
@bors r+ |
📌 Commit 28bbd40 has been approved by |
⌛ Testing commit 28bbd404b2fe4f0374f6c2cf50d9250494e1e1ca with merge 1ca0804c053742f49b8a8fd270d9c64261963e9f... |
📌 Commit 84f4508 has been approved by |
⌛ Testing commit 84f4508 with merge e487a7ca907bcc41eca1a810515c75ab458196cc... |
💔 Test failed - status-travis |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
☀️ Test successful - status-appveyor, status-travis |
Replaces older, clunky syntax. rust-lang/rust#50030
Replaces older, clunky syntax. rust-lang/rust#50030
Do not accidentally treat multi-segment meta-items as single-segment Fixes rust-lang#55168 and many other regressions from rust-lang#50030 Basically, attributes like `#[any::prefix::foo]` were commonly interpreted as `#[foo]` due to `name()` successfully returning the last segment (this applies to nested things as well `#[attr(any::prefix::foo)]`).
Do not accidentally treat multi-segment meta-items as single-segment Fixes rust-lang#55168 and many other regressions from rust-lang#50030 Basically, attributes like `#[any::prefix::foo]` were commonly interpreted as `#[foo]` due to `name()` successfully returning the last segment (this applies to nested things as well `#[attr(any::prefix::foo)]`).
Do not accidentally treat multi-segment meta-items as single-segment Fixes #55168 and many other regressions from #50030 Basically, attributes like `#[any::prefix::foo]` were commonly interpreted as `#[foo]` due to `name()` successfully returning the last segment (this applies to nested things as well `#[attr(any::prefix::foo)]`).
cc #44690
This is currently just a rebased and compiling (hopefully) version of #47773.
Let's see if travis likes this. I will add the implementation for
tool_lints
this week.