Skip to content
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.

Sign up for GitHub

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

Jump to bottom

try to not rely on attributes from extern crates #505

Closed
1 of 3 tasks
lcnr opened this issue Apr 6, 2022 · 3 comments
Closed
1 of 3 tasks

try to not rely on attributes from extern crates #505

lcnr opened this issue Apr 6, 2022 · 3 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@lcnr
Copy link
Contributor

lcnr commented Apr 6, 2022

Proposal

see zulip for some previous discussion.

We currently encode all attributes in the crate metadata, even if the attribute should only be needed in the local crate. I tried to change that in rust-lang/rust#95562 by filtering builtin attributes which should only be used in the local crate for the sake of performance. While that works, it is quite easy to accidentally miss one place where we use the attribute directly in another crate, causing surprising failures, see rust-lang/rust#95562 (comment) where that happened with #[inline].

Attributes are also generally not a great way to store information, considering that they have to be re-parsed each time they are used.

I would like to work towards not encoding attributes in the crate metadata at all, by adding specialized data structures for all attributes which are needed in other crates instead.

As that can't happen all at once, I propose the following steps:

  • land don't encode only locally used attrs rust#95562 with changes to get_attrs to also detect misuse, causing any incorrect accesses to cause an ICE
  • start to move existing attributes to a different encoding, also changing them to only_local
  • if the previous step continues to have a positive - or at least neutral - perf impact and does not negatively impact the maintainability of rustc, stop encoding attributes entirely and remove the item_attrs query

Mentors or Reviewers

If you have a reviewer or mentor in mind for this work, mention then
here. You can put your own name here if you are planning to mentor the
work.

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.
@lcnr lcnr added T-compiler Add this label so rfcbot knows to poll the compiler team major-change A proposal to make a major change to rustc labels Apr 6, 2022
@rustbot
Copy link
Collaborator

rustbot commented Apr 6, 2022

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Apr 6, 2022
@petrochenkov
Copy link

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Apr 6, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 7, 2022
@apiraino
Copy link
Contributor

@rustbot label -final-comment-period +major-change-accepted

@rustbot rustbot added major-change-accepted A major change proposal that was accepted to-announce Announce this issue on triage meeting and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Apr 18, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 21, 2022
@lcnr lcnr mentioned this issue Apr 26, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this issue May 12, 2022
@bors
Auto merge of rust-lang#95562 - lcnr:attr-no-encode, r=davidtwco
481db40 
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)
xFrednet pushed a commit to xFrednet/rust-clippy that referenced this issue May 15, 2022
@bors
Auto merge of #95562 - lcnr:attr-no-encode, r=davidtwco
7161a70 
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/rust#94963 (comment)
surechen added a commit to surechen/rust that referenced this issue Feb 23, 2024
@surechen
By changing some attributes to only_local, reducing encoding attribut…
9fc0257 
…es in the crate metadate.

Thank you.
This is part of changing attributes to only_local. I hope get your opinion  whether I should split into multiple PRs, or submit in one.

