don't encode only locally used attrs #95562

lcnr · 2022-04-01T13:06:09Z

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.

cc #94963 (comment)

rust-highfive · 2022-04-01T13:06:13Z

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

lcnr · 2022-04-01T13:06:36Z

@bors try @rust-timer queue

rust-timer · 2022-04-01T13:06:37Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-04-01T13:06:44Z

⌛ Trying commit b98bb027949f170814d85f3cc530638351349065 with merge 72c6b3095acbcb061511d821eb46205167a0f5f2...

bors · 2022-04-01T14:57:10Z

☀️ Try build successful - checks-actions
Build commit: 72c6b3095acbcb061511d821eb46205167a0f5f2 (72c6b3095acbcb061511d821eb46205167a0f5f2)

rust-timer · 2022-04-01T14:57:11Z

Queued 72c6b3095acbcb061511d821eb46205167a0f5f2 with parent 0677edc, future comparison URL.

rust-timer · 2022-04-01T16:10:21Z

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)
count¹	0	0	14	10	14
mean²	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

number of relevant changes ↩
the arithmetic mean of the percent change ↩

rylev · 2022-04-01T16:25:44Z

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

davidtwco · 2022-04-03T10:09:33Z

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

rust-timer · 2022-04-03T10:09:34Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-04-03T10:09:40Z

⌛ Trying commit b98bb027949f170814d85f3cc530638351349065 with merge ad580c3d5edddb91a7378991ba50ec39bd6676ef...

davidtwco

LGTM, r=me if perf results are good and you want to land this

bors · 2022-04-03T11:29:20Z

☀️ Try build successful - checks-actions
Build commit: ad580c3d5edddb91a7378991ba50ec39bd6676ef (ad580c3d5edddb91a7378991ba50ec39bd6676ef)

rust-timer · 2022-04-03T11:29:22Z

Queued ad580c3d5edddb91a7378991ba50ec39bd6676ef with parent 133859d, future comparison URL.

rust-timer · 2022-04-03T13:00:22Z

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)
count¹	0	0	2	8	2
mean²	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

number of relevant changes ↩
the arithmetic mean of the percent change ↩

compiler/rustc_metadata/src/rmeta/encoder.rs

compiler/rustc_middle/src/ty/mod.rs

bjorn3 · 2022-04-04T10:25:13Z

Could we have a debug assertion to check that there is no attempt made to get an only_local attribute from a foreign crate?

bjorn3 · 2022-04-04T15:30:46Z

compiler/rustc_feature/src/builtin_attrs.rs

@@ -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),


This can be skipped too I think.

rust-log-analyzer · 2022-05-12T10:19:32Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2022-05-12T10:22:02Z

💔 Test failed - checks-actions

lcnr · 2022-05-12T11:47:05Z

@bors retry

bors · 2022-05-12T12:48:34Z

⌛ Testing commit ebf9583 with merge 481db40...

bors · 2022-05-12T15:07:29Z

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 481db40 to master...

rust-highfive · 2022-05-12T15:08:03Z

📣 Toolstate changed by #95562!

Tested on commit 481db40.
Direct link to PR: #95562

💔 miri on windows: test-pass → build-fail (cc @eddyb @RalfJung @oli-obk).
💔 miri on linux: test-pass → build-fail (cc @eddyb @RalfJung @oli-obk).

@eddyb

Tested on commit rust-lang/rust@481db40. Direct link to PR: <rust-lang/rust#95562> 💔 miri on windows: test-pass → build-fail (cc @eddyb @RalfJung @oli-obk). 💔 miri on linux: test-pass → build-fail (cc @eddyb @RalfJung @oli-obk).

rust-timer · 2022-05-12T16:46:04Z

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)
count¹	0	0	22	23	22
mean²	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

number of relevant changes ↩
the arithmetic mean of the percent change ↩

Status: Compilation succeeds but regression fails due to new intrinsic. Relevant changes: - rust-lang/rust#95837 - rust-lang/rust#95562 - rust-lang/rust#96883

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. cc rust-lang#94963 (comment)

* Update rust toolchain to 2022-05-17 Status: Compilation succeeds but regression fails due to new intrinsic. Relevant changes: - rust-lang/rust#95837 - rust-lang/rust#95562 - rust-lang/rust#96883 * 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>

BGR360 · 2022-07-05T23:16:39Z

compiler/rustc_middle/src/ty/mod.rs

+    // 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)
        }
    }


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.

if i understand you correctly you should still be able to fetch all attributes with foo and then check for the ones with bar 🤔

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:

rust/compiler/rustc_ast/src/ast.rs

Lines 103 to 108 in ebf9583

impl PartialEq<Symbol> for Path {

#[inline]

fn eq(&self, symbol: &Symbol) -> bool {

self.segments.len() == 1 && { self.segments[0].ident.name == *symbol }

}

}

ah... so we should maybe add something like get_tool_attrs or something? 🤔 we definitely want to keep supporting this in some way '^^

Something like that, yeah.

…ed}` [rust-lang/rust#95562][1] 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][2] this use case. [1]: rust-lang/rust#95562 [2]: https://github.com/rust-lang/rust/pull/95562/files#r915537557

…ed}` [rust-lang/rust#95562][1] 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][2] this use case. [1]: rust-lang/rust#95562 [2]: https://github.com/rust-lang/rust/pull/95562/files#r915537557

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 1, 2022

rust-highfive assigned estebank Apr 1, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 1, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 1, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 1, 2022

rust-highfive assigned davidtwco and unassigned estebank Apr 3, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 3, 2022

davidtwco approved these changes Apr 3, 2022

View reviewed changes

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 3, 2022

bjorn3 reviewed Apr 4, 2022

View reviewed changes

compiler/rustc_metadata/src/rmeta/encoder.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Apr 4, 2022

View reviewed changes

compiler/rustc_middle/src/ty/mod.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Apr 4, 2022

View reviewed changes

bors added S-waiting-on-review 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 May 12, 2022

bors added S-waiting-on-bors 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 May 12, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label May 12, 2022

bors merged commit 481db40 into rust-lang:master May 12, 2022

rustbot added this to the 1.62.0 milestone May 12, 2022

rust-highfive mentioned this pull request May 12, 2022

miri no longer builds after rust-lang/rust#95562 #96975

Closed

lcnr deleted the attr-no-encode branch May 12, 2022 15:09

This was referenced May 12, 2022

[WIP] Rework the entire const trait system #96077

Closed

Retire ItemLikeVisitor trait #96825

Merged

Lint single-use lifetimes during AST resolution #96833

Merged

Replace #[default_method_body_is_const] with #[const_trait] #96964

Merged

celinval mentioned this pull request May 19, 2022

Update rust toolchain to 2022-05-17 model-checking/kani#1209

Merged

4 tasks

BGR360 reviewed Jul 5, 2022

View reviewed changes

celinval mentioned this pull request Sep 14, 2022

Add initial implementation of the reachability algorithm model-checking/kani#1683

Merged

4 tasks

oli-obk mentioned this pull request Dec 15, 2022

Make encode_attrs use queries instead of accessing the HIR #105496

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't encode only locally used attrs #95562

don't encode only locally used attrs #95562

lcnr commented Apr 1, 2022 •

edited

Loading

rust-highfive commented Apr 1, 2022

lcnr commented Apr 1, 2022

rust-timer commented Apr 1, 2022

bors commented Apr 1, 2022

bors commented Apr 1, 2022

rust-timer commented Apr 1, 2022

rust-timer commented Apr 1, 2022

rylev commented Apr 1, 2022

davidtwco commented Apr 3, 2022

rust-timer commented Apr 3, 2022

bors commented Apr 3, 2022

davidtwco left a comment

bors commented Apr 3, 2022

rust-timer commented Apr 3, 2022

rust-timer commented Apr 3, 2022

bjorn3 commented Apr 4, 2022

bjorn3 Apr 4, 2022

rust-log-analyzer commented May 12, 2022

bors commented May 12, 2022

lcnr commented May 12, 2022

bors commented May 12, 2022

bors commented May 12, 2022

rust-highfive commented May 12, 2022

rust-timer commented May 12, 2022

BGR360 Jul 5, 2022

lcnr Jul 6, 2022

BGR360 Jul 6, 2022

lcnr Jul 7, 2022

BGR360 Jul 7, 2022

	impl PartialEq<Symbol> for Path {
	#[inline]
	fn eq(&self, symbol: &Symbol) -> bool {
	self.segments.len() == 1 && { self.segments[0].ident.name == *symbol }
	}
	}

don't encode only locally used attrs #95562

don't encode only locally used attrs #95562

Conversation

lcnr commented Apr 1, 2022 • edited Loading

rust-highfive commented Apr 1, 2022

lcnr commented Apr 1, 2022

rust-timer commented Apr 1, 2022

bors commented Apr 1, 2022

bors commented Apr 1, 2022

rust-timer commented Apr 1, 2022

rust-timer commented Apr 1, 2022

Footnotes

rylev commented Apr 1, 2022

davidtwco commented Apr 3, 2022

rust-timer commented Apr 3, 2022

bors commented Apr 3, 2022

davidtwco left a comment

Choose a reason for hiding this comment

bors commented Apr 3, 2022

rust-timer commented Apr 3, 2022

rust-timer commented Apr 3, 2022

Footnotes

bjorn3 commented Apr 4, 2022

bjorn3 Apr 4, 2022

Choose a reason for hiding this comment

rust-log-analyzer commented May 12, 2022

bors commented May 12, 2022

lcnr commented May 12, 2022

bors commented May 12, 2022

bors commented May 12, 2022

rust-highfive commented May 12, 2022

rust-timer commented May 12, 2022

Footnotes

BGR360 Jul 5, 2022

Choose a reason for hiding this comment

lcnr Jul 6, 2022

Choose a reason for hiding this comment

BGR360 Jul 6, 2022

Choose a reason for hiding this comment

lcnr Jul 7, 2022

Choose a reason for hiding this comment

BGR360 Jul 7, 2022

Choose a reason for hiding this comment

lcnr commented Apr 1, 2022 •

edited

Loading