don't encode only locally used attrs by lcnr · Pull Request #95562 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation70 Commits8 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Part of rust-lang/compiler-team#505.
We now filter builtin attributes before encoding them in the crate metadata in case they should only be used in the local crate. To prevent accidental misuse get_attrs now requires the caller to state which attribute they are interested in. For places where that isn't trivially possible, I've added a method fn get_attrs_unchecked which I intend to remove in a followup PR.
After this pull request landed, we can then slowly move all attributes to only be used in the local crate while being certain that we don't accidentally try to access them from extern crates.
Relevant to the compiler team, which will review and decide on the PR/issue.
label
r? @estebank
(rust-highfive has picked a reviewer for you, use r? to override)
Awaiting bors try build completion.
@rustbot label: +S-waiting-on-perf
⌛ Trying commit b98bb027949f170814d85f3cc530638351349065 with merge 72c6b3095acbcb061511d821eb46205167a0f5f2...
☀️ Try build successful - checks-actions
Build commit: 72c6b3095acbcb061511d821eb46205167a0f5f2 (72c6b3095acbcb061511d821eb46205167a0f5f2)
Finished benchmarking commit (72c6b3095acbcb061511d821eb46205167a0f5f2): comparison url.
Summary:
- Primary benchmarks: 🎉 relevant improvements found
- Secondary benchmarks: no relevant changes found. 10 results were found to be statistically significant but too small to be relevant.
| Regressions 😿 (primary) | Regressions 😿 (secondary) | Improvements 🎉 (primary) | Improvements 🎉 (secondary) | All 😿 🎉 (primary) | |
|---|---|---|---|---|---|
| count1 | 0 | 0 | 14 | 10 | 14 |
| mean2 | N/A | N/A | -51.5% | -0.3% | -51.5% |
| max | N/A | N/A | -79.2% | -0.3% | 0.0% |
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.
Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.
@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression
Footnotes
Unfortunately, I believe the performance gains seen here are not real. The benchmark showing such large improvements was switched out for another version that uses a different feature set and compiles in general roughly twice as fast. You can find the discussion about this here
r? @davidtwco
Let's trigger another perf run to see what the actual improvements are (excluding the big improvements from the last run, probably going to be a modest improvement).
@bors try @rust-timer queue
Awaiting bors try build completion.
@rustbot label: +S-waiting-on-perf
⌛ Trying commit b98bb027949f170814d85f3cc530638351349065 with merge ad580c3d5edddb91a7378991ba50ec39bd6676ef...
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, r=me if perf results are good and you want to land this
☀️ Try build successful - checks-actions
Build commit: ad580c3d5edddb91a7378991ba50ec39bd6676ef (ad580c3d5edddb91a7378991ba50ec39bd6676ef)
Finished benchmarking commit (ad580c3d5edddb91a7378991ba50ec39bd6676ef): comparison url.
Summary:
- Primary benchmarks: no relevant changes found. 2 results were found to be statistically significant but too small to be relevant.
- Secondary benchmarks: 🎉 relevant improvements found
| Regressions 😿 (primary) | Regressions 😿 (secondary) | Improvements 🎉 (primary) | Improvements 🎉 (secondary) | All 😿 🎉 (primary) | |
|---|---|---|---|---|---|
| count1 | 0 | 0 | 2 | 8 | 2 |
| mean2 | N/A | N/A | -0.3% | -0.6% | -0.3% |
| max | N/A | N/A | -0.4% | -3.5% | 0.0% |
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.
Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.
@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression
Footnotes
Could we have a debug assertion to check that there is no attempt made to get an only_local attribute from a foreign crate?
| @@ -359,7 +378,7 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[ |
|---|
| ungated!(panic_handler, Normal, template!(Word), WarnFollowing), // RFC 2070 |
| // Code generation: |
| ungated!(inline, Normal, template!(Word, List: "always|never"), FutureWarnFollowing), |
| ungated!(inline, Normal, template!(Word, List: "always|never"), FutureWarnFollowing, @only_local: true), |
| ungated!(cold, Normal, template!(Word), WarnFollowing), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be skipped too I think.
A job failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request
This was referenced
May 12, 2022
Finished benchmarking commit (481db40): comparison url.
Summary:
- Primary benchmarks: 🎉 relevant improvements found
- Secondary benchmarks: 🎉 relevant improvements found
| Regressions 😿 (primary) | Regressions 😿 (secondary) | Improvements 🎉 (primary) | Improvements 🎉 (secondary) | All 😿 🎉 (primary) | |
|---|---|---|---|---|---|
| count1 | 0 | 0 | 22 | 23 | 22 |
| mean2 | N/A | N/A | -0.8% | -1.8% | -0.8% |
| max | N/A | N/A | -2.7% | -2.8% | -2.7% |
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.
@rustbot label: -perf-regression
Footnotes
celinval added a commit to celinval/kani-dev that referenced this pull request
xFrednet pushed a commit to xFrednet/rust that referenced this pull request
don't encode only locally used attrs
Part of rust-lang/compiler-team#505.
We now filter builtin attributes before encoding them in the crate metadata in case they should only be used in the local crate. To prevent accidental misuse get_attrs now requires the caller to state which attribute they are interested in. For places where that isn't trivially possible, I've added a method fn get_attrs_unchecked which I intend to remove in a followup PR.
After this pull request landed, we can then slowly move all attributes to only be used in the local crate while being certain that we don't accidentally try to access them from extern crates.
celinval added a commit to model-checking/kani that referenced this pull request
- Update rust toolchain to 2022-05-17
Status: Compilation succeeds but regression fails due to new intrinsic.
Relevant changes:
- Implement new intrinsic ptr_offset_from_unsigned
This new intrinsic is used in many different places in the standard library and it was failing some tests for vectors.
- Apply suggestions from code review
Co-authored-by: Adrian Palacios 73246657+adpaco-aws@users.noreply.github.com
- Address PR comments
- Fix order of checks.
- Improve error message.
- Add comments to the new tests.
Co-authored-by: Adrian Palacios 73246657+adpaco-aws@users.noreply.github.com
Comment on lines +2187 to 2194
| // FIXME(@lcnr): Remove this function. |
|---|
| pub fn get_attrs_unchecked(self, did: DefId) -> &'tcx [ast::Attribute] { |
| if let Some(did) = did.as_local() { |
| self.hir().attrs(self.hir().local_def_id_to_hir_id(did)) |
| } else { |
| self.item_attrs(did) |
| } |
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @lcnr , if and when you get around to removing this, what will be the recommended way to query for multi-component attributes?
We have a compiler tool with a late lint pass that checks for the presence of some foo::bar attribute on items, and it seems like if you get rid of this function, I won't be able to query for that since PartialEq<Symbol> for Path wants the path to have exactly one component.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if i understand you correctly you should still be able to fetch all attributes with foo and then check for the ones with bar 🤔
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. If I get_attrs with attr = Symbol("foo"), then that calls a.has_name("foo") for each attribute, which does a.path == "foo", and that == wants path to have exactly one component:
| impl PartialEq<Symbol> for Path { |
|---|
| #[inline] |
| fn eq(&self, symbol: &Symbol) -> bool { |
| self.segments.len() == 1 && { self.segments[0].ident.name == *symbol } |
| } |
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah... so we should maybe add something like get_tool_attrs or something? 🤔 we definitely want to keep supporting this in some way '^^
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like that, yeah.
yvt added a commit to yvt/servo that referenced this pull request
…ed}`
rust-lang/rust#95562 renames the existing method get_attrs to
get_attrs_unchecked and introduces a new method in its former place.
The new method takes an attribute name and returns attributes of that
name. It also checks that, if the attribute name is marked as local-
only, the given DefId is local as well to prevent misuses. The old
method, now named get_attrs_unchecked, returns all attributes of a
given DefId; thus it's "unchecked" in the sense that it's up to the
callers to be certain whether the attributes they are looking for are
local-only.
The new get_attrs method lacks the support for attribute names with
more than one path component, which is why we can't just migrate to the
new get_attrs method here. Although get_attrs_unchecked is marked
for future removal in the compile source code, there's also a discussion
about supporting this use case.
yvt added a commit to yvt/servo that referenced this pull request
…ed}`
rust-lang/rust#95562 renames the existing method get_attrs to
get_attrs_unchecked and introduces a new method in its former place.
The new method takes an attribute name and returns attributes of that
name. It also checks that, if the attribute name is marked as local-
only, the given DefId is local as well to prevent misuses. The old
method, now named get_attrs_unchecked, returns all attributes of a
given DefId; thus it's "unchecked" in the sense that it's up to the
callers to be certain whether the attributes they are looking for are
local-only.
The new get_attrs method lacks the support for attribute names with
more than one path component, which is why we can't just migrate to the
new get_attrs method here. Although get_attrs_unchecked is marked
for future removal in the compile source code, there's a discussion
about supporting this use case.
Labels
This PR was explicitly merged by bors.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.