According to [try to not rely on attributes from extern crates](rust-lang/compiler-team#505) and lcnr's guidance.
@surechen surechen mentioned this issue Feb 23, 2024
Merged
surechen added a commit to surechen/rust that referenced this issue Feb 23, 2024
@surechen
By changing some attributes to only_local, reducing encoding attribut…
7df3949 
…es in the crate metadate.

Thank you.
This is part of changing attributes to only_local. I hope get your opinion  whether I should split into multiple PRs, or submit in one.

According to [try to not rely on attributes from extern crates](rust-lang/compiler-team#505) and lcnr's guidance.
surechen added a commit to surechen/rust that referenced this issue Feb 23, 2024
@surechen
By changing some attributes to only_local, reducing encoding attribut…
ccf789b 
…es in the crate metadate.

Thank you.
This is part of changing attributes to only_local. I hope get your opinion  whether I should split into multiple PRs, or submit in one.

According to [try to not rely on attributes from extern crates](rust-lang/compiler-team#505) and lcnr's guidance.
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 26, 2024
@bors
Auto merge of rust-lang#121493 - surechen:edit_attribute_only_local, …
87d28ff 
…r=<try>

By changing some attributes to only_local, reducing encoding attributes in the crate metadate.

Thank you.
This is part of changing attributes to only_local. I hope get your opinion  whether I should split into multiple PRs, or submit in one.

According to [try to not rely on attributes from extern crates](rust-lang/compiler-team#505) and lcnr's guidance.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 26, 2024
@matthiaskrgr
Rollup merge of rust-lang#121493 - surechen:edit_attribute_only_local…
6e3ece3 
…, r=lcnr

By changing some attributes to only_local, reducing encoding attributes in the crate metadate.

Thank you.
This is part of changing attributes to only_local. I hope get your opinion  whether I should split into multiple PRs, or submit in one.

According to [try to not rely on attributes from extern crates](rust-lang/compiler-team#505) and lcnr's guidance.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 26, 2024
@rust-timer
Unrolled build for rust-lang#121493
d7fefaa 
Rollup merge of rust-lang#121493 - surechen:edit_attribute_only_local, r=lcnr

By changing some attributes to only_local, reducing encoding attributes in the crate metadate.

Thank you.
This is part of changing attributes to only_local. I hope get your opinion  whether I should split into multiple PRs, or submit in one.

According to [try to not rely on attributes from extern crates](rust-lang/compiler-team#505) and lcnr's guidance.
surechen added a commit to surechen/rust that referenced this issue Feb 27, 2024
@surechen
Changing some attributes to only_local.
03a10a9 
Modified according to rust-lang/compiler-team#505.

By running test cases, I found that modifying the attribute's only_local tag sometimes causes some unintuitive error reports, so I tend to split it into multiple PRs and edit a small number of attributes each time to prevent too many changes at once. Prevent possible subsequent difficulties in locating problems.
@surechen surechen mentioned this issue Feb 27, 2024
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 27, 2024
@matthiaskrgr
Rollup merge of rust-lang#121661 - surechen:change_attribute_to_local…
324a8ff 
…_20240226, r=lcnr

Changing some attributes to only_local.

Modified according to rust-lang/compiler-team#505.

By running test cases, I found that modifying the attribute's only_local tag sometimes causes some unintuitive error reports, so I tend to split it into multiple PRs and edit a small number of attributes each time to prevent too many changes at once. Prevent possible subsequent difficulties in locating problems.

r? `@lcnr`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 27, 2024
@matthiaskrgr
Rollup merge of rust-lang#121661 - surechen:change_attribute_to_local…
17e9709 
…_20240226, r=lcnr

Changing some attributes to only_local.

Modified according to rust-lang/compiler-team#505.

By running test cases, I found that modifying the attribute's only_local tag sometimes causes some unintuitive error reports, so I tend to split it into multiple PRs and edit a small number of attributes each time to prevent too many changes at once. Prevent possible subsequent difficulties in locating problems.

r? ``@lcnr``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 27, 2024
@matthiaskrgr
Rollup merge of rust-lang#121661 - surechen:change_attribute_to_local…
9b84cbf 
…_20240226, r=lcnr

Changing some attributes to only_local.

Modified according to rust-lang/compiler-team#505.

By running test cases, I found that modifying the attribute's only_local tag sometimes causes some unintuitive error reports, so I tend to split it into multiple PRs and edit a small number of attributes each time to prevent too many changes at once. Prevent possible subsequent difficulties in locating problems.

r? ```@lcnr```
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Feb 28, 2024
@GuillaumeGomez
Rollup merge of rust-lang#121661 - surechen:change_attribute_to_local…
f6a68c8 
…_20240226, r=lcnr

Changing some attributes to only_local.

Modified according to rust-lang/compiler-team#505.

By running test cases, I found that modifying the attribute's only_local tag sometimes causes some unintuitive error reports, so I tend to split it into multiple PRs and edit a small number of attributes each time to prevent too many changes at once. Prevent possible subsequent difficulties in locating problems.

r? `@lcnr`
surechen added a commit to surechen/rust that referenced this issue Feb 28, 2024
@surechen
Changing some attributes to only_local.
11f9489 
Modified according to rust-lang/compiler-team#505.
@surechen surechen mentioned this issue Feb 28, 2024
Merged
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Feb 28, 2024
@GuillaumeGomez
Rollup merge of rust-lang#121661 - surechen:change_attribute_to_local…
796af69 
…_20240226, r=lcnr

Changing some attributes to only_local.

Modified according to rust-lang/compiler-team#505.

By running test cases, I found that modifying the attribute's only_local tag sometimes causes some unintuitive error reports, so I tend to split it into multiple PRs and edit a small number of attributes each time to prevent too many changes at once. Prevent possible subsequent difficulties in locating problems.

r? ``@lcnr``
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 28, 2024
@rust-timer
Unrolled build for rust-lang#121661
55a9dc3 
Rollup merge of rust-lang#121661 - surechen:change_attribute_to_local_20240226, r=lcnr

Changing some attributes to only_local.

Modified according to rust-lang/compiler-team#505.

By running test cases, I found that modifying the attribute's only_local tag sometimes causes some unintuitive error reports, so I tend to split it into multiple PRs and edit a small number of attributes each time to prevent too many changes at once. Prevent possible subsequent difficulties in locating problems.

r? ``@lcnr``
jhpratt added a commit to jhpratt/rust that referenced this issue Feb 29, 2024
@jhpratt
Rollup merge of rust-lang#121740 - surechen:change_attribute_to_local…
4010270 
…_20240228, r=lcnr

Changing some attributes to only_local.

Modified according to rust-lang/compiler-team#505.

r?  `@lcnr`
jhpratt added a commit to jhpratt/rust that referenced this issue Feb 29, 2024
@jhpratt
Rollup merge of rust-lang#121740 - surechen:change_attribute_to_local…
6971629 
…_20240228, r=lcnr

Changing some attributes to only_local.

Modified according to rust-lang/compiler-team#505.

r?  ``@lcnr``
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 29, 2024
@rust-timer
Unrolled build for rust-lang#121740
5e3b66d 
Rollup merge of rust-lang#121740 - surechen:change_attribute_to_local_20240228, r=lcnr

Changing some attributes to only_local.

Modified according to rust-lang/compiler-team#505.

r?  ``@lcnr``
surechen added a commit to surechen/rust that referenced this issue Mar 5, 2024
@surechen
Change some attributes to only_local.
c5a6842 
Modified according to rust-lang/compiler-team#505.
@surechen surechen mentioned this issue Mar 5, 2024
Merged
surechen added a commit to surechen/rust that referenced this issue Mar 5, 2024
@surechen
Change some attributes to only_local.
912c685 
Modified according to rust-lang/compiler-team#505.
surechen added a commit to surechen/rust that referenced this issue Mar 5, 2024
@surechen
Change some attributes to only_local.
79f0fde 
Modified according to rust-lang/compiler-team#505.
surechen added a commit to surechen/rust that referenced this issue Mar 5, 2024
@surechen
Change some attributes to only_local.
8dd126d 
Modified according to rust-lang/compiler-team#505.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 5, 2024
@matthiaskrgr
Rollup merge of rust-lang#122014 - surechen:change_attributes_to_loca…
59ada09 
…l_20240304, r=lcnr

Change some attributes to only_local.

Modified according to rust-lang/compiler-team#505.

r? `@lcnr`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 5, 2024
@matthiaskrgr
Rollup merge of rust-lang#122014 - surechen:change_attributes_to_loca…
c1bb406 
…l_20240304, r=lcnr

Change some attributes to only_local.

Modified according to rust-lang/compiler-team#505.

r? ``@lcnr``
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 6, 2024
@rust-timer
Unrolled build for rust-lang#122014
86c6651 
Rollup merge of rust-lang#122014 - surechen:change_attributes_to_local_20240304, r=lcnr

Change some attributes to only_local.

Modified according to rust-lang/compiler-team#505.

r? ``@lcnr``
@surechen surechen mentioned this issue Mar 14, 2024
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 14, 2024
@matthiaskrgr
Rollup merge of rust-lang#122477 - surechen:change_attribute_only_loc…
8303c6a 
…al_20240314, r=lcnr

Change some attribute to only_local

Modified according to rust-lang/compiler-team#505.

r? `@lcnr`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 14, 2024
@rust-timer
Unrolled build for rust-lang#122477
7ea4321 
Rollup merge of rust-lang#122477 - surechen:change_attribute_only_local_20240314, r=lcnr

Change some attribute to only_local

Modified according to rust-lang/compiler-team#505.

r? `@lcnr`
bors pushed a commit to rust-lang/miri that referenced this issue Mar 15, 2024
@matthiaskrgr
Rollup merge of #122477 - surechen:change_attribute_only_local_202403…
3050da6 
…14, r=lcnr

Change some attribute to only_local

Modified according to rust-lang/compiler-team#505.

r? `@lcnr`
@surechen surechen mentioned this issue Apr 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@petrochenkov @apiraino @lcnr @rustbot