-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
Add new unstable attribute: #[export_visibility = ...].
#151431
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: main
Are you sure you want to change the base?
Conversation
|
Some changes occurred in compiler/rustc_hir/src/attrs cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_passes/src/check_attr.rs cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_attr_parsing |
|
r? @chenyukang rustbot has assigned @chenyukang. Use |
|
i had a quick look, mostly looks good, but i'd like to maybe @JonathanBrouwer take a look on this as well, i may overlooked something r? JonathanBrouwer |
|
Would like to take a look, will do so tomorrow :) |
This comment has been minimized.
This comment has been minimized.
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 minor questions, looks good on a high level :)
| return None; | ||
| }; | ||
| let Ok(visibility) = ExportVisibilityAttrValue::from_str(sv.as_str()) else { | ||
| cx.emit_err(InvalidExportVisibility { |
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 this use cx.expected_specific_argument_strings instead?
(not sure)
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.
I am also not sure. Right now the new attribute expects a string literal as an argument (e.g. #[export_visibility = "target_default"] - this is the syntax that has been used so far by the RFC) . And it seems that expected_specific_argument_strings is meant to be used with symbols rather than with string literals (e.g. #[export_visibility = target_default]). Do you think the new attribute should use the latter syntax?
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.
I think the attribute should continue to use the #[export_visibility = "target_default"] syntax.
If expected_specific_argument_strings does not give the proper suggestions, could you make a new method that does?
| EiiImpls(..) => No, | ||
| ExportName { .. } => Yes, | ||
| ExportStable => No, | ||
| ExportVisibility { .. } => Yes, |
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.
Is exporting this attribute needed? (Not sure, genuine question)
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.
I am also not sure. I think if #[export_name = ...] needs to be exported, then so does #[export_visibility = ...] although I can't really convincingly point to a specific scenario where this is needed.
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.
I think export_name is exported for rustdoc, tho not sure. I suppose it can't hurt to set this to Yes for now to be sure, it's a pretty niche attribute anyways
|
Reminder, once the PR becomes ready for a review, use |
2043b51 to
8dc8f60
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
8dc8f60 to
6ccbf57
Compare
88ddd87 to
a741ebc
Compare
|
|
||
| fn main() { | ||
| // Compile a cdylib | ||
| rustc().input("foo.rs").arg("-Zdefault-visibility=hidden").run(); |
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.
Maybe the test should build one version with the flag and one without?
| let out = | ||
| llvm_readobj().arg("--dyn-symbols").input(dynamic_lib_name("foo")).run().stdout_utf8(); | ||
|
|
||
| // `#[no_mangle]` with no other attributes means: publicly exported function. | ||
| assert!(out.contains("test_fn_no_attr"), "{out}"); | ||
|
|
||
| // `#[no_mangle]` with `#[export_visibility = "target_default"]` means | ||
| // that visibility is inherited from `-Zdefault-visibility=hidden`. | ||
| assert!(!out.contains("test_fn_export_visibility_asks_for_target_default"), "{out}"); |
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.
The run_make_support::object module may be helpful here so it gives a symbol dump on failure:
assert_eq!(object_contains_all_symbols(path, ["test_fn_no_attr"]), ContainsAllSymbolsOutcome::Ok);
assert!(!object_contains_any_symbols(path, ["test_fn_export_visibility_asks_for_target_default"]));Or, more complicated but maybe useful if there are more properties to check down the line, inspect the file directly.
use run_make_support::object;
let obj = object::File::parse(path).unwrap();
let s = obj.symbol_by_name("test_fn_no_attr").unwrap();
assert!(s.is_global());
assert!(!s.is_undefined());
// ...a741ebc to
aa26225
Compare
This PR is an implementation of the RFC tracked in #151425