-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Reexport tests without polluting namespaces #52890
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
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 |
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 |
Interesting trick. Actually, can't this work in the opposite way? #[test]
fn foo() {}
=>
// The function itself is not changed
#[test]
fn foo() {}
// ... but the import is added.
#[some_marker_for_test_harness]
pub use foo as foo_gensym; This would make the logic cleaner on the expander side, but looks like it would make life harder for the test harness generator since it needs to check function signatures, process |
src/libsyntax/ext/expand.rs
Outdated
if self.tests_nameable && item.attrs.iter().any(|attr| is_test_or_bench(attr)) { | ||
let orig_vis = item.vis.clone(); | ||
|
||
// Publicize the item under gensymed name to avoid pollution |
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.
Could you add a comment showing the transformation in code, like in #52890 (comment)?
src/libsyntax/ext/expand.rs
Outdated
item = item.map(|mut item| { | ||
item.vis = respan(item.vis.span, ast::VisibilityKind::Public); | ||
item.ident = Ident::from_interned_str( | ||
item.ident.as_interned_str()).gensym(); |
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.
orig_ident = item.ident;
item.ident = item.ident.gensym();
src/libsyntax/ext/expand.rs
Outdated
let use_item = self.cx.item_use_simple_( | ||
item.ident.span, | ||
orig_vis, | ||
Some(Ident::from_interned_str(item.ident.as_interned_str())), |
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.
Some(orig_ident)
src/libsyntax/ext/expand.rs
Outdated
item.ident.span, | ||
orig_vis, | ||
Some(Ident::from_interned_str(item.ident.as_interned_str())), | ||
self.cx.path(item.ident.span, vec![Ident::from_str("self"), item.ident])); |
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.
keywords::SelfValue.ident()
@petrochenkov unfortunately, that change wouldn't work because we can't
into:
and then insert the appropriate The tricky thing is that we don't want to expand all
would become
which will fail. |
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 |
@petrochenkov Any idea why those changes broke the build? |
@djrenren |
Yeah, the errors make sense, I'm just confused why it passed before... |
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 |
@petrochenkov Should be all set now. |
@bors r+ |
📌 Commit 77f9aca has been approved by |
Reexport tests without polluting namespaces This should fix issue #52557. Basically now we gensym a new name for the test function and reexport that. That way the test function's reexport name can't conflict because it was impossible for the test author to write it down. We then use a `use` statement to expose the original name using the original visibility.
☀️ Test successful - status-appveyor, status-travis |
This should fix issue #52557.
Basically now we gensym a new name for the test function and reexport that.
That way the test function's reexport name can't conflict because it was impossible for the test author to write it down.
We then use a
use
statement to expose the original name using the original visibility